[PATCH v2] target/arm: Correctly initialize MDCR_EL2.HPMN

[PATCH v2] target/arm: Correctly initialize MDCR_EL2.HPMN

[Daniel Müller via]

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.
---
 target/arm/helper.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 38cd35c049..4a5d512b51 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -37,6 +37,7 @@
 #endif
 
 #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
+#define PMCR_NUM_COUNTERS 4 /* QEMU IMPDEF choice */
 
 #ifndef CONFIG_USER_ONLY
 
@@ -5662,13 +5663,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 = PMCR_NUM_COUNTERS,
       .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), },
     { .name = "HPFAR", .state = ARM_CP_STATE_AA32,
       .cp = 15, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
@@ -6566,7 +6565,7 @@ static void define_pmu_regs(ARMCPU *cpu)
      * field as main ID register, and we implement four counters in
      * addition to the cycle count register.
      */
-    unsigned int i, pmcrn = 4;
+    unsigned int i, pmcrn = PMCR_NUM_COUNTERS;
     ARMCPRegInfo pmcr = {
         .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
         .access = PL0_RW,
-- 
2.27.0

Re: [PATCH v2] target/arm: Correctly initialize MDCR_EL2.HPMN

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210210174122.410690-1-muellerd@fb.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210210174122.410690-1-muellerd@fb.com
Subject: [PATCH v2] target/arm: Correctly initialize MDCR_EL2.HPMN

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
016a49e target/arm: Correctly initialize MDCR_EL2.HPMN

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 30 lines checked

Commit 016a49ea2b79 (target/arm: Correctly initialize MDCR_EL2.HPMN) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210210174122.410690-1-muellerd@fb.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2] target/arm: Correctly initialize MDCR_EL2.HPMN

[Peter Maydell]

On Wed, 10 Feb 2021 at 18:05, Daniel Müller via <qemu-devel@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.
> ---
>  target/arm/helper.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>



Applied to target-arm.next, thanks.

-- PMM