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