On Aug 24 17:33, Peter Maydell wrote: > On Fri, 18 Jan 2019 at 14:58, Peter Maydell <[email protected]> wrote: > > > > From: Aaron Lindsay <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Aaron Lindsay <[email protected]> > > Reviewed-by: Peter Maydell <[email protected]> > > Reviewed-by: Richard Henderson <[email protected]> > > Message-id: [email protected] > > Signed-off-by: Peter Maydell <[email protected]> > > 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
