Re: [Qemu-devel] [PATCH v2 12/35] target-arm: Convert performance monitor reginfo to accesfn

[Peter Crosthwaite]

cc Alistair, this may conflict with his timer work.

On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> wrote:
> Convert the performance monitor reginfo definitions to use
> an accessfn rather than returning EXCP_UDEF from read and
> write functions. This also allows us to fix a couple of XXX
> cases where we weren't imposing the access restrictions on
> RAZ/WI or constant registers.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target-arm/helper.c | 70 
> +++++++++++++++++++++--------------------------------
>  1 file changed, 28 insertions(+), 42 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index bd78350..9adaeb9 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -485,26 +485,20 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> -
> -static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
> -                      uint64_t *value)
> +static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> -    /* Generic performance monitor register read function for where
> -     * user access may be allowed by PMUSERENR.
> +    /* Perfomance monitor registers user accessibility is controlled
> +     * by PMUSERENR.
>       */
>      if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> -        return EXCP_UDEF;
> +        return CP_ACCESS_TRAP;
>      }
> -    *value = CPREG_FIELD32(env, ri);
> -    return 0;
> +    return CP_ACCESS_OK;
>  }
>
>  static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                        uint64_t value)
>  {
> -    if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> -        return EXCP_UDEF;
> -    }
>      /* only the DP, X, D and E bits are writable */
>      env->cp15.c9_pmcr &= ~0x39;
>      env->cp15.c9_pmcr |= (value & 0x39);
> @@ -514,9 +508,6 @@ static int pmcr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static int pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> -    if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> -        return EXCP_UDEF;
> -    }
>      value &= (1 << 31);
>      env->cp15.c9_pmcnten |= value;
>      return 0;
> @@ -525,9 +516,6 @@ static int pmcntenset_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static int pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> -    if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> -        return EXCP_UDEF;
> -    }
>      value &= (1 << 31);
>      env->cp15.c9_pmcnten &= ~value;
>      return 0;
> @@ -536,9 +524,6 @@ static int pmcntenclr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static int pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                          uint64_t value)
>  {
> -    if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> -        return EXCP_UDEF;
> -    }
>      env->cp15.c9_pmovsr &= ~value;
>      return 0;
>  }
> @@ -546,9 +531,6 @@ static int pmovsr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static int pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> -    if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> -        return EXCP_UDEF;
> -    }
>      env->cp15.c9_pmxevtyper = value & 0xff;
>      return 0;
>  }
> @@ -624,37 +606,41 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>      { .name = "PMCNTENSET", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 
> = 1,
>        .access = PL0_RW, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
> -      .readfn = pmreg_read, .writefn = pmcntenset_write,
> -      .raw_readfn = raw_read, .raw_writefn = raw_write },
> +      .writefn = pmcntenset_write,
> +      .accessfn = pmreg_access,
> +      .raw_writefn = raw_write },

A nit but,

You're field ordering scheme is inconsistent, here you go, write ->
access -> raw_write ....

>      { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 
> = 2,
>        .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, 
> cp15.c9_pmcnten),
> -      .readfn = pmreg_read, .writefn = pmcntenclr_write,
> +      .accessfn = pmreg_access,
> +      .writefn = pmcntenclr_write,
>        .type = ARM_CP_NO_MIGRATE },
>      { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
>        .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> -      .readfn = pmreg_read, .writefn = pmovsr_write,
> -      .raw_readfn = raw_read, .raw_writefn = raw_write },
> -    /* Unimplemented so WI. Strictly speaking write accesses in PL0 should
> -     * respect PMUSERENR.
> -     */
> +      .accessfn = pmreg_access,
> +      .writefn = pmovsr_write,
> +      .raw_writefn = raw_write },

... and this is access -> write -> raw_write. Is there a prescribed
order to keep new (or gradually refactored) code consistent?

Regards,
Peter

> +    /* Unimplemented so WI. */
>      { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
> -      .access = PL0_W, .type = ARM_CP_NOP },
> +      .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
>      /* Since we don't implement any events, writing to PMSELR is 
> UNPREDICTABLE.
> -     * We choose to RAZ/WI. XXX should respect PMUSERENR.
> +     * We choose to RAZ/WI.
>       */
>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> -    /* Unimplemented, RAZ/WI. XXX PMUSERENR */
> +      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> +      .accessfn = pmreg_access },
> +    /* Unimplemented, RAZ/WI. */
>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> +      .accessfn = pmreg_access },
>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 
> = 1,
>        .access = PL0_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> -      .readfn = pmreg_read, .writefn = pmxevtyper_write,
> -      .raw_readfn = raw_read, .raw_writefn = raw_write },
> -    /* Unimplemented, RAZ/WI. XXX PMUSERENR */
> +      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
> +      .raw_writefn = raw_write },
> +    /* Unimplemented, RAZ/WI. */
>      { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 
> 2,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> +      .accessfn = pmreg_access },
>      { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 
> 0,
>        .access = PL0_R | PL1_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
> @@ -1708,8 +1694,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 
> = 0,
>              .access = PL0_RW, .resetvalue = cpu->midr & 0xff000000,
>              .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
> -            .readfn = pmreg_read, .writefn = pmcr_write,
> -            .raw_readfn = raw_read, .raw_writefn = raw_write,
> +            .accessfn = pmreg_access, .writefn = pmcr_write,
> +            .raw_writefn = raw_write,
>          };
>          ARMCPRegInfo clidr = {
>              .name = "CLIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 
> = 1,
> --
> 1.8.5
>
>