Re: [Qemu-devel] [PATCH target-arm v2 09/13] target-arm: Add registers for PMSAv7

[Peter Maydell]

On 12 June 2015 at 20:10, Peter Crosthwaite
<address@hidden> wrote:
> Define the arm CP registers for PMSAv7 and their accessor functions.
> RGNR serves as a shared index that indexes into arrays storing the
> DRBAR, DRSR and DRACR registers. DRBAR and friends have to be VMSDd
> separately from the CP interface using a new PMSA specific VMSD
> subsection.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---

> +static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint32_t *u32p = *(uint32_t **)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 = *(uint32_t **)raw_ptr(env, ri);
> +
> +    u32p += env->cp15.c6_rgnr;
> +    tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +    *u32p = value;
> +}
> +
> +static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint32_t *u32p = *(uint32_t **)raw_ptr(env, ri);
> +
> +    memset(u32p, 0, sizeof(*u32p) * cpu->pmsav7_dregion);
> +}
> +
> +static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              uint64_t value)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint32_t nrgs = cpu->pmsav7_dregion;
> +
> +    if (value >= nrgs) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "PMSAv7 RGNR write > # supported regions, %" PRIu32
> +                      " > %" PRIu32 "\n", (uint32_t)value, nrgs);
> +        return;
> +    }
> +
> +    raw_write(env, ri, value);
> +}

I'm not wild about the pointer games going on in these
functions but I don't have a better suggestion.

> +static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
> +    { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
> +      .access = PL1_RW, .type = ARM_CP_NO_RAW,
> +      .fieldoffset = offsetof(CPUARMState, pmsav7.drbar),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = 
> pmsav7_reset },
> +    { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
> +      .access = PL1_RW, .type = ARM_CP_NO_RAW,
> +      .fieldoffset = offsetof(CPUARMState, pmsav7.drsr),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = 
> pmsav7_reset },
> +    { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
> +      .access = PL1_RW, .type = ARM_CP_NO_RAW,
> +      .fieldoffset = offsetof(CPUARMState, pmsav7.dracr),
> +      .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),
> +      .writefn = pmsav7_rgnr_write },
> +    REGINFO_SENTINEL
> +};
> +
>  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_ALIAS,
> @@ -3349,13 +3412,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_one_arm_cp_reg(cpu, &rvbar);
>      }
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
> -        /* These are the MPU registers prior to PMSAv6. Any new
> -         * PMSA core later than the ARM946 will require that we
> -         * implement the PMSAv6 or PMSAv7 registers, which are
> -         * completely different.
> -         */
> -        assert(!arm_feature(env, ARM_FEATURE_V6));
> -        define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
> +        if (arm_feature(env, ARM_FEATURE_V6)) {
> +            /* PMSAv6 not implemented */
> +            assert(arm_feature(env, ARM_FEATURE_V7));
> +            define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
> +            define_arm_cp_regs(cpu, pmsav7_cp_reginfo);
> +        } else {
> +            define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
> +        }
>      } else {
>          define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
>          define_arm_cp_regs(cpu, vmsa_cp_reginfo);
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 9446e5a..ae0f7b1 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -121,6 +121,38 @@ static const VMStateDescription vmstate_thumb2ee = {
>      }
>  };
>
> +static bool pmsav7_needed(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    CPUARMState *env = &cpu->env;
> +
> +    return arm_feature(env, ARM_FEATURE_MPU) &&
> +           arm_feature(env, ARM_FEATURE_V7);
> +}
> +
> +static bool pmsav7_rgnr_vmstate_validate(void *opaque, int version_id)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    return cpu->env.cp15.c6_rgnr < cpu->pmsav7_dregion;
> +}
> +
> +static const VMStateDescription vmstate_pmsav7 = {
> +    .name = "cpu/pmsav7",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_VARRAY_UINT32(env.pmsav7.drbar, ARMCPU, pmsav7_dregion, 0,
> +                              vmstate_info_uint32, uint32_t),
> +        VMSTATE_VARRAY_UINT32(env.pmsav7.drsr, ARMCPU, pmsav7_dregion, 0,
> +                              vmstate_info_uint32, uint32_t),
> +        VMSTATE_VARRAY_UINT32(env.pmsav7.dracr, ARMCPU, pmsav7_dregion, 0,
> +                              vmstate_info_uint32, uint32_t),
> +        VMSTATE_VALIDATE("rgnr is valid", pmsav7_rgnr_vmstate_validate),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static int get_cpsr(QEMUFile *f, void *opaque, size_t size)
>  {
>      ARMCPU *cpu = opaque;
> @@ -296,6 +328,9 @@ const VMStateDescription vmstate_arm_cpu = {
>              .vmsd = &vmstate_thumb2ee,
>              .needed = thumb2ee_needed,
>          } , {
> +            .vmsd = &vmstate_pmsav7,
> +            .needed = pmsav7_needed,
> +        } , {

You'll find this has changed as of commit 5cd8cadae8db -- the
needed function goes in the vmstate_pmsav7 VMStateDescription now.

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM