[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 39/49] target/arm: Filter cycle counter based on PMCCFILTR_EL0
From: |
Aaron Lindsay |
Subject: |
Re: [PULL 39/49] target/arm: Filter cycle counter based on PMCCFILTR_EL0 |
Date: |
Tue, 25 Aug 2020 10:41:06 -0400 |
On Aug 24 17:33, Peter Maydell wrote:
> On Fri, 18 Jan 2019 at 14:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > From: Aaron Lindsay <aaron@os.amperecomputing.com>
> >
> > Rename arm_ccnt_enabled to pmu_counter_enabled, and add logic to only
> > return 'true' if the specified counter is enabled and neither prohibited
> > or filtered.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Message-id: 20181211151945.29137-5-aaron@os.amperecomputing.com
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Hi Aaron; I've just had somebody report what seems like a bug
> in this code from last year:
>
> > +/* Returns true if the counter (pass 31 for PMCCNTR) should count events
> > using
> > + * the current EL, security state, and register configuration.
> > + */
> > +static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
> > {
> > - /* This does not support checking PMCCFILTR_EL0 register */
> > + uint64_t filter;
> > + bool e, p, u, nsk, nsu, nsh, m;
> > + bool enabled, prohibited, filtered;
> > + bool secure = arm_is_secure(env);
> > + int el = arm_current_el(env);
> > + uint8_t hpmn = env->cp15.mdcr_el2 & MDCR_HPMN;
> >
> > - if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 <<
> > 31))) {
> > - return false;
> > + if (!arm_feature(env, ARM_FEATURE_EL2) ||
> > + (counter < hpmn || counter == 31)) {
> > + e = env->cp15.c9_pmcr & PMCRE;
> > + } else {
> > + e = env->cp15.mdcr_el2 & MDCR_HPME;
> > + }
> > + enabled = e && (env->cp15.c9_pmcnten & (1 << counter));
> > +
> > + if (!secure) {
> > + if (el == 2 && (counter < hpmn || counter == 31)) {
> > + prohibited = env->cp15.mdcr_el2 & MDCR_HPMD;
> > + } else {
> > + prohibited = false;
> > + }
> > + } else {
> > + prohibited = arm_feature(env, ARM_FEATURE_EL3) &&
> > + (env->cp15.mdcr_el3 & MDCR_SPME);
>
> The Arm ARM says that MDCR.SPME is 0 to prohibit secure-state
> event counting, and 1 to enable it. So shouldn't this test
> be "!(env->cp15.mdcr_el3 & MDCR_SPME)" ?
>
> (compare also the AArch32.CountEvents pseudocode, which has
> a test "HaveEL3(EL3) && ISSecure() && spme == '0' &&...")
I agree my original patch was incorrect. My guess is that I overlooked
the trailing D changing to an E and got caught assuming MDCR_HPMD and
MDCR_SPME both prohibited counting when set. Sending a fix separately.
-Aaron