Re: [PATCH] target/arm: add support for 64-bit PMCCNTR in AArch32 mode

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 2 Aug 2024 at 02:00, Alex Richardson <alexrichardson@google.com> wrote:
>
> See https://developer.arm.com/documentation/ddi0601/2024-06/AArch32-Registers/PMCCNTR--Performance-Monitors-Cycle-Count-Register?lang=en
>
> Signed-off-by: Alex Richardson <alexrichardson@google.com>
> ---
>  target/arm/helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8fb4b474e8..94900667c3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5952,6 +5952,12 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>        .writefn = sdcr_write,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
> +    { .name = "PMCCNTR", .state = ARM_CP_STATE_AA32,
> +      .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_64BIT,
> +      .cp = 15, .crm = 9, .opc1 = 0,
> +      .access = PL0_RW, .resetvalue = 0, .fgt = FGT_PMCCNTR_EL0,
> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
> +      .accessfn = pmreg_access_ccntr },
>  };
>
>  /* These are present only when EL1 supports AArch32 */
> --
> 2.46.0.rc2.264.g509ed76dc8-goog

Coincidentally I'd also noticed recently that we don't implement
the 64-bit accessor for PMCCNTR, but I hadn't got round to
writing a patch, so thanks for doing this.

The 64-bit AArch32 PMCCNTR was added in v8 with the PMUv3, and
presumably most guests which use the PMU in AArch32 code want
to retain the ability to work with PMUv1 or v2 (or were simply
written for PMUv1/v2 and never updated), so they stick to the
32-bit sysreg, which is why we haven't noticed this bug before.

There is an argument that we should gate this on
ARM_FEATURE_PMU being set (or on an ID register test
for the PMU version field being at least 3), but we don't
currently do that for the existing PMU registers, which we
just add regardless for v7 CPUs. So I think we should
consider that a separate cleanup, and this is OK.

I've applied this to target-arm.next (with an expansion
of the commit message to note some of the above).

thanks
-- PMM