[PATCH 0/3] target/arm: Fix bugs in recent PMU changes

[PATCH 0/3] target/arm: Fix bugs in recent PMU changes

[Peter Maydell]

This patchset fixes some bugs in the recent changes I made
to the Arm PMU emualtion as part of implementing FEAT_PMUv3p5.

The important patch here is the first one, which fixes a crash
when in icount mode if the guest touched MDCR_EL3, MDCR_EL2,
PMCNTENSET_EL0 or PMCNTENCLR_EL0. The other two are more minor,
things which I noticed while I was looking at the code.

thanks
-- PMM

Peter Maydell (3):
  target/arm: Mark registers which call pmu_op_start() as ARM_CP_IO
  target/arm: Make writes to MDCR_EL3 use PMU start/finish calls
  target/arm: Update SDCR_VALID_MASK to include SCCD

 target/arm/cpu.h    |  8 +++++++-
 target/arm/helper.c | 30 ++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/3] target/arm: Mark registers which call pmu_op_start() as ARM_CP_IO

[Peter Maydell]

In commit 01765386a888 we made some system register write functions
call pmu_op_start()/pmu_op_finish(). This means that they now touch
timers, so for icount to work these registers must have the ARM_CP_IO
flag set.

This fixes a bug where when icount is enabled a guest that touches
MDCR_EL3, MDCR_EL2, PMCNTENSET_EL0 or PMCNTENCLR_EL0 would cause
QEMU to print an error message and exit, for example:

[    2.495971] TCP: Hash tables configured (established 1024 bind 1024)
[    2.496213] UDP hash table entries: 256 (order: 1, 8192 bytes)
[    2.496386] UDP-Lite hash table entries: 256 (order: 1, 8192 bytes)
[    2.496917] NET: Registered protocol family 1
qemu-system-aarch64: Bad icount read

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1a57d2e1d60..7c7ba328d6d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1927,12 +1927,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
      * or PL0_RO as appropriate and then check PMUSERENR in the helper fn.
      */
     { .name = "PMCNTENSET", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 1,
-      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .access = PL0_RW, .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmcnten),
       .writefn = pmcntenset_write,
       .accessfn = pmreg_access,
       .raw_writefn = raw_write },
-    { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64,
+    { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64, .type = ARM_CP_IO,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 1,
       .access = PL0_RW, .accessfn = pmreg_access,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .resetvalue = 0,
@@ -1942,11 +1942,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmcnten),
       .accessfn = pmreg_access,
       .writefn = pmcntenclr_write,
-      .type = ARM_CP_ALIAS },
+      .type = ARM_CP_ALIAS | ARM_CP_IO },
     { .name = "PMCNTENCLR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 2,
       .access = PL0_RW, .accessfn = pmreg_access,
-      .type = ARM_CP_ALIAS,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
       .writefn = pmcntenclr_write },
     { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
@@ -5130,7 +5130,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
       .resetvalue = 0,
       .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
-    { .name = "SDCR", .type = ARM_CP_ALIAS,
+    { .name = "SDCR", .type = ARM_CP_ALIAS | ARM_CP_IO,
       .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
       .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
       .writefn = sdcr_write,
@@ -7837,7 +7837,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
          * value is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N.
          */
         ARMCPRegInfo mdcr_el2 = {
-            .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
+            .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH, .type = ARM_CP_IO,
             .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
             .writefn = mdcr_el2_write,
             .access = PL2_RW, .resetvalue = pmu_num_counters(env),
-- 
2.25.1

Re: [PATCH 1/3] target/arm: Mark registers which call pmu_op_start() as ARM_CP_IO

[Richard Henderson]

On 9/23/22 05:34, Peter Maydell wrote:
> In commit 01765386a888 we made some system register write functions
> call pmu_op_start()/pmu_op_finish(). This means that they now touch
> timers, so for icount to work these registers must have the ARM_CP_IO
> flag set.
> 
> This fixes a bug where when icount is enabled a guest that touches
> MDCR_EL3, MDCR_EL2, PMCNTENSET_EL0 or PMCNTENCLR_EL0 would cause
> QEMU to print an error message and exit, for example:
> 
> [    2.495971] TCP: Hash tables configured (established 1024 bind 1024)
> [    2.496213] UDP hash table entries: 256 (order: 1, 8192 bytes)
> [    2.496386] UDP-Lite hash table entries: 256 (order: 1, 8192 bytes)
> [    2.496917] NET: Registered protocol family 1
> qemu-system-aarch64: Bad icount read
> 
> Reported-by: Thomas Huth<thuth@redhat.com>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/3] target/arm: Make writes to MDCR_EL3 use PMU start/finish calls

[Peter Maydell]

In commit 01765386a88868 we fixed a bug where we weren't correctly
bracketing changes to some registers with pmu_op_start() and
pmu_op_finish() calls for changes which affect whether the PMU
counters might be enabled.  However, we missed the case of writes to
the AArch64 MDCR_EL3 register, because (unlike its AArch32
counterpart) they are currently done directly to the CPU state struct
without going through the sdcr_write() function.

Give MDCR_EL3 a writefn which handles the PMU start/finish calls.
The SDCR writefn then simplfies to "call the MDCR_EL3 writefn after
masking off the bits which don't exist in the AArch32 register".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7c7ba328d6d..cebce23da07 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4761,8 +4761,8 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }
 
-static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
-                       uint64_t value)
+static void mdcr_el3_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
 {
     /*
      * Some MDCR_EL3 bits affect whether PMU counters are running:
@@ -4774,12 +4774,19 @@ static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     if (pmu_op) {
         pmu_op_start(env);
     }
-    env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
+    env->cp15.mdcr_el3 = value;
     if (pmu_op) {
         pmu_op_finish(env);
     }
 }
 
+static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                       uint64_t value)
+{
+    /* Not all bits defined for MDCR_EL3 exist in the AArch32 SDCR */
+    mdcr_el3_write(env, ri, value & SDCR_VALID_MASK);
+}
+
 static void mdcr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
                            uint64_t value)
 {
@@ -5127,9 +5134,12 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
       .access = PL2_RW,
       .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_FIQ]) },
     { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
+      .type = ARM_CP_IO,
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
       .resetvalue = 0,
-      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
+      .access = PL3_RW,
+      .writefn = mdcr_el3_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
     { .name = "SDCR", .type = ARM_CP_ALIAS | ARM_CP_IO,
       .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
       .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
-- 
2.25.1

Re: [PATCH 2/3] target/arm: Make writes to MDCR_EL3 use PMU start/finish calls

[Richard Henderson]

On 9/23/22 05:34, Peter Maydell wrote:
> In commit 01765386a88868 we fixed a bug where we weren't correctly
> bracketing changes to some registers with pmu_op_start() and
> pmu_op_finish() calls for changes which affect whether the PMU
> counters might be enabled.  However, we missed the case of writes to
> the AArch64 MDCR_EL3 register, because (unlike its AArch32
> counterpart) they are currently done directly to the CPU state struct
> without going through the sdcr_write() function.
> 
> Give MDCR_EL3 a writefn which handles the PMU start/finish calls.
> The SDCR writefn then simplfies to "call the MDCR_EL3 writefn after
> masking off the bits which don't exist in the AArch32 register".
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/3] target/arm: Update SDCR_VALID_MASK to include SCCD

[Peter Maydell]

Our SDCR_VALID_MASK doesn't include all of the bits which are defined
by the current architecture.  In particular in commit 0b42f4fab9d3 we
forgot to add SCCD, which meant that an AArch32 guest couldn't
actually use the SCCD bit to disable counting in Secure state.

Add all the currently defined bits; we don't implement all of them,
but this makes them be reads-as-written, which is architecturally
valid and matches how we currently handle most of the others in the
mask.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 33cdbc0143e..429ed42eece 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1334,11 +1334,15 @@ FIELD(CPTR_EL3, TTA, 20, 1)
 FIELD(CPTR_EL3, TAM, 30, 1)
 FIELD(CPTR_EL3, TCPAC, 31, 1)
 
+#define MDCR_MTPME    (1U << 28)
+#define MDCR_TDCC     (1U << 27)
 #define MDCR_HLP      (1U << 26)  /* MDCR_EL2 */
 #define MDCR_SCCD     (1U << 23)  /* MDCR_EL3 */
 #define MDCR_HCCD     (1U << 23)  /* MDCR_EL2 */
 #define MDCR_EPMAD    (1U << 21)
 #define MDCR_EDAD     (1U << 20)
+#define MDCR_TTRF     (1U << 19)
+#define MDCR_STE      (1U << 18)  /* MDCR_EL3 */
 #define MDCR_SPME     (1U << 17)  /* MDCR_EL3 */
 #define MDCR_HPMD     (1U << 17)  /* MDCR_EL2 */
 #define MDCR_SDD      (1U << 16)
@@ -1353,7 +1357,9 @@ FIELD(CPTR_EL3, TCPAC, 31, 1)
 #define MDCR_HPMN     (0x1fU)
 
 /* Not all of the MDCR_EL3 bits are present in the 32-bit SDCR */
-#define SDCR_VALID_MASK (MDCR_EPMAD | MDCR_EDAD | MDCR_SPME | MDCR_SPD)
+#define SDCR_VALID_MASK (MDCR_MTPME | MDCR_TDCC | MDCR_SCCD | \
+                         MDCR_EPMAD | MDCR_EDAD | MDCR_TTRF | \
+                         MDCR_STE | MDCR_SPME | MDCR_SPD)
 
 #define CPSR_M (0x1fU)
 #define CPSR_T (1U << 5)
-- 
2.25.1

Re: [PATCH 3/3] target/arm: Update SDCR_VALID_MASK to include SCCD

[Richard Henderson]

On 9/23/22 05:34, Peter Maydell wrote:
> Our SDCR_VALID_MASK doesn't include all of the bits which are defined
> by the current architecture.  In particular in commit 0b42f4fab9d3 we
> forgot to add SCCD, which meant that an AArch32 guest couldn't
> actually use the SCCD bit to disable counting in Secure state.
> 
> Add all the currently defined bits; we don't implement all of them,
> but this makes them be reads-as-written, which is architecturally
> valid and matches how we currently handle most of the others in the
> mask.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~