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

[Peter Maydell]

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' &&...")

thanks
-- PMM