Re: [Qemu-devel] [PATCH v4 13/21] target-arm: Use dedicated CPU state fields for ARM946 access bit registers

[Peter Crosthwaite <peter.crosthwaite@xilinx.com>]

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
>
>