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

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>
---
v1->v2: fixed comment typo
---
 target/arm/helper.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

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),
>          };

On Mon, 3 Oct 2022 at 09:55, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> 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

Known issue, fixed by
https://patchew.org/QEMU/20220930133511.2112734-1-peter.maydell@linaro.org/20220930133511.2112734-2-peter.maydell@linaro.org/
in a pending pullreq.

(I have no idea why avocado reports this as a timeout, because
what actually happens is the QEMU binary prints an error
message and exits with non-zero exit status.)

-- PMM