Re: [Qemu-devel] [PATCH v3 28/32] target-arm: make DFSR banked

[Edgar E. Iglesias]

On Fri, Jun 13, 2014 at 05:06:15PM -0500, Greg Bellows wrote:
> I just wanted to point out that the change from array-notation to hard-code
> numbers in the names undoes Edgar's EL2/EL3 changes.  I prefer this way
> over the array notation.

Hi,

This was discussed briefly here
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03561.html

IMO, for some regs the array version doesn't make sense but for regs
that need to be indexed by EL it does. Just look at what this patch
results in for aarch64_cpu_do_interrupt(). AArch64 has a simpler/cleaner
architecture in this respect, IMO we should keep the
AArch64 simple and clean and take the banking pain in the AArch32 port.


> 
> 
> On 10 June 2014 18:55, Fabian Aggeler <address@hidden> wrote:
> 
> > When EL3 is running in Aarch32 (or ARMv7 with Security Extensions)
> > DFSR has a secure and a non-secure instance.
> >
> > Signed-off-by: Fabian Aggeler <address@hidden>
> > ---
> >  target-arm/cpu.h        | 13 ++++++++++++-
> >  target-arm/helper-a64.c | 17 ++++++++++++++---
> >  target-arm/helper.c     | 15 ++++++++-------
> >  3 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 54c51a4..71782cf 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -266,7 +266,18 @@ typedef struct CPUARMState {
> >                  uint32_t ifsr32_el2;
> >              };
> >          };
> > -        uint64_t esr_el[4];
> > +        union {
> > +            struct {
> > +                uint64_t dfsr_ns;
> > +                uint64_t hsr;
> > +                uint64_t dfsr_s;
> > +            };
> > +            struct {
> > +                uint64_t esr_el1;
> > +                uint64_t esr_el2;
> > +                uint64_t esr_el3;
> > +            };
> > +        };

I'd prefer:

-        uint64_t esr_el[4];
+        union {
+            struct {
+                uint64_t dummy 
+                uint64_t dfsr_ns;
+                uint64_t hsr;
+                uint64_t dfsr_s;
+            };
+            struct {
+                uint64_t esr_el[4];
+            };
+        };

And avoid the whole target_esr pointer thing in aarch64_cpu_do_interrupt().

Cheers,
Edgar


> >          uint32_t c6_region[8]; /* MPU base/size registers.  */
> >          uint64_t far_el[4]; /* Fault address registers.  */
> >          uint64_t par_el1;  /* Translation result. */
> > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> > index d7522b6..dbbf012 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -447,6 +447,18 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      target_ulong addr = env->cp15.vbar_el[new_el];
> >      unsigned int new_mode = aarch64_pstate_mode(new_el, true);
> >      int i;
> > +    uint64_t *target_esr;
> > +    switch (new_el) {
> > +    case 3:
> > +        target_esr = &env->cp15.esr_el3;
> > +        break;
> > +    case 2:
> > +        target_esr = &env->cp15.esr_el2;
> > +        break;
> > +    case 1:
> > +        target_esr = &env->cp15.esr_el1;
> > +        break;
> > +    }
> >
> >      if (arm_current_pl(env) < new_el) {
> >          if (env->aarch64) {
> > @@ -477,8 +489,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      case EXCP_SWI:
> >      case EXCP_HVC:
> >      case EXCP_SMC:
> > -        env->cp15.esr_el[new_el] = env->exception.syndrome;
> > -        break;
> > +        *target_esr = env->exception.syndrome;
> >      case EXCP_IRQ:
> >      case EXCP_VIRQ:
> >          addr += 0x80;
> > @@ -498,7 +509,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      } else {
> >          env->banked_spsr[0] = cpsr_read(env);
> >          if (!env->thumb) {
> > -            env->cp15.esr_el[new_el] |= 1 << 25;
> > +            *target_esr |= 1 << 25;
> >          }
> >          env->elr_el[new_el] = env->regs[15];
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index f51498a..793985e 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -1492,7 +1492,8 @@ static void vmsa_ttbr_write(CPUARMState *env, const
> > ARMCPRegInfo *ri,
> >  static const ARMCPRegInfo vmsa_cp_reginfo[] = {
> >      { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
> >        .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
> > -      .fieldoffset = offsetoflow32(CPUARMState, cp15.esr_el[1]),
> > +      .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dfsr_s),
> > +                             offsetoflow32(CPUARMState, cp15.dfsr_ns) },
> >        .resetfn = arm_cp_reset_ignore, },
> >      { .name = "IFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 1,
> >        .access = PL1_RW, .resetvalue = 0,
> > @@ -1501,7 +1502,7 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
> >      { .name = "ESR_EL1", .state = ARM_CP_STATE_AA64,
> >        .opc0 = 3, .crn = 5, .crm = 2, .opc1 = 0, .opc2 = 0,
> >        .access = PL1_RW,
> > -      .fieldoffset = offsetof(CPUARMState, cp15.esr_el[1]), .resetvalue =
> > 0, },
> > +      .fieldoffset = offsetof(CPUARMState, cp15.esr_el1), .resetvalue =
> > 0, },
> >      { .name = "TTBR0_EL1", .state = ARM_CP_STATE_BOTH,
> >        .opc0 = 3, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
> >        .access = PL1_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0,
> > @@ -1565,7 +1566,7 @@ static void omap_cachemaint_write(CPUARMState *env,
> > const ARMCPRegInfo *ri,
> >  static const ARMCPRegInfo omap_cp_reginfo[] = {
> >      { .name = "DFSR", .cp = 15, .crn = 5, .crm = CP_ANY,
> >        .opc1 = CP_ANY, .opc2 = CP_ANY, .access = PL1_RW, .type =
> > ARM_CP_OVERRIDE,
> > -      .fieldoffset = offsetoflow32(CPUARMState, cp15.esr_el[1]),
> > +      .fieldoffset = offsetoflow32(CPUARMState, cp15.esr_el1),
> >        .resetvalue = 0, },
> >      { .name = "", .cp = 15, .crn = 15, .crm = 0, .opc1 = 0, .opc2 = 0,
> >        .access = PL1_RW, .type = ARM_CP_NOP },
> > @@ -2187,7 +2188,7 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> >      { .name = "ESR_EL2", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 4, .crn = 5, .crm = 2, .opc2 = 0,
> > -      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState,
> > cp15.esr_el[2]) },
> > +      .access = PL2_RW, .fieldoffset = offsetof(CPUARMState,
> > cp15.esr_el2) },
> >      { .name = "FAR_EL2", .state = ARM_CP_STATE_AA64,
> >        .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 0,
> >        .access = PL2_RW, .fieldoffset = offsetof(CPUARMState,
> > cp15.far_el[2]) },
> > @@ -2299,7 +2300,7 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
> >      { .name = "ESR_EL3", .state = ARM_CP_STATE_AA64,
> >        .type = ARM_CP_NO_MIGRATE,
> >        .opc0 = 3, .opc1 = 6, .crn = 5, .crm = 2, .opc2 = 0,
> > -      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState,
> > cp15.esr_el[3]) },
> > +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState,
> > cp15.esr_el3) },
> >      { .name = "FAR_EL3", .state = ARM_CP_STATE_AA64,
> >        .opc0 = 3, .opc1 = 6, .crn = 6, .crm = 0, .opc2 = 0,
> >        .access = PL3_RW, .fieldoffset = offsetof(CPUARMState,
> > cp15.far_el[3]) },
> > @@ -3847,11 +3848,11 @@ void arm_cpu_do_interrupt(CPUState *cs)
> >          offset = 4;
> >          break;
> >      case EXCP_DATA_ABORT:
> > -        env->cp15.esr_el[1] = env->exception.fsr;
> > +        A32_BANKED_CURRENT_REG_SET(env, dfsr, env->exception.fsr);
> >          env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 0, 32,
> >                                          env->exception.vaddress);
> >          qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
> > -                      (uint32_t)env->cp15.esr_el[1],
> > +                      env->exception.fsr,
> >                        (uint32_t)env->exception.vaddress);
> >          new_mode = ARM_CPU_MODE_ABT;
> >          addr = 0x10;
> > --
> > 1.8.3.2
> >
> >