[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/13] target/arm: Mask PMU register writes base
From: |
Aaron Lindsay |
Subject: |
Re: [Qemu-devel] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N |
Date: |
Tue, 17 Oct 2017 11:42:41 -0400 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Oct 17 14:41, Peter Maydell wrote:
> 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.)
Will do.
> > +#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?
I forgot I'd also included the cycle counter in PMU_COUNTER_MASK... I'll
remove the duplication here and in pmcntenclr_write below.
> > 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? :-)
Yes :( ...apparently `git config` doesn't interpret them as I expected.
> thanks
> -- PMM
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.