target/arm/cpregs-pmu.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
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 on the 32-bit one 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
Change v2->v3:
- Moved ARM_CP_NO_GDB to the 32-bit register if Armv8 is supported
Changes v1->v2:
- Moved to new file
- Updated commit message
- Added ARM_CP_NO_GDB
Signed-off-by: Alex Richardson <alexrichardson@google.com>
---
target/arm/cpregs-pmu.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/target/arm/cpregs-pmu.c b/target/arm/cpregs-pmu.c
index 0f295b1376..144e339c76 100644
--- a/target/arm/cpregs-pmu.c
+++ b/target/arm/cpregs-pmu.c
@@ -1067,11 +1067,6 @@ static const ARMCPRegInfo v7_pm_reginfo[] = {
.fgt = FGT_PMSELR_EL0,
.fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
.writefn = pmselr_write, .raw_writefn = raw_write, },
- { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
- .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
- .fgt = FGT_PMCCNTR_EL0,
- .readfn = pmccntr_read, .writefn = pmccntr_write32,
- .accessfn = pmreg_access_ccntr },
{ .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 0,
.access = PL0_RW, .accessfn = pmreg_access_ccntr,
@@ -1211,6 +1206,19 @@ void define_pm_cpregs(ARMCPU *cpu)
define_one_arm_cp_reg(cpu, &pmcr);
define_one_arm_cp_reg(cpu, &pmcr64);
define_arm_cp_regs(cpu, v7_pm_reginfo);
+ /* When Armv8 is supported, PMCCNTR aliases the new 64-bit version */
+ ARMCPRegInfo pmccntr = {
+ .name = "PMCCNTR",
+ .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
+ .access = PL0_RW, .accessfn = pmreg_access_ccntr,
+ .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
+ .fgt = FGT_PMCCNTR_EL0,
+ .readfn = pmccntr_read, .writefn = pmccntr_write32,
+ };
+ if (arm_feature(env, ARM_FEATURE_V8)) {
+ pmccntr.type |= ARM_CP_NO_GDB;
+ }
+ define_one_arm_cp_reg(cpu, &pmccntr);
for (unsigned i = 0, pmcrn = pmu_num_counters(env); i < pmcrn; i++) {
g_autofree char *pmevcntr_name = g_strdup_printf("PMEVCNTR%d", i);
@@ -1276,6 +1284,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,
+ .fgt = FGT_PMCCNTR_EL0, .readfn = pmccntr_read,
+ .writefn = pmccntr_write, },
};
define_arm_cp_regs(cpu, v8_pm_reginfo);
}
--
2.50.1.470.g6ba607880d-googOn 25.07.2025 20:01, Alex Richardson 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 on the 32-bit one 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
>
> Change v2->v3:
> - Moved ARM_CP_NO_GDB to the 32-bit register if Armv8 is supported
>
> Changes v1->v2:
> - Moved to new file
> - Updated commit message
> - Added ARM_CP_NO_GDB
>
> Signed-off-by: Alex Richardson <alexrichardson@google.com>
> ---
> target/arm/cpregs-pmu.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
Hi!
This patch is marked as to back-port to stable series, with
the commit message tweak by Peter:
Note for potential backporting:
* this code in cpregs-pmu.c will be in helper.c on stable
branches that don't have commit ae2086426d37
The mentioned commit:
commit ae2086426d3784cf66e5b0b7ac823c08e87b4c57
Author: Richard Henderson <richard.henderson@linaro.org>
Date: Mon Jul 7 09:15:47 2025 -0600
target/arm: Split out performance monitor regs to cpregs-pmu.c
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-id: 20250707151547.196393-4-richard.henderson@linaro.org
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
The thing is that this commit not only split out parts of helper.c
to cpregs-pmu.c, but it also modified the code quite significantly,
which is not even mentioned in the commit message.
This makes back-porting of the changes more difficult, which is okay, -
for this I'm asking for help (provided this fix is really needed for
10.0.x stable branch).
But this also brings a more general question, or a suggestion. When
splitting stuff into a separate file, *and* modifying it in process,
can't we do it in 2 stages, - first is to modify it, and next is to
move it without modifications? I think this makes review process
significantly easier. And I wonder how Philippe and Peter reviewed
this whole thing, as the changes in ae2086426d37 are rather large.
If it were me, I'd had a difficult time reviewing this change.
Thanks,
/mjt
On 8/5/25 17:44, Michael Tokarev wrote: > The mentioned commit: > > commit ae2086426d3784cf66e5b0b7ac823c08e87b4c57 > Author: Richard Henderson <richard.henderson@linaro.org> > Date: Mon Jul 7 09:15:47 2025 -0600 > > target/arm: Split out performance monitor regs to cpregs-pmu.c > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-id: 20250707151547.196393-4-richard.henderson@linaro.org > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > The thing is that this commit not only split out parts of helper.c > to cpregs-pmu.c, but it also modified the code quite significantly, > which is not even mentioned in the commit message. I most certainly did not modify the code significantly. The code was moved from several discontiguous blocks, but that's it. r~
On 05.08.2025 11:04, Richard Henderson wrote:
> On 8/5/25 17:44, Michael Tokarev wrote:
>> The mentioned commit:
>>
>> commit ae2086426d3784cf66e5b0b7ac823c08e87b4c57
>> Author: Richard Henderson <richard.henderson@linaro.org>
>> Date: Mon Jul 7 09:15:47 2025 -0600
>>
>> target/arm: Split out performance monitor regs to cpregs-pmu.c
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Message-id: 20250707151547.196393-4-richard.henderson@linaro.org
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> The thing is that this commit not only split out parts of helper.c
>> to cpregs-pmu.c, but it also modified the code quite significantly,
>> which is not even mentioned in the commit message.
>
> I most certainly did not modify the code significantly.
> The code was moved from several discontiguous blocks, but that's it.
Maybe "significantly" is an over-statement, but there are quite some
additional changes. Take a look at this commit, and see, for example,
v7_pm_reginfo definition, - it does not exist in the code deleted from
helper.c. Or the code near the usage of v7_pm_reginfo --
- define_one_arm_cp_reg(cpu, &pmcr);
- define_one_arm_cp_reg(cpu, &pmcr64);
- for (i = 0; i < pmcrn; i++) {
- char *pmevcntr_name = g_strdup_printf("PMEVCNTR%d", i);
..
+ define_one_arm_cp_reg(cpu, &pmcr);
+ define_one_arm_cp_reg(cpu, &pmcr64);
+ define_arm_cp_regs(cpu, v7_pm_reginfo);
+
+ for (unsigned i = 0, pmcrn = pmu_num_counters(env); i < pmcrn;
i++) {
+ g_autofree char *pmevcntr_name =
g_strdup_printf("PMEVCNTR%d", i);
...
To me, this is not just moving code around. Yes, I understand the
consolidation (taken a few pieces from various places and put them
into a single function). But the above change does more than that.
- for (i = 0; i < pmcrn; i++) {
+ for (unsigned i = 0, pmcrn = pmu_num_counters(env); i < pmcrn; i++)
heh. now I see how to back-port this PMCCNTR change to 10.0. Please see
https://gitlab.com/mjt0k/qemu/-/commit/8cd2995c1b0cda0a86ffa4e20a49372e5d30dc5d
for the current backport. I'm not sure about the order of "define_*_reg"
function calls here though - does it look sane?
Thanks,
/mjt
On Fri, 25 Jul 2025 at 18:02, 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 on the 32-bit one 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
>
> Change v2->v3:
> - Moved ARM_CP_NO_GDB to the 32-bit register if Armv8 is supported
>
> Changes v1->v2:
> - Moved to new file
> - Updated commit message
> - Added ARM_CP_NO_GDB
>
> Signed-off-by: Alex Richardson <alexrichardson@google.com>
Thanks for this patch; I added the following comment tweaks
and have applied it to target-arm.next for 10.1 (and marked
for potential backports to stable branches):
--- a/target/arm/cpregs-pmu.c
+++ b/target/arm/cpregs-pmu.c
@@ -1206,7 +1206,11 @@ void define_pm_cpregs(ARMCPU *cpu)
define_one_arm_cp_reg(cpu, &pmcr);
define_one_arm_cp_reg(cpu, &pmcr64);
define_arm_cp_regs(cpu, v7_pm_reginfo);
- /* When Armv8 is supported, PMCCNTR aliases the new 64-bit version */
+ /*
+ * 32-bit AArch32 PMCCNTR. We don't expose this to GDB if the
+ * new-in-v8 PMUv3 64-bit AArch32 PMCCNTR register is implemented
+ * (as that will provide the GDB user's view of "PMCCNTR").
+ */
ARMCPRegInfo pmccntr = {
.name = "PMCCNTR",
.cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
@@ -1284,6 +1288,7 @@ void define_pm_cpregs(ARMCPU *cpu)
.access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST,
.fgt = FGT_PMCEIDN_EL0,
.resetvalue = cpu->pmceid1 },
+ /* AArch32 64-bit PMCCNTR view: added in PMUv3 with Armv8 */
{ .name = "PMCCNTR", .state = ARM_CP_STATE_AA32,
.cp = 15, .crm = 9, .opc1 = 0,
.access = PL0_RW, .accessfn = pmreg_access_ccntr,
.resetvalue = 0,
thanks
-- PMM
© 2016 - 2025 Red Hat, Inc.