From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e4U14-0008MG-O6 for qemu-devel@nongnu.org; Tue, 17 Oct 2017 11:42:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e4U13-0000Bu-Lo for qemu-devel@nongnu.org; Tue, 17 Oct 2017 11:42:50 -0400 Date: Tue, 17 Oct 2017 11:42:41 -0400 From: Aaron Lindsay Message-ID: <20171017154240.GF3676@codeaurora.org> References: <1506737310-21880-1-git-send-email-alindsay@codeaurora.org> <1506737310-21880-5-git-send-email-alindsay@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-arm , Alistair Francis , Peter Crosthwaite , Wei Huang , QEMU Developers , Michael Spradling , Digant Desai On Oct 17 14:41, Peter Maydell wrote: > On 30 September 2017 at 03:08, Aaron Lindsay wrote: > > This is in preparation for enabling counters other than PMCCNTR > > > > Signed-off-by: Aaron Lindsay > > --- > > 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.