Peter Maydell <peter.mayd...@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.mayd...@linaro.org> > Reviewed-by: Richard Henderson <richard.hender...@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