Re: [Qemu-devel] [PATCH v3 16/22] target-arm: A64: Generalize ERET to various ELs

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

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