Re: [PULL 39/49] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay <aaron@os.amperecomputing.com>]

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