I agree, if we are going to allow for EL-based array indexing then the indices should correspond to the actual EL for clarity.
I am fine with the above compromise. On 17 June 2014 01:12, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote: > 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 <aggel...@ethz.ch> 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 <aggel...@ethz.ch> > > > --- > > > 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 > > > > > > >
