Re: [Qemu-devel] [PATCH v1 02/22] target-arm: Make elr_el1 an array

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

On Wed, May 07, 2014 at 03:10:54PM +1000, Peter Crosthwaite wrote:
> On Tue, May 6, 2014 at 4:08 PM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > No functional change.
> > Prepares for future additions of the EL2 and 3 versions of this reg.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h        | 3 ++-
> >  target-arm/helper-a64.c | 4 ++--
> >  target-arm/helper.c     | 3 ++-
> >  target-arm/kvm64.c      | 4 ++--
> >  target-arm/machine.c    | 2 +-
> >  target-arm/op_helper.c  | 6 +++---
> >  6 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index c83f249..eb7a0f5 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -162,7 +162,8 @@ typedef struct CPUARMState {
> >      uint32_t condexec_bits; /* IT bits.  cpsr[15:10,26:25].  */
> >      uint64_t daif; /* exception masks, in the bits they are in in PSTATE */
> >
> > -    uint64_t elr_el1; /* AArch64 ELR_EL1 */
> > +#define ELR_EL_IDX(x) (x - 1)
> > +    uint64_t elr_el[1]; /* AArch64 exception link regs  */
> 
> Is it perhaps just easier to waste the space and always pad these
> EL-banked CP arrays out to length 4 you can just use literal numbers
> in the code? Probably make life easier when introspecting the CPU
> state in GDB too.

Thanks Peter,

I've fixed all your comments except this one. I considered this
pattern but avoided it due to the bloating of CPUARMState. Anyway,
I'm happy to change to this full array allocation if others agree.

PMM, what is your preference on this?

Best regards,
Edgar


> 
> Regards,
> Peter
> 
> >      uint64_t sp_el[2]; /* AArch64 banked stack pointers */
> >
> >      /* System control coprocessor (cp15) */
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index bf921cc..5adf2b5 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -491,13 +491,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >          env->banked_spsr[0] = pstate_read(env);
> >          env->sp_el[arm_current_pl(env)] = env->xregs[31];
> >          env->xregs[31] = env->sp_el[1];
> > -        env->elr_el1 = env->pc;
> > +        env->elr_el[ELR_EL_IDX(1)] = env->pc;
> >      } else {
> >          env->banked_spsr[0] = cpsr_read(env);
> >          if (!env->thumb) {
> >              env->cp15.esr_el1 |= 1 << 25;
> >          }
> > -        env->elr_el1 = env->regs[15];
> > +        env->elr_el[ELR_EL_IDX(1)] = env->regs[15];
> >
> >          for (i = 0; i < 15; i++) {
> >              env->xregs[i] = env->regs[i];
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 3be917c..3457d3e 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -2055,7 +2055,8 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
> >      { .name = "ELR_EL1", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 1,
> > -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, elr_el1) },
> > +      .access = PL1_RW,
> > +      .fieldoffset = offsetof(CPUARMState, elr_el[ELR_EL_IDX(1)]) },
> >      { .name = "SPSR_EL1", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 0,
> > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> > index e115879..da376cf 100644
> > --- a/target-arm/kvm64.c
> > +++ b/target-arm/kvm64.c
> > @@ -161,7 +161,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >      }
> >
> >      reg.id = AARCH64_CORE_REG(elr_el1);
> > -    reg.addr = (uintptr_t) &env->elr_el1;
> > +    reg.addr = (uintptr_t) &env->elr_el[ELR_EL_IDX(1)];
> >      ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> >      if (ret) {
> >          return ret;
> > @@ -241,7 +241,7 @@ int kvm_arch_get_registers(CPUState *cs)
> >      }
> >
> >      reg.id = AARCH64_CORE_REG(elr_el1);
> > -    reg.addr = (uintptr_t) &env->elr_el1;
> > +    reg.addr = (uintptr_t) &env->elr_el[ELR_EL_IDX(1)];
> >      ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> >      if (ret) {
> >          return ret;
> > diff --git a/target-arm/machine.c b/target-arm/machine.c
> > index b967223..8b299a0 100644
> > --- a/target-arm/machine.c
> > +++ b/target-arm/machine.c
> > @@ -243,7 +243,7 @@ const VMStateDescription vmstate_arm_cpu = {
> >          VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 6),
> >          VMSTATE_UINT32_ARRAY(env.usr_regs, ARMCPU, 5),
> >          VMSTATE_UINT32_ARRAY(env.fiq_regs, ARMCPU, 5),
> > -        VMSTATE_UINT64(env.elr_el1, ARMCPU),
> > +        VMSTATE_UINT64(env.elr_el[ELR_EL_IDX(1)], ARMCPU),
> >          VMSTATE_UINT64_ARRAY(env.sp_el, ARMCPU, 2),
> >          /* The length-check must come before the arrays to avoid
> >           * incoming data possibly overflowing the array.
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index fb90676..21545d0 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -406,7 +406,7 @@ void HELPER(exception_return)(CPUARMState *env)
> >              env->regs[i] = env->xregs[i];
> >          }
> >
> > -        env->regs[15] = env->elr_el1 & ~0x1;
> > +        env->regs[15] = env->elr_el[ELR_EL_IDX(1)] & ~0x1;
> >      } else {
> >          new_el = extract32(spsr, 2, 2);
> >          if (new_el > 1) {
> > @@ -424,7 +424,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_el1;
> > +        env->pc = env->elr_el[ELR_EL_IDX(1)];
> >      }
> >
> >      return;
> > @@ -438,7 +438,7 @@ illegal_return:
> >       * no change to exception level, execution state or stack pointer
> >       */
> >      env->pstate |= PSTATE_IL;
> > -    env->pc = env->elr_el1;
> > +    env->pc = env->elr_el[ELR_EL_IDX(1)];
> >      spsr &= PSTATE_NZCV | PSTATE_DAIF;
> >      spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
> >      pstate_write(env, spsr);
> > --
> > 1.8.3.2
> >
> >