From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39194) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwmlF-00024V-QB for qemu-devel@nongnu.org; Tue, 17 Jun 2014 02:20:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WwmlB-0002nd-4f for qemu-devel@nongnu.org; Tue, 17 Jun 2014 02:20:49 -0400 Received: from mail-pb0-x229.google.com ([2607:f8b0:400e:c01::229]:50612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwmlA-0002nX-R9 for qemu-devel@nongnu.org; Tue, 17 Jun 2014 02:20:45 -0400 Received: by mail-pb0-f41.google.com with SMTP id ma3so5347001pbc.28 for ; Mon, 16 Jun 2014 23:20:43 -0700 (PDT) Date: Tue, 17 Jun 2014 08:12:32 +0200 From: "Edgar E. Iglesias" Message-ID: <20140617061232.GD10398@toto> References: <1402444514-19658-1-git-send-email-aggelerf@ethz.ch> <1402444514-19658-29-git-send-email-aggelerf@ethz.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 28/32] target-arm: make DFSR banked List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Bellows Cc: Peter Maydell , Peter Crosthwaite , Fabian Aggeler , QEMU Developers , Sergey Fedorov , Christoffer Dall 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 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 > > --- > > 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 > > > >