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

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

Posted by Alex Richardson 3 months, 3 weeks ago

In the PMUv3, a new AArch32 64-bit (MCRR/MRRC) accessor for the
PMCCNTR was added. In QEMU we forgot to implement this, so only
provide the 32-bit accessor. Since we have a 64-bit PMCCNTR
sysreg for AArch64, adding the 64-bit AArch32 version is easy.

We add the PMCCNTR to the v8_cp_reginfo because PMUv3 was added
in the ARMv8 architecture. This is consistent with how we
handle the existing PMCCNTR support, where we always implement
it for all v7 CPUs. This is arguably something we should
clean up so it is gated on ARM_FEATURE_PMU and/or an ID
register check for the relevant PMU version, but we should
do that as its own tidyup rather than being inconsistent between
this PMCCNTR accessor and the others.

Since the register name is the same as the 32-bit PMCCNTR, we set
ARM_CP_NO_GDB to avoid generating an invalid GDB XML.

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/cpregs-pmu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/arm/cpregs-pmu.c b/target/arm/cpregs-pmu.c
index 0f295b1376..ef176e4045 100644
--- a/target/arm/cpregs-pmu.c
+++ b/target/arm/cpregs-pmu.c
@@ -1276,6 +1276,12 @@ void define_pm_cpregs(ARMCPU *cpu)
               .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST,
               .fgt = FGT_PMCEIDN_EL0,
               .resetvalue = cpu->pmceid1 },
+            { .name = "PMCCNTR", .state = ARM_CP_STATE_AA32,
+              .cp = 15, .crm = 9, .opc1 = 0,
+              .access = PL0_RW, .accessfn = pmreg_access_ccntr, .resetvalue = 0,
+              .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_64BIT | ARM_CP_NO_GDB,
+              .fgt = FGT_PMCCNTR_EL0, .readfn = pmccntr_read,
+              .writefn = pmccntr_write,  },
         };
         define_arm_cp_regs(cpu, v8_pm_reginfo);
     }
-- 
2.50.1.470.g6ba607880d-goog

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

Posted by Peter Maydell 3 months, 3 weeks ago

On Fri, 25 Jul 2025 at 01:10, Alex Richardson <alexrichardson@google.com> wrote:
>
> In the PMUv3, a new AArch32 64-bit (MCRR/MRRC) accessor for the
> PMCCNTR was added. In QEMU we forgot to implement this, so only
> provide the 32-bit accessor. Since we have a 64-bit PMCCNTR
> sysreg for AArch64, adding the 64-bit AArch32 version is easy.
>
> We add the PMCCNTR to the v8_cp_reginfo because PMUv3 was added
> in the ARMv8 architecture. This is consistent with how we
> handle the existing PMCCNTR support, where we always implement
> it for all v7 CPUs. This is arguably something we should
> clean up so it is gated on ARM_FEATURE_PMU and/or an ID
> register check for the relevant PMU version, but we should
> do that as its own tidyup rather than being inconsistent between
> this PMCCNTR accessor and the others.
>
> Since the register name is the same as the 32-bit PMCCNTR, we set
> ARM_CP_NO_GDB to avoid generating an invalid GDB XML.

This is the wrong way around, I think. We should prefer to expose
to GDB the 64-bit view of it, not the 32-bit view, because
it's more comprehensive. Compare handling of the "PAR" definition
in target/arm/helper.c.

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

It would have been helpful to mention here what the changes
from v1 were -- I had to go and look up the list archives to
remind myself of why we had to drop v1.

>  target/arm/cpregs-pmu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/arm/cpregs-pmu.c b/target/arm/cpregs-pmu.c
> index 0f295b1376..ef176e4045 100644
> --- a/target/arm/cpregs-pmu.c
> +++ b/target/arm/cpregs-pmu.c
> @@ -1276,6 +1276,12 @@ void define_pm_cpregs(ARMCPU *cpu)
>                .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST,
>                .fgt = FGT_PMCEIDN_EL0,
>                .resetvalue = cpu->pmceid1 },
> +            { .name = "PMCCNTR", .state = ARM_CP_STATE_AA32,
> +              .cp = 15, .crm = 9, .opc1 = 0,
> +              .access = PL0_RW, .accessfn = pmreg_access_ccntr, .resetvalue = 0,
> +              .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_64BIT | ARM_CP_NO_GDB,
> +              .fgt = FGT_PMCCNTR_EL0, .readfn = pmccntr_read,
> +              .writefn = pmccntr_write,  },
>          };
>          define_arm_cp_regs(cpu, v8_pm_reginfo);
>      }
> --
> 2.50.1.470.g6ba607880d-goog

thanks
-- PMM

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

Posted by Alex Richardson 3 months, 3 weeks ago

On Fri, 25 Jul 2025 at 02:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 25 Jul 2025 at 01:10, Alex Richardson <alexrichardson@google.com> wrote:
> >
> > In the PMUv3, a new AArch32 64-bit (MCRR/MRRC) accessor for the
> > PMCCNTR was added. In QEMU we forgot to implement this, so only
> > provide the 32-bit accessor. Since we have a 64-bit PMCCNTR
> > sysreg for AArch64, adding the 64-bit AArch32 version is easy.
> >
> > We add the PMCCNTR to the v8_cp_reginfo because PMUv3 was added
> > in the ARMv8 architecture. This is consistent with how we
> > handle the existing PMCCNTR support, where we always implement
> > it for all v7 CPUs. This is arguably something we should
> > clean up so it is gated on ARM_FEATURE_PMU and/or an ID
> > register check for the relevant PMU version, but we should
> > do that as its own tidyup rather than being inconsistent between
> > this PMCCNTR accessor and the others.
> >
> > Since the register name is the same as the 32-bit PMCCNTR, we set
> > ARM_CP_NO_GDB to avoid generating an invalid GDB XML.
>
> This is the wrong way around, I think. We should prefer to expose
> to GDB the 64-bit view of it, not the 32-bit view, because
> it's more comprehensive. Compare handling of the "PAR" definition
> in target/arm/helper.c.
>
Thanks for the reference to the existing code, will send out v3 shortly.

> >
> > 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>
> > ---
>
> It would have been helpful to mention here what the changes
> from v1 were -- I had to go and look up the list archives to
> remind myself of why we had to drop v1.

Apologies, I usually contribute via code review systems that track
changes and am new to contributing to email-based workflows.

>
> >  target/arm/cpregs-pmu.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/target/arm/cpregs-pmu.c b/target/arm/cpregs-pmu.c
> > index 0f295b1376..ef176e4045 100644
> > --- a/target/arm/cpregs-pmu.c
> > +++ b/target/arm/cpregs-pmu.c
> > @@ -1276,6 +1276,12 @@ void define_pm_cpregs(ARMCPU *cpu)
> >                .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST,
> >                .fgt = FGT_PMCEIDN_EL0,
> >                .resetvalue = cpu->pmceid1 },
> > +            { .name = "PMCCNTR", .state = ARM_CP_STATE_AA32,
> > +              .cp = 15, .crm = 9, .opc1 = 0,
> > +              .access = PL0_RW, .accessfn = pmreg_access_ccntr, .resetvalue = 0,
> > +              .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_64BIT | ARM_CP_NO_GDB,
> > +              .fgt = FGT_PMCCNTR_EL0, .readfn = pmccntr_read,
> > +              .writefn = pmccntr_write,  },
> >          };
> >          define_arm_cp_regs(cpu, v8_pm_reginfo);
> >      }
> > --
> > 2.50.1.470.g6ba607880d-goog
>
> thanks
> -- PMM