On Fri, 18 Jan 2019 at 14:58, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> From: Aaron Lindsay <aa...@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 <alind...@codeaurora.org>
> Signed-off-by: Aaron Lindsay <aclin...@gmail.com>
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> Message-id: 20181211151945.29137-5-aa...@os.amperecomputing.com
> Signed-off-by: Peter Maydell <peter.mayd...@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
