| Message ID | 1400491383-6725-17-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 19 May 2014 10:22, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Adds support for ERET to Aarch64 EL2 and 3. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/op_helper.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index d89755a..c632dd6 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -386,13 +386,13 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm) > > void HELPER(exception_return)(CPUARMState *env) > { > - unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(1) : 0; > + int cur_el = arm_current_pl(env); > + unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(cur_el) : 0; This will now allow the guest to trigger an assert() by doing an ERET in EL0... The fix for that is to put in the check in translate-a64.c, I think, since ERET in EL0 should be an UnallocatedEncoding. thanks -- PMM
On 19 May 2014 10:22, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Adds support for ERET to Aarch64 EL2 and 3. "AArch64". Also "to" here is ambiguous. The ARM ARM tries to keep the terminology straight to avoid confusion: exceptions are "taken from ELx" and "taken to ELx"; we "return from ELx" and "return to ELx". This looks like it's talking about "return to EL2/EL3" but probably doesn't really mean that. If this patch is adding support for ERET from EL2/EL3 it needs to also fix the bit in the "returning to an exception level which is 32 bit" which says "new_el = 0" since that's not guaranteed to be true any more. (Also I think the register mapping for AArch32 EL2/EL1 needs handling correctly.) > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/op_helper.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index d89755a..c632dd6 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -386,13 +386,13 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm) > > void HELPER(exception_return)(CPUARMState *env) > { > - unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(1) : 0; > + int cur_el = arm_current_pl(env); > + unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(cur_el) : 0; > uint32_t spsr = env->banked_spsr[spsr_idx]; > int new_el, i; > - int cur_el = arm_current_pl(env); > > if (env->pstate & PSTATE_SP) { > - env->sp_el[1] = env->xregs[31]; > + env->sp_el[cur_el] = env->xregs[31]; > } else { > env->sp_el[0] = env->xregs[31]; > } > @@ -429,7 +429,7 @@ void HELPER(exception_return)(CPUARMState *env) > env->aarch64 = 1; > pstate_write(env, spsr); > env->xregs[31] = env->sp_el[new_el]; > - env->pc = env->elr_el[1]; > + env->pc = env->elr_el[cur_el]; > } > > return; > @@ -443,7 +443,7 @@ illegal_return: > * no change to exception level, execution state or stack pointer > */ > env->pstate |= PSTATE_IL; > - env->pc = env->elr_el[1]; > + env->pc = env->elr_el[cur_el]; > spsr &= PSTATE_NZCV | PSTATE_DAIF; > spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF); > pstate_write(env, spsr); > -- > 1.8.3.2 > thanks -- PMM
On Wed, May 21, 2014 at 08:20:20PM +0100, Peter Maydell wrote: > On 19 May 2014 10:22, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Adds support for ERET to Aarch64 EL2 and 3. > > "AArch64". Also "to" here is ambiguous. The ARM ARM tries to Changed to AArch64. > keep the terminology straight to avoid confusion: exceptions > are "taken from ELx" and "taken to ELx"; we "return from ELx" > and "return to ELx". This looks like it's talking about "return to > EL2/EL3" but probably doesn't really mean that. > > If this patch is adding support for ERET from EL2/EL3 My first tests when starting booting in EL3 where to take exceptions and return back to EL3 and EL2, the commit log msg didn't evolve since but I guess I mean from and to EL2/3. Changed it to: Adds support for ERET to and from AArch64 EL2 and 3. If you have better suggestions I'm happy to change it to whatever. > it needs to also fix the bit in the "returning to an exception > level which is 32 bit" which says "new_el = 0" since that's > not guaranteed to be true any more. (Also I think the register > mapping for AArch32 EL2/EL1 needs handling correctly.) I've tried to stay away from touching too much of the AArch32 code as I haven't had a setup to test 64/32 transitions beyond a64/el1 and a32/el0. I do have a follow-up patch (not very tested) for ERET to aarch32. If you feel strongly about it I can squash the little I've got for aarch32 allthough I'd rather do it incrementally and leave it for follow-ups. Cheers, Edgar > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/op_helper.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > > index d89755a..c632dd6 100644 > > --- a/target-arm/op_helper.c > > +++ b/target-arm/op_helper.c > > @@ -386,13 +386,13 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm) > > > > void HELPER(exception_return)(CPUARMState *env) > > { > > - unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(1) : 0; > > + int cur_el = arm_current_pl(env); > > + unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(cur_el) : 0; > > uint32_t spsr = env->banked_spsr[spsr_idx]; > > int new_el, i; > > - int cur_el = arm_current_pl(env); > > > > if (env->pstate & PSTATE_SP) { > > - env->sp_el[1] = env->xregs[31]; > > + env->sp_el[cur_el] = env->xregs[31]; > > } else { > > env->sp_el[0] = env->xregs[31]; > > } > > @@ -429,7 +429,7 @@ void HELPER(exception_return)(CPUARMState *env) > > env->aarch64 = 1; > > pstate_write(env, spsr); > > env->xregs[31] = env->sp_el[new_el]; > > - env->pc = env->elr_el[1]; > > + env->pc = env->elr_el[cur_el]; > > } > > > > return; > > @@ -443,7 +443,7 @@ illegal_return: > > * no change to exception level, execution state or stack pointer > > */ > > env->pstate |= PSTATE_IL; > > - env->pc = env->elr_el[1]; > > + env->pc = env->elr_el[cur_el]; > > spsr &= PSTATE_NZCV | PSTATE_DAIF; > > spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF); > > pstate_write(env, spsr); > > -- > > 1.8.3.2 > > > > thanks > -- PMM
On Wed, May 21, 2014 at 08:10:53PM +0100, Peter Maydell wrote: > On 19 May 2014 10:22, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Adds support for ERET to Aarch64 EL2 and 3. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/op_helper.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > > index d89755a..c632dd6 100644 > > --- a/target-arm/op_helper.c > > +++ b/target-arm/op_helper.c > > @@ -386,13 +386,13 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm) > > > > void HELPER(exception_return)(CPUARMState *env) > > { > > - unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(1) : 0; > > + int cur_el = arm_current_pl(env); > > + unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(cur_el) : 0; > > This will now allow the guest to trigger an assert() by doing an > ERET in EL0... The fix for that is to put in the check in translate-a64.c, > I think, since ERET in EL0 should be an UnallocatedEncoding. Nice catch. I've prepended this patch with a patch traping eret in el0 at translation time. Thanks, Edgar
On 22 May 2014 01:48, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Wed, May 21, 2014 at 08:20:20PM +0100, Peter Maydell wrote: >> it needs to also fix the bit in the "returning to an exception >> level which is 32 bit" which says "new_el = 0" since that's >> not guaranteed to be true any more. (Also I think the register >> mapping for AArch32 EL2/EL1 needs handling correctly.) > > I've tried to stay away from touching too much of the AArch32 > code as I haven't had a setup to test 64/32 transitions > beyond a64/el1 and a32/el0. OK; if we put in a TODO comment that we assume EL1..EL3 are 64 bit currently, we'll have a marker to come back and fix later. thanks -- PMM
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index d89755a..c632dd6 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -386,13 +386,13 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm) void HELPER(exception_return)(CPUARMState *env) { - unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(1) : 0; + int cur_el = arm_current_pl(env); + unsigned int spsr_idx = is_a64(env) ? aarch64_banked_spsr_index(cur_el) : 0; uint32_t spsr = env->banked_spsr[spsr_idx]; int new_el, i; - int cur_el = arm_current_pl(env); if (env->pstate & PSTATE_SP) { - env->sp_el[1] = env->xregs[31]; + env->sp_el[cur_el] = env->xregs[31]; } else { env->sp_el[0] = env->xregs[31]; } @@ -429,7 +429,7 @@ void HELPER(exception_return)(CPUARMState *env) env->aarch64 = 1; pstate_write(env, spsr); env->xregs[31] = env->sp_el[new_el]; - env->pc = env->elr_el[1]; + env->pc = env->elr_el[cur_el]; } return; @@ -443,7 +443,7 @@ illegal_return: * no change to exception level, execution state or stack pointer */ env->pstate |= PSTATE_IL; - env->pc = env->elr_el[1]; + env->pc = env->elr_el[cur_el]; spsr &= PSTATE_NZCV | PSTATE_DAIF; spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF); pstate_write(env, spsr);