Re: [Qemu-devel] [PATCH target-arm v1 4/9] target-arm: Add registers for PMSAv7

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <address@hidden> wrote:
> define the arm CP registers for PMSAv7 and their accessor functions.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>  target-arm/cpu.h    |  6 ++++++
>  target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 09cc16d..9cb2e49 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -286,6 +286,12 @@ typedef struct CPUARMState {
>              };
>              uint64_t par_el[4];
>          };
> +
> +        uint32_t c6_rgnr;
> +        uint32_t c6_drbar[PMSAV7_MPU_NUM_REGIONS];
> +        uint32_t c6_drsr[PMSAV7_MPU_NUM_REGIONS];
> +        uint32_t c6_dracr[PMSAV7_MPU_NUM_REGIONS];
> +
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
>          uint64_t c9_pmcr; /* performance monitor control register */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index cb21bbf..f11efea 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1709,6 +1709,54 @@ static uint64_t pmsav5_insn_ap_read(CPUARMState *env, 
> const ARMCPRegInfo *ri)
>      return simple_mpu_ap_bits(env->cp15.pmsav5_insn_ap);
>  }
>
> +static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint32_t *u32p = (void *)env + ri->fieldoffset;

This is raw_ptr(env, ri);

> +
> +    u32p += env->cp15.c6_rgnr;
> +    return *u32p;
> +}
> +
> +static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
> +
> +    u32p += env->cp15.c6_rgnr;
> +    tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +    *u32p = value;

Since you're not boundary-checking c6_rgnr either when the guest
tries to set it (or on inbound migration) or at point of use,
this is a handy way for the guest to do controlled writes to
anywhere in QEMU's address space...

> +}
> +
> +static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    int i;
> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
> +
> +    for (i = 0; i < 16; ++i) {

You have a #define for number of regions...
In any case you could just memset() the whole thing rather
than looping.

> +        u32p[i] = 0;
> +    }
> +}
> +
> +static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
> +    { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_drbar[0]),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = 
> pmsav7_reset },
> +    { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_drsr[0]),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = 
> pmsav7_reset },
> +    { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_dracr[0]),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = 
> pmsav7_reset },
> +    { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr), .resetvalue = 0, },
> +    REGINFO_SENTINEL
> +};

This won't handle migration/state save/load properly.

-- PMM