Message ID | 1394134385-1727-14-git-send-email-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > 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 > >
On 17 March 2014 05:20, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> 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). The semantics allow you to leave out .resetvalue if it is 0; I think generally when I've written reginfo definitions I've tended to put in the .resetvalue as an indication of "I know this reginfo needs a reset value and it is zero" as opposed to "I didn't think about reset when I wrote this". We definitely don't want checkpatch to warn about explicit zero initializers in structs like this, otherwise we wouldn't be able to say ".crm = 0, .opc1 = 0, .opc2 = 0", which would be less comprehensible than writing them out explicitly I think. thanks -- PMM
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, .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) {
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 <peter.maydell@linaro.org> --- target-arm/cpu.h | 2 ++ target-arm/helper.c | 24 ++++++++++++++---------- 2 files changed, 16 insertions(+), 10 deletions(-)