From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPPyO-00023l-5D for qemu-devel@nongnu.org; Mon, 17 Mar 2014 01:20:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPPyJ-0004E1-CH for qemu-devel@nongnu.org; Mon, 17 Mar 2014 01:20:28 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:35063) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPPyI-0004Dt-U7 for qemu-devel@nongnu.org; Mon, 17 Mar 2014 01:20:23 -0400 Received: by mail-wi0-f173.google.com with SMTP id f8so1626618wiw.0 for ; Sun, 16 Mar 2014 22:20:22 -0700 (PDT) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <1394134385-1727-14-git-send-email-peter.maydell@linaro.org> References: <1394134385-1727-1-git-send-email-peter.maydell@linaro.org> <1394134385-1727-14-git-send-email-peter.maydell@linaro.org> Date: Mon, 17 Mar 2014 15:20:21 +1000 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH v4 13/21] target-arm: Use dedicated CPU state fields for ARM946 access bit registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Rob Herring , Patch Tracking , Michael Matz , Claudio Fontana , Alexander Graf , "qemu-devel@nongnu.org Developers" , Laurent Desnogues , Dirk Mueller , Will Newton , =?ISO-8859-1?Q?Alex_Benn=E9e?= , "kvmarm@lists.cs.columbia.edu" , Christoffer Dall , Richard Henderson On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell wrote: > The ARM946 model currently uses the c5_data and c5_insn fields in the CPU > state struct to store the contents of its access permission registers. > This is confusing and a good source of bugs because for all the MMU-based > CPUs those fields are fault status and fault address registers, which > behave completely differently; they just happen to use the same cpreg > encoding. Split them out to use their own fields instead. > > These registers are only present in PMSAv5 MPU systems (of which the > ARM946 is our only current example); PMSAv6 and PMSAv7 (which we have > no implementations of) handle access permissions differently. We name > the new state fields accordingly. > > Note that this change fixes a bug where a data abort or prefetch abort > on the ARM946 would accidentally corrupt the access permission registers > because the interrupt handling code assumed the c5_data and c5_insn > fields were always fault status registers. > > Signed-off-by: Peter Maydell Reviewed-by: Peter Crosthwaite > --- > target-arm/cpu.h | 2 ++ > target-arm/helper.c | 24 ++++++++++++++---------- > 2 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index fa826c4..ffa4b37 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -179,6 +179,8 @@ typedef struct CPUARMState { > uint32_t c2_insn; /* MPU instruction cachable bits. */ > uint32_t c3; /* MMU domain access control register > MPU write buffer control. */ > + uint32_t pmsav5_data_ap; /* PMSAv5 MPU data access permissions */ > + uint32_t pmsav5_insn_ap; /* PMSAv5 MPU insn access permissions */ > uint32_t c5_insn; /* Fault status registers. */ > uint32_t c5_data; > uint32_t c6_region[8]; /* MPU base/size registers. */ > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 45e6910..cbef0e5 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -1183,40 +1183,44 @@ static uint32_t extended_mpu_ap_bits(uint32_t val) > static void pmsav5_data_ap_write(CPUARMState *env, const ARMCPRegInfo *ri, > uint64_t value) > { > - env->cp15.c5_data = extended_mpu_ap_bits(value); > + env->cp15.pmsav5_data_ap = extended_mpu_ap_bits(value); > } > > static uint64_t pmsav5_data_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > - return simple_mpu_ap_bits(env->cp15.c5_data); > + return simple_mpu_ap_bits(env->cp15.pmsav5_data_ap); > } > > static void pmsav5_insn_ap_write(CPUARMState *env, const ARMCPRegInfo *ri, > uint64_t value) > { > - env->cp15.c5_insn = extended_mpu_ap_bits(value); > + env->cp15.pmsav5_insn_ap = extended_mpu_ap_bits(value); > } > > static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > - return simple_mpu_ap_bits(env->cp15.c5_insn); > + return simple_mpu_ap_bits(env->cp15.pmsav5_insn_ap); > } > > static const ARMCPRegInfo pmsav5_cp_reginfo[] = { > { .name = "DATA_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0, > .access = PL1_RW, .type = ARM_CP_NO_MIGRATE, > - .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0, > + .fieldoffset = offsetof(CPUARMState, cp15.pmsav5_data_ap), > + .resetvalue = 0, I know its just motion of existing code, but what's the policy on zero resets? Can we leave them out for brevity? Checkpatch complains when a global is explictly 0 initialized, so it seems sane that the same rule applies to individual fields (just checkpatch probably has hard time figuring this one out). Regards, Peter > .readfn = pmsav5_data_ap_read, .writefn = pmsav5_data_ap_write, }, > { .name = "INSN_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 1, > .access = PL1_RW, .type = ARM_CP_NO_MIGRATE, > - .fieldoffset = offsetof(CPUARMState, cp15.c5_insn), .resetvalue = 0, > + .fieldoffset = offsetof(CPUARMState, cp15.pmsav5_insn_ap), > + .resetvalue = 0, > .readfn = pmsav5_insn_ap_read, .writefn = pmsav5_insn_ap_write, }, > { .name = "DATA_EXT_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 2, > .access = PL1_RW, > - .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0, }, > + .fieldoffset = offsetof(CPUARMState, cp15.pmsav5_data_ap), > + .resetvalue = 0, }, > { .name = "INSN_EXT_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 3, > .access = PL1_RW, > - .fieldoffset = offsetof(CPUARMState, cp15.c5_insn), .resetvalue = 0, }, > + .fieldoffset = offsetof(CPUARMState, cp15.pmsav5_insn_ap), > + .resetvalue = 0, }, > { .name = "DCACHE_CFG", .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0, > .access = PL1_RW, > .fieldoffset = offsetof(CPUARMState, cp15.c2_data), .resetvalue = 0, }, > @@ -3568,9 +3572,9 @@ static int get_phys_addr_mpu(CPUARMState *env, uint32_t address, > return 2; > > if (access_type == 2) { > - mask = env->cp15.c5_insn; > + mask = env->cp15.pmsav5_insn_ap; > } else { > - mask = env->cp15.c5_data; > + mask = env->cp15.pmsav5_data_ap; > } > mask = (mask >> (n * 4)) & 0xf; > switch (mask) { > -- > 1.9.0 > >