[PATCH] target/arm: Correctly initialize MDCR_EL2.HPMN

[PATCH] target/arm: Correctly initialize MDCR_EL2.HPMN

[muellerd--- via]

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,
-- 
2.24.1

Re: [PATCH] target/arm: Correctly initialize MDCR_EL2.HPMN

[Peter Maydell]

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