On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@xxxxxxxxxx> 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@xxxxxxxxxx>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xxxxxxxxxx>
> ---
> 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
>
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
