diff mbox series

[PULL,09/11] target/arm: add support for PMUv3 64-bit PMCCNTR in AArch32 mode

Message ID 20240809180835.1243269-10-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show
Series [PULL,01/11] target/arm: Fix BTI versus CF_PCREL | expand

Commit Message

Peter Maydell Aug. 9, 2024, 6:08 p.m. UTC 
From: Alex Richardson <alexrichardson@google.com>

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.

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>
Message-id: 20240801220328.941866-1-alexrichardson@google.com
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Richard Henderson Aug. 11, 2024, 2:58 a.m. UTC | #1 
On 8/10/24 04:08, Peter Maydell wrote:
> From: Alex Richardson <alexrichardson@google.com>
> 
> 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.
> 
> 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>
> Message-id: 20240801220328.941866-1-alexrichardson@google.com
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 8fb4b474e83..94900667c33 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 */

This fails testing:

https://gitlab.com/qemu-project/qemu/-/jobs/7551982466

FAIL: duplicate register {'name': 'PMCCNTR', 'regnum': 96} vs {'name': 'PMCCNTR', 
'regnum': 79}
FAIL: counted all 219 registers in XML
FAIL: PMCCNTR 96 == 79 (xml)


r~
Peter Maydell Aug. 12, 2024, 9:39 a.m. UTC | #2 
On Sun, 11 Aug 2024 at 22:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/10/24 04:08, Peter Maydell wrote:
> > From: Alex Richardson <alexrichardson@google.com>
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 8fb4b474e83..94900667c33 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 */
>
> This fails testing:
>
> https://gitlab.com/qemu-project/qemu/-/jobs/7551982466
>
> FAIL: duplicate register {'name': 'PMCCNTR', 'regnum': 96} vs {'name': 'PMCCNTR',
> 'regnum': 79}
> FAIL: counted all 219 registers in XML
> FAIL: PMCCNTR 96 == 79 (xml)

Hmm, not sure why that didn't get caught by my local testing
or by my gitlab run -- does it only get run on an aarch64 host?

Anyway, the registers do architecturally have the same name
(they're the same register, just accessible via different
pathways). What is our practice for this? Do we just give
one of them a non-standard name?

-- PMM
Peter Maydell Aug. 12, 2024, 10:30 a.m. UTC | #3 
On Mon, 12 Aug 2024 at 10:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 11 Aug 2024 at 22:36, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 8/10/24 04:08, Peter Maydell wrote:
> > > From: Alex Richardson <alexrichardson@google.com>
> > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > index 8fb4b474e83..94900667c33 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 */
> >
> > This fails testing:
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/7551982466
> >
> > FAIL: duplicate register {'name': 'PMCCNTR', 'regnum': 96} vs {'name': 'PMCCNTR',
> > 'regnum': 79}
> > FAIL: counted all 219 registers in XML
> > FAIL: PMCCNTR 96 == 79 (xml)
>
> Hmm, not sure why that didn't get caught by my local testing
> or by my gitlab run -- does it only get run on an aarch64 host?
>
> Anyway, the registers do architecturally have the same name
> (they're the same register, just accessible via different
> pathways). What is our practice for this? Do we just give
> one of them a non-standard name?

Looking at how we handle "PAR", we have some clunky code
to add the ARM_CP_NO_GDB flag to the 32-bit version.
I guess that produces the best results but it's going
to be kind of awkward...

-- PMM
Alex Bennée Aug. 12, 2024, 11:10 a.m. UTC | #4 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Sun, 11 Aug 2024 at 22:36, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 8/10/24 04:08, Peter Maydell wrote:
>> > From: Alex Richardson <alexrichardson@google.com>
>> > diff --git a/target/arm/helper.c b/target/arm/helper.c
>> > index 8fb4b474e83..94900667c33 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 */
>>
>> This fails testing:
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/7551982466
>>
>> FAIL: duplicate register {'name': 'PMCCNTR', 'regnum': 96} vs {'name': 'PMCCNTR',
>> 'regnum': 79}
>> FAIL: counted all 219 registers in XML
>> FAIL: PMCCNTR 96 == 79 (xml)
>
> Hmm, not sure why that didn't get caught by my local testing
> or by my gitlab run -- does it only get run on an aarch64 host?

It will depend what your local GDB is like - a modern gdb-multiarch
should be fine but we do test for a minimum version to be able to probe
the supported architectures.

> Anyway, the registers do architecturally have the same name
> (they're the same register, just accessible via different
> pathways). What is our practice for this? Do we just give
> one of them a non-standard name?
>
> -- PMM
Peter Maydell Aug. 12, 2024, 11:40 a.m. UTC | #5 
On Mon, 12 Aug 2024 at 12:10, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Sun, 11 Aug 2024 at 22:36, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 8/10/24 04:08, Peter Maydell wrote:
> >> > From: Alex Richardson <alexrichardson@google.com>
> >> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> >> > index 8fb4b474e83..94900667c33 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 */
> >>
> >> This fails testing:
> >>
> >> https://gitlab.com/qemu-project/qemu/-/jobs/7551982466
> >>
> >> FAIL: duplicate register {'name': 'PMCCNTR', 'regnum': 96} vs {'name': 'PMCCNTR',
> >> 'regnum': 79}
> >> FAIL: counted all 219 registers in XML
> >> FAIL: PMCCNTR 96 == 79 (xml)
> >
> > Hmm, not sure why that didn't get caught by my local testing
> > or by my gitlab run -- does it only get run on an aarch64 host?
>
> It will depend what your local GDB is like - a modern gdb-multiarch
> should be fine but we do test for a minimum version to be able to probe
> the supported architectures.

Mmm, I found that a local "make check-tcg" does catch this for me,
so I guess the answer is "the gdb on the non aarch64 host CI jobs
is too old and/or we missed the coverage, and I forgot to run
this in my local checkout".

Why doesn't "make check" run "check-tcg" as a sub-test ?
Having it be separate is asking for people to forget to
run it, I think.

-- PMM
Alex Bennée Aug. 13, 2024, 1:10 p.m. UTC | #6 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 12 Aug 2024 at 12:10, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
<snip>
>> >>
>> >> This fails testing:
>> >>
>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7551982466
>> >>
>> >> FAIL: duplicate register {'name': 'PMCCNTR', 'regnum': 96} vs {'name': 'PMCCNTR',
>> >> 'regnum': 79}
>> >> FAIL: counted all 219 registers in XML
>> >> FAIL: PMCCNTR 96 == 79 (xml)
>> >
>> > Hmm, not sure why that didn't get caught by my local testing
>> > or by my gitlab run -- does it only get run on an aarch64 host?
>>
>> It will depend what your local GDB is like - a modern gdb-multiarch
>> should be fine but we do test for a minimum version to be able to probe
>> the supported architectures.
>
> Mmm, I found that a local "make check-tcg" does catch this for me,
> so I guess the answer is "the gdb on the non aarch64 host CI jobs
> is too old and/or we missed the coverage, and I forgot to run
> this in my local checkout".
>
> Why doesn't "make check" run "check-tcg" as a sub-test ?
> Having it be separate is asking for people to forget to
> run it, I think.

I think historically because not everyone cared about TCG testing and
you need to either setup docker or have cross compilers on your system.
Obviously we know this is the case when we run check-tcg in the CI.

We could certainly include it in the main "check" set if you want.

>
> -- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8fb4b474e83..94900667c33 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 */