| Message ID | 20210125210817.2564212-1-muellerd@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Correctly initialize MDCR_EL2.HPMN | expand |
On Mon, 25 Jan 2021 at 21:48, muellerd--- via <qemu-arm@nongnu.org> wrote: > > When working with performance monitoring counters, we look at > MDCR_EL2.HPMN as part of the check whether a counter is enabled. This > check fails, because MDCR_EL2.HPMN is reset to 0, meaning that no > counters are "enabled" for < EL2. > That's in violation of the Arm specification, which states that > > > On a Warm reset, this field [MDCR_EL2.HPMN] resets to the value in > > PMCR_EL0.N > > That's also what a comment in the code acknowledges, but the necessary > adjustment seems to have been forgotten when support for more counters > was added. > This change fixes the issue by setting the reset value to PMCR.N, which > is four. > > Signed-off-by: Daniel Müller <muellerd@fb.com> > --- > target/arm/helper.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index d2ead3fcbd..195db4d378 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5705,13 +5705,11 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { > .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write }, > #endif > /* The only field of MDCR_EL2 that has a defined architectural reset value > - * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we > - * don't implement any PMU event counters, so using zero as a reset > - * value for MDCR_EL2 is okay > + * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N. > */ > { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1, > - .access = PL2_RW, .resetvalue = 0, > + .access = PL2_RW, .resetvalue = 4, > .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), }, > { .name = "HPFAR", .state = ARM_CP_STATE_AA32, > .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4, Rather than having a hardcoded 4 here, could you add a #define PMCR_NUM_COUNTERS 4 /* QEMU IMPDEF choice */ and then use the constant name both in the resetvalue here and also where we currently have 'pmcrn = 4' in define_pmu_regs()? thanks -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index d2ead3fcbd..195db4d378 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5705,13 +5705,11 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write }, #endif /* The only field of MDCR_EL2 that has a defined architectural reset value - * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we - * don't implement any PMU event counters, so using zero as a reset - * value for MDCR_EL2 is okay + * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N. */ { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1, - .access = PL2_RW, .resetvalue = 0, + .access = PL2_RW, .resetvalue = 4, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), }, { .name = "HPFAR", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,