Re: [Qemu-arm] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Peter Maydell]

On 30 September 2017 at 03:08, Aaron Lindsay <address@hidden> wrote:
> This is in preparation for enabling counters other than PMCCNTR
>
> Signed-off-by: Aaron Lindsay <address@hidden>
> ---
>  target/arm/helper.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ecf8c55..54070a3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -30,11 +30,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int 
> *prot,
>                                 target_ulong *page_size_ptr, uint32_t *fsr,
>                                 ARMMMUFaultInfo *fi);
> -
> -/* Definitions for the PMCCNTR and PMCR registers */
> -#define PMCRD   0x8
> -#define PMCRC   0x4
> -#define PMCRE   0x1
>  #endif
>
>  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> @@ -876,6 +871,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> +/* Definitions for the PMU registers */
> +#define PMCRN   0xf800

We usually name this kind of whole-field mask value something like PMCRN_MASK.

(We also have a FIELD() macro in hw/registerfields.h for defining
mask/length/shift constants; though it can be a bit verbose in
the length of the names it uses, so I don't insist on its use
if you don't like the results. I'm on the fence myself about it.)

> +#define PMCRN_SHIFT 11
> +#define PMCRD   0x8
> +#define PMCRC   0x4
> +#define PMCRE   0x1
> +
> +#define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT)
> +/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
> +#define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 
> 1))
> +
>  static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                     bool isread)
>  {
> @@ -1060,14 +1066,14 @@ static void pmccfiltr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> -    value &= (1 << 31);
> +    value &= (PMU_COUNTER_MASK(env) | (1 << 31));

PMU_COUNTER_MASK always contains bit 31, so why do we manually OR it in
again here?

>      env->cp15.c9_pmcnten |= value;
>  }
>
>  static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                               uint64_t value)
>  {
> -    value &= (1 << 31);
> +    value &= (PMU_COUNTER_MASK(env) | (1 << 31));
>      env->cp15.c9_pmcnten &= ~value;
>  }
>
> @@ -1115,14 +1121,14 @@ static void pmintenset_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>                               uint64_t value)
>  {
>      /* We have no event counters so only the C bit can be changed */
> -    value &= (1 << 31);
> +    value &= PMU_COUNTER_MASK(env);
>      env->cp15.c9_pminten |= value;
>  }
>
>  static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                               uint64_t value)
>  {
> -    value &= (1 << 31);
> +    value &= PMU_COUNTER_MASK(env);
>      env->cp15.c9_pminten &= ~value;
>  }
>
> --
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
> Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a 
> Linux Foundation Collaborative Project.

Shouldn't these '\n' in your sig be literal line breaks? :-)

thanks
-- PMM