Re: [PATCH v2 03/10] target/arm: Don't mishandle count when enabling or disabling PMU counters

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> The PMU cycle and event counter infrastructure design requires that
> operations on the PMU register fields are wrapped in pmu_op_start()
> and pmu_op_finish() calls (or their more specific pmmcntr and
> pmevcntr equivalents).  This includes any changes to registers which
> affect whether the counter should be enabled or disabled, but we
> forgot to do this.
>
> The effect of this bug is that in sequences like:
>  * disable the cycle counter (PMCCNTR) using the PMCNTEN register
>  * write a value such as 0xfffff000 to the PMCCNTR
>  * restart the counter by writing to PMCNTEN
> the value written to the cycle counter is corrupted, and it starts
> counting from the wrong place. (Essentially, we fail to record that
> the QEMU_CLOCK_VIRTUAL timestamp when the counter should be considered
> to have started counting is the point when PMCNTEN is written to enable
> the counter.)
>
> Add the necessary bracketing calls, so that updates to the various
> registers which affect whether the PMU is counting are handled
> correctly.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I'm not sure why but this commit seems to be breaking a bunch of avocado
tests for me, including the TCG plugin ones:

  ➜  ./tests/venv/bin/avocado run tests/avocado/tcg_plugins.py:test_aarch64_virt_insn_icount
  JOB ID     : 0f5647d95f678e73fc01730cf9f8d7f80118443e
  JOB LOG    : /home/alex/avocado/job-results/job-2022-10-02T20.19-0f5647d/job.log
   (1/1) tests/avocado/tcg_plugins.py:PluginKernelNormal.test_aarch64_virt_insn_icount: INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOrigi
  nal status: ERROR\n{'name': '1-tests/avocado/tcg_plugins.py:PluginKernelNormal.test_aarch64_virt_insn_icount', 'logdir': '/home/alex/avocado/job-results/job-2022-10-02T20.19
  -0f5647d/te... (120.43 s)
  RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 1 | CANCEL 0
  JOB TIME   : 120.72 s

> ---
> v1->v2: fixed comment typo
> ---
>  target/arm/helper.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 87c89748954..59e1280a9cd 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1079,6 +1079,14 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
>      return pmreg_access(env, ri, isread);
>  }
>  
> +/*
> + * Bits in MDCR_EL2 and MDCR_EL3 which pmu_counter_enabled() looks at.
> + * We use these to decide whether we need to wrap a write to MDCR_EL2
> + * or MDCR_EL3 in pmu_op_start()/pmu_op_finish() calls.
> + */
> +#define MDCR_EL2_PMU_ENABLE_BITS (MDCR_HPME | MDCR_HPMD | MDCR_HPMN)
> +#define MDCR_EL3_PMU_ENABLE_BITS (MDCR_SPME)
> +
>  /* Returns true if the counter (pass 31 for PMCCNTR) should count events using
>   * the current EL, security state, and register configuration.
>   */
> @@ -1432,15 +1440,19 @@ static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
>  static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> +    pmu_op_start(env);
>      value &= pmu_counter_mask(env);
>      env->cp15.c9_pmcnten |= value;
> +    pmu_op_finish(env);
>  }
>  
>  static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                               uint64_t value)
>  {
> +    pmu_op_start(env);
>      value &= pmu_counter_mask(env);
>      env->cp15.c9_pmcnten &= ~value;
> +    pmu_op_finish(env);
>  }
>  
>  static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -4681,7 +4693,39 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t value)
>  {
> +    /*
> +     * Some MDCR_EL3 bits affect whether PMU counters are running:
> +     * if we are trying to change any of those then we must
> +     * bracket this update with PMU start/finish calls.
> +     */
> +    bool pmu_op = (env->cp15.mdcr_el3 ^ value) & MDCR_EL3_PMU_ENABLE_BITS;
> +
> +    if (pmu_op) {
> +        pmu_op_start(env);
> +    }
>      env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
> +    if (pmu_op) {
> +        pmu_op_finish(env);
> +    }
> +}
> +
> +static void mdcr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                           uint64_t value)
> +{
> +    /*
> +     * Some MDCR_EL2 bits affect whether PMU counters are running:
> +     * if we are trying to change any of those then we must
> +     * bracket this update with PMU start/finish calls.
> +     */
> +    bool pmu_op = (env->cp15.mdcr_el2 ^ value) & MDCR_EL2_PMU_ENABLE_BITS;
> +
> +    if (pmu_op) {
> +        pmu_op_start(env);
> +    }
> +    env->cp15.mdcr_el2 = value;
> +    if (pmu_op) {
> +        pmu_op_finish(env);
> +    }
>  }
>  
>  static const ARMCPRegInfo v8_cp_reginfo[] = {
> @@ -7669,6 +7713,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          ARMCPRegInfo mdcr_el2 = {
>              .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>              .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
> +            .writefn = mdcr_el2_write,
>              .access = PL2_RW, .resetvalue = pmu_num_counters(env),
>              .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2),
>          };


-- 
Alex Bennée