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

[Peter Crosthwaite]

On Mon, Jun 1, 2015 at 11:56 AM, Peter Maydell <address@hidden> wrote:
> 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);
>

Fixed.

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

Boundary check added at time of set.

>> +}
>> +
>> +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.
>

Memset added. Hard constant removed.

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

So this was non trivial in the end. I have marked the arrays as
ARM_CP_NO_RAW as they simply don't have state in their own right. I
have then added a subsection in machine.c that VMSTATEs the arrays. I
could have used VMSTATE_UINT32_ARRAY but decided to refactor the
series to have the MPU number or regions available as variable in
ARMCPU. This means we can get the VMSTATE right straight away with a
VMSTATE_VARRAY for these arrays.

The tricky part is the RGNR incoming boundary validation. I have added
this to the new subsection but it is disjunct from the actual VMSD for
RGNR itself which just uses the automatic CP reginfo VMSD.

Regards,
Peter


> -- PMM
>