[PATCH 00/10] target/arm: Implement FEAT_PMUv3p5

[PATCH 00/10] target/arm: Implement FEAT_PMUv3p5

[Peter Maydell]

This patchset implements the Armv8.5 feature FEAT_PMUv3p5, which is
a set of minor enhancements to the PMU:
 * EL2 and EL3 can now prohibit the cycle counter from counting
   when in EL2 or when Secure, using new MDCR_EL2.HCCD and
   MDCR_EL3.SCCD bits
 * event counters are now 64 bits, with the overflow detection
   configurably at the 32 bit or 64 bit mark

It also fixes a set of bugs in the existing PMU emulation which I
discovered while trying to test my additions.

This is of course all intended for 7.2.

thanks
-- PMM

Peter Maydell (10):
  target/arm: Don't corrupt high half of PMOVSR when cycle counter
    overflows
  target/arm: Correct value returned by pmu_counter_mask()
  target/arm: Don't mishandle count when enabling or disabling PMU
    counters
  target/arm: Ignore PMCR.D when PMCR.LC is set
  target/arm: Honour MDCR_EL2.HPMD in Secure EL2
  target/arm: Detect overflow when calculating next PMU interrupt
  target/arm: Rename pmu_8_n feature test functions
  target/arm: Implement FEAT_PMUv3p5 cycle counter disable bits
  target/arm: Support 64-bit event counters for FEAT_PMUv3p5
  target/arm: Report FEAT_PMUv3p5 for TCG '-cpu max'

 target/arm/cpu.h       |  37 ++++++--
 target/arm/internals.h |   5 +-
 target/arm/cpu64.c     |   2 +-
 target/arm/cpu_tcg.c   |   2 +-
 target/arm/helper.c    | 197 ++++++++++++++++++++++++++++++++---------
 5 files changed, 190 insertions(+), 53 deletions(-)

-- 
2.25.1

[PATCH 01/10] target/arm: Don't corrupt high half of PMOVSR when cycle counter overflows

[Peter Maydell]

When the cycle counter overflows, we are intended to set bit 31 in PMOVSR
to indicate this. However a missing ULL suffix means that we end up
setting all of bits 63-31. Fix the bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d7bc467a2a5..87c89748954 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1186,7 +1186,7 @@ static void pmccntr_op_start(CPUARMState *env)
         uint64_t overflow_mask = env->cp15.c9_pmcr & PMCRLC ? \
                                  1ull << 63 : 1ull << 31;
         if (env->cp15.c15_ccnt & ~new_pmccntr & overflow_mask) {
-            env->cp15.c9_pmovsr |= (1 << 31);
+            env->cp15.c9_pmovsr |= (1ULL << 31);
             pmu_update_irq(env);
         }
 
-- 
2.25.1

Re: [PATCH 01/10] target/arm: Don't corrupt high half of PMOVSR when cycle counter overflows

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> When the cycle counter overflows, we are intended to set bit 31 in PMOVSR
> to indicate this. However a missing ULL suffix means that we end up
> setting all of bits 63-31. Fix the bug.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d7bc467a2a5..87c89748954 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1186,7 +1186,7 @@ static void pmccntr_op_start(CPUARMState *env)
>           uint64_t overflow_mask = env->cp15.c9_pmcr & PMCRLC ? \
>                                    1ull << 63 : 1ull << 31;
>           if (env->cp15.c15_ccnt & ~new_pmccntr & overflow_mask) {
> -            env->cp15.c9_pmovsr |= (1 << 31);
> +            env->cp15.c9_pmovsr |= (1ULL << 31);
>               pmu_update_irq(env);
>           }
>   
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 02/10] target/arm: Correct value returned by pmu_counter_mask()

[Peter Maydell]

pmu_counter_mask() accidentally returns a value with bits [63:32]
set, because the expression it returns is evaluated as a signed value
that gets sign-extended to 64 bits.  Force the whole expression to be
evaluated with 64-bit arithmetic with ULL suffixes.

The main effect of this bug was that a guest could write to the bits
in the high half of registers like PMCNTENSET_EL0 that are supposed
to be RES0.

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

diff --git a/target/arm/internals.h b/target/arm/internals.h
index b8fefdff675..83526166de0 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1296,7 +1296,7 @@ static inline uint32_t pmu_num_counters(CPUARMState *env)
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 static inline uint64_t pmu_counter_mask(CPUARMState *env)
 {
-  return (1 << 31) | ((1 << pmu_num_counters(env)) - 1);
+  return (1ULL << 31) | ((1ULL << pmu_num_counters(env)) - 1);
 }
 
 #ifdef TARGET_AARCH64
-- 
2.25.1

Re: [PATCH 02/10] target/arm: Correct value returned by pmu_counter_mask()

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> pmu_counter_mask() accidentally returns a value with bits [63:32]
> set, because the expression it returns is evaluated as a signed value
> that gets sign-extended to 64 bits.  Force the whole expression to be
> evaluated with 64-bit arithmetic with ULL suffixes.
> 
> The main effect of this bug was that a guest could write to the bits
> in the high half of registers like PMCNTENSET_EL0 that are supposed
> to be RES0.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/internals.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index b8fefdff675..83526166de0 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1296,7 +1296,7 @@ static inline uint32_t pmu_num_counters(CPUARMState *env)
>   /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
>   static inline uint64_t pmu_counter_mask(CPUARMState *env)
>   {
> -  return (1 << 31) | ((1 << pmu_num_counters(env)) - 1);
> +  return (1ULL << 31) | ((1ULL << pmu_num_counters(env)) - 1);
>   }
>   
>   #ifdef TARGET_AARCH64
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 03/10] target/arm: Don't mishandle count when enabling or disabling PMU counters

[Peter Maydell]

The PMU cycle and event counter infrastructure design requires that
operations on the PMU register fields are wrapped in pmu_op_start()
and pmu_op_finish() calls (or their more specific pmmcntr and
pmevcntr equivalents).  This includes any changes to registers which
affect whether the counter should be enabled or disabled, but we
forgot to do this.

The effect of this bug is that in sequences like:
 * disable the cycle counter (PMCCNTR) using the PMCNTEN register
 * write a value such as 0xfffff000 to the PMCCNTR
 * restart the counter by writing to PMCNTEN
the value written to the cycle counter is corrupted, and it starts
counting from the wrong place. (Essentially, we fail to record that
the QEMU_CLOCK_VIRTUAL timestamp when the counter should be considered
to have started counting is the point when PMCNTEN is written to enable
the counter.)

Add the necessary bracketing calls, so that updates to the various
registers which affect whether the PMU is counting are handled
correctly.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 87c89748954..7a367371921 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1079,6 +1079,14 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
     return pmreg_access(env, ri, isread);
 }
 
+/*
+ * Bits in MDCR_EL2 and MDCR_EL3 which pmu_counter_enabled() looks at.
+ * We use these to decide whether we need to wrap a write to MDCR_EL2
+ * or MDCR_EL3 in pmu_op_start()/pmu_op_finish() calls.
+ */
+#define MDCR_EL2_PMU_ENABLE_BITS (MDCR_HPME | MDCR_HPMD | MDCR_HPMN)
+#define MDCR_EL3_PMU_ENABLE_BITS (MDCR_SPME)
+
 /* Returns true if the counter (pass 31 for PMCCNTR) should count events using
  * the current EL, security state, and register configuration.
  */
@@ -1432,15 +1440,19 @@ static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
+    pmu_op_start(env);
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmcnten |= value;
+    pmu_op_finish(env);
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
+    pmu_op_start(env);
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmcnten &= ~value;
+    pmu_op_finish(env);
 }
 
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -4681,7 +4693,39 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
+    /*
+     * Some MDCR_EL3 bits affect whether PMU counters are running:
+     * if we are trying to change any of those then we must
+     * bracket this update with PMU start/finish calls.
+     */
+    bool pmu_op = (env->cp15.mdcr_el3 ^ value) & MDCR_EL3_PMU_ENABLE_BITS;
+
+    if (pmu_op) {
+        pmu_op_start(env);
+    }
     env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
+    if (pmu_op) {
+        pmu_op_finish(env);
+    }
+}
+
+static void mdcr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    /*
+     * Some MDCR_EL3 bits affect whether PMU counters are running:
+     * if we are trying to change any of those then we must
+     * bracket this update with PMU start/finish calls.
+     */
+    bool pmu_op = (env->cp15.mdcr_el2 ^ value) & MDCR_EL2_PMU_ENABLE_BITS;
+
+    if (pmu_op) {
+        pmu_op_start(env);
+    }
+    env->cp15.mdcr_el2 = value;
+    if (pmu_op) {
+        pmu_op_finish(env);
+    }
 }
 
 static const ARMCPRegInfo v8_cp_reginfo[] = {
@@ -7669,6 +7713,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         ARMCPRegInfo mdcr_el2 = {
             .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
             .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
+            .writefn = mdcr_el2_write,
             .access = PL2_RW, .resetvalue = pmu_num_counters(env),
             .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2),
         };
-- 
2.25.1

Re: [PATCH 03/10] target/arm: Don't mishandle count when enabling or disabling PMU counters

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> +static void mdcr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                           uint64_t value)
> +{
> +    /*
> +     * Some MDCR_EL3 bits affect whether PMU counters are running:

Typo el3.

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

r~

> +     * if we are trying to change any of those then we must
> +     * bracket this update with PMU start/finish calls.
> +     */
> +    bool pmu_op = (env->cp15.mdcr_el2 ^ value) & MDCR_EL2_PMU_ENABLE_BITS;
> +
> +    if (pmu_op) {
> +        pmu_op_start(env);
> +    }
> +    env->cp15.mdcr_el2 = value;
> +    if (pmu_op) {
> +        pmu_op_finish(env);
> +    }
>   }
>   
>   static const ARMCPRegInfo v8_cp_reginfo[] = {
> @@ -7669,6 +7713,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>           ARMCPRegInfo mdcr_el2 = {
>               .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>               .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
> +            .writefn = mdcr_el2_write,
>               .access = PL2_RW, .resetvalue = pmu_num_counters(env),
>               .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2),
>           };

[PATCH 04/10] target/arm: Ignore PMCR.D when PMCR.LC is set

[Peter Maydell]

The architecture requires that if PMCR.LC is set (for a 64-bit cycle
counter) then PMCR.D (which enables the clock divider so the counter
ticks every 64 cycles rather than every cycle) should be ignored.  We
were always honouring PMCR.D; fix the bug so we correctly ignore it
in this situation.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7a367371921..41def52cf7b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1172,6 +1172,17 @@ static void pmu_update_irq(CPUARMState *env)
             (env->cp15.c9_pminten & env->cp15.c9_pmovsr));
 }
 
+static bool pmccntr_clockdiv_enabled(CPUARMState *env)
+{
+    /*
+     * Return true if the clock divider is enabled and the cycle counter
+     * is supposed to tick only once every 64 clock cycles. This is
+     * controlled by PMCR.D, but if PMCR.LC is set to enable the long
+     * (64-bit) cycle counter PMCR.D has no effect.
+     */
+    return (env->cp15.c9_pmcr & (PMCRD | PMCRLC)) == PMCRD;
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1184,8 +1195,7 @@ static void pmccntr_op_start(CPUARMState *env)
 
     if (pmu_counter_enabled(env, 31)) {
         uint64_t eff_cycles = cycles;
-        if (env->cp15.c9_pmcr & PMCRD) {
-            /* Increment once every 64 processor clock cycles */
+        if (pmccntr_clockdiv_enabled(env)) {
             eff_cycles /= 64;
         }
 
@@ -1228,8 +1238,7 @@ static void pmccntr_op_finish(CPUARMState *env)
 #endif
 
         uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
-        if (env->cp15.c9_pmcr & PMCRD) {
-            /* Increment once every 64 processor clock cycles */
+        if (pmccntr_clockdiv_enabled(env)) {
             prev_cycles /= 64;
         }
         env->cp15.c15_ccnt_delta = prev_cycles - env->cp15.c15_ccnt;
-- 
2.25.1

Re: [PATCH 04/10] target/arm: Ignore PMCR.D when PMCR.LC is set

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> The architecture requires that if PMCR.LC is set (for a 64-bit cycle
> counter) then PMCR.D (which enables the clock divider so the counter
> ticks every 64 cycles rather than every cycle) should be ignored.  We
> were always honouring PMCR.D; fix the bug so we correctly ignore it
> in this situation.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)

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

r~

[PATCH 05/10] target/arm: Honour MDCR_EL2.HPMD in Secure EL2

[Peter Maydell]

The logic in pmu_counter_enabled() for handling the 'prohibit event
counting' bits MDCR_EL2.HPMD and MDCR_EL3.SPME is written in a way
that assumes that EL2 is never Secure.  This used to be true, but the
architecture now permits Secure EL2, and QEMU can emulate this.

Refactor the prohibit logic so that we effectively OR together
the various prohibit bits when they apply, rather than trying to
construct an if-else ladder where any particular state of the CPU
ends up in exactly one branch of the ladder.

This fixes the Secure EL2 case and also is a better structure for
adding the PMUv8.5 bits MDCR_EL2.HCCD and MDCR_EL3.SCCD.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 41def52cf7b..434885d024a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1094,7 +1094,7 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
 {
     uint64_t filter;
     bool e, p, u, nsk, nsu, nsh, m;
-    bool enabled, prohibited, filtered;
+    bool enabled, prohibited = false, filtered;
     bool secure = arm_is_secure(env);
     int el = arm_current_el(env);
     uint64_t mdcr_el2 = arm_mdcr_el2_eff(env);
@@ -1112,15 +1112,12 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
     }
     enabled = e && (env->cp15.c9_pmcnten & (1 << counter));
 
-    if (!secure) {
-        if (el == 2 && (counter < hpmn || counter == 31)) {
-            prohibited = mdcr_el2 & MDCR_HPMD;
-        } else {
-            prohibited = false;
-        }
-    } else {
-        prohibited = arm_feature(env, ARM_FEATURE_EL3) &&
-           !(env->cp15.mdcr_el3 & MDCR_SPME);
+    /* Is event counting prohibited? */
+    if (el == 2 && (counter < hpmn || counter == 31)) {
+        prohibited = mdcr_el2 & MDCR_HPMD;
+    }
+    if (secure) {
+        prohibited = prohibited || !(env->cp15.mdcr_el3 & MDCR_SPME);
     }
 
     if (prohibited && counter == 31) {
-- 
2.25.1

Re: [PATCH 05/10] target/arm: Honour MDCR_EL2.HPMD in Secure EL2

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> The logic in pmu_counter_enabled() for handling the 'prohibit event
> counting' bits MDCR_EL2.HPMD and MDCR_EL3.SPME is written in a way
> that assumes that EL2 is never Secure.  This used to be true, but the
> architecture now permits Secure EL2, and QEMU can emulate this.
> 
> Refactor the prohibit logic so that we effectively OR together
> the various prohibit bits when they apply, rather than trying to
> construct an if-else ladder where any particular state of the CPU
> ends up in exactly one branch of the ladder.
> 
> This fixes the Secure EL2 case and also is a better structure for
> adding the PMUv8.5 bits MDCR_EL2.HCCD and MDCR_EL3.SCCD.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 41def52cf7b..434885d024a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1094,7 +1094,7 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
>   {
>       uint64_t filter;
>       bool e, p, u, nsk, nsu, nsh, m;
> -    bool enabled, prohibited, filtered;
> +    bool enabled, prohibited = false, filtered;
>       bool secure = arm_is_secure(env);
>       int el = arm_current_el(env);
>       uint64_t mdcr_el2 = arm_mdcr_el2_eff(env);
> @@ -1112,15 +1112,12 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
>       }
>       enabled = e && (env->cp15.c9_pmcnten & (1 << counter));
>   
> -    if (!secure) {
> -        if (el == 2 && (counter < hpmn || counter == 31)) {
> -            prohibited = mdcr_el2 & MDCR_HPMD;
> -        } else {
> -            prohibited = false;
> -        }
> -    } else {
> -        prohibited = arm_feature(env, ARM_FEATURE_EL3) &&
> -           !(env->cp15.mdcr_el3 & MDCR_SPME);
> +    /* Is event counting prohibited? */
> +    if (el == 2 && (counter < hpmn || counter == 31)) {
> +        prohibited = mdcr_el2 & MDCR_HPMD;
> +    }
> +    if (secure) {
> +        prohibited = prohibited || !(env->cp15.mdcr_el3 & MDCR_SPME);

You can use |= for bool as well.  You don't need the short-circuting of || here.
For consistency, perhaps better to use |= with the first if as well, relying on the 
compiler to fold the initial value false.

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

r~

Re: [PATCH 05/10] target/arm: Honour MDCR_EL2.HPMD in Secure EL2

[Peter Maydell]

On Thu, 11 Aug 2022 at 18:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
> You can use |= for bool as well.  You don't need the short-circuting of || here.

That seems like the kind of thing that -Wbool-operation is likely to
warn about either now or in future, though...

-- PMM

[PATCH 06/10] target/arm: Detect overflow when calculating next PMU interrupt

[Peter Maydell]

In pmccntr_op_finish() and pmevcntr_op_finish() we calculate the next
point at which we will get an overflow and need to fire the PMU
interrupt or set the overflow flag.  We do this by calculating the
number of nanoseconds to the overflow event and then adding it to
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).  However, we don't check
whether that signed addition overflows, which can happen if the next
PMU interrupt would happen massively far in the future (250 years or
more).

Since QEMU assumes that "when the QEMU_CLOCK_VIRTUAL rolls over" is
"never", the sensible behaviour in this situation is simply to not
try to set the timer if it would be beyond that point.  Detect the
overflow, and skip setting the timer in that case.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 434885d024a..b7a420981f8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1227,10 +1227,13 @@ static void pmccntr_op_finish(CPUARMState *env)
         int64_t overflow_in = cycles_ns_per(remaining_cycles);
 
         if (overflow_in > 0) {
-            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-                overflow_in;
-            ARMCPU *cpu = env_archcpu(env);
-            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+            int64_t overflow_at;
+
+            if (!sadd64_overflow(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                                overflow_in, &overflow_at)) {
+                ARMCPU *cpu = env_archcpu(env);
+                timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+            }
         }
 #endif
 
@@ -1275,10 +1278,13 @@ static void pmevcntr_op_finish(CPUARMState *env, uint8_t counter)
         int64_t overflow_in = pm_events[event_idx].ns_per_count(delta);
 
         if (overflow_in > 0) {
-            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-                overflow_in;
-            ARMCPU *cpu = env_archcpu(env);
-            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+            int64_t overflow_at;
+
+            if (!sadd64_overflow(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                                 overflow_in, &overflow_at)) {
+                ARMCPU *cpu = env_archcpu(env);
+                timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+            }
         }
 #endif
 
-- 
2.25.1

Re: [PATCH 06/10] target/arm: Detect overflow when calculating next PMU interrupt

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> In pmccntr_op_finish() and pmevcntr_op_finish() we calculate the next
> point at which we will get an overflow and need to fire the PMU
> interrupt or set the overflow flag.  We do this by calculating the
> number of nanoseconds to the overflow event and then adding it to
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).  However, we don't check
> whether that signed addition overflows, which can happen if the next
> PMU interrupt would happen massively far in the future (250 years or
> more).
> 
> Since QEMU assumes that "when the QEMU_CLOCK_VIRTUAL rolls over" is
> "never", the sensible behaviour in this situation is simply to not
> try to set the timer if it would be beyond that point.  Detect the
> overflow, and skip setting the timer in that case.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 434885d024a..b7a420981f8 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1227,10 +1227,13 @@ static void pmccntr_op_finish(CPUARMState *env)
>           int64_t overflow_in = cycles_ns_per(remaining_cycles);
>   
>           if (overflow_in > 0) {
> -            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -                overflow_in;
> -            ARMCPU *cpu = env_archcpu(env);
> -            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
> +            int64_t overflow_at;
> +
> +            if (!sadd64_overflow(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                                overflow_in, &overflow_at)) {

Maybe my mailer, but it looks like alignment is off on the continuation line here...

> +            int64_t overflow_at;
> +
> +            if (!sadd64_overflow(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                                 overflow_in, &overflow_at)) {

but not here.

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

r~

[PATCH 07/10] target/arm: Rename pmu_8_n feature test functions

[Peter Maydell]

Our feature test functions that check the PMU version are named
isar_feature_{aa32,aa64,any}_pmu_8_{1,4}.  This doesn't match the
current Arm ARM official feature names, which are FEAT_PMUv3p1 and
FEAT_PMUv3p4.  Rename these functions to _pmuv3p1 and _pmuv3p4.

This commit was created with:
  sed -i -e 's/pmu_8_/pmuv3p/g' target/arm/*.[ch]

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h    | 16 ++++++++--------
 target/arm/helper.c | 18 +++++++++---------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5168e3d837e..122ec8a47ec 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3710,14 +3710,14 @@ static inline bool isar_feature_aa32_ats1e1(const ARMISARegisters *id)
     return FIELD_EX32(id->id_mmfr3, ID_MMFR3, PAN) >= 2;
 }
 
-static inline bool isar_feature_aa32_pmu_8_1(const ARMISARegisters *id)
+static inline bool isar_feature_aa32_pmuv3p1(const ARMISARegisters *id)
 {
     /* 0xf means "non-standard IMPDEF PMU" */
     return FIELD_EX32(id->id_dfr0, ID_DFR0, PERFMON) >= 4 &&
         FIELD_EX32(id->id_dfr0, ID_DFR0, PERFMON) != 0xf;
 }
 
-static inline bool isar_feature_aa32_pmu_8_4(const ARMISARegisters *id)
+static inline bool isar_feature_aa32_pmuv3p4(const ARMISARegisters *id)
 {
     /* 0xf means "non-standard IMPDEF PMU" */
     return FIELD_EX32(id->id_dfr0, ID_DFR0, PERFMON) >= 5 &&
@@ -4036,13 +4036,13 @@ static inline bool isar_feature_aa64_sme(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, SME) != 0;
 }
 
-static inline bool isar_feature_aa64_pmu_8_1(const ARMISARegisters *id)
+static inline bool isar_feature_aa64_pmuv3p1(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) >= 4 &&
         FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) != 0xf;
 }
 
-static inline bool isar_feature_aa64_pmu_8_4(const ARMISARegisters *id)
+static inline bool isar_feature_aa64_pmuv3p4(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) >= 5 &&
         FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) != 0xf;
@@ -4211,14 +4211,14 @@ static inline bool isar_feature_any_predinv(const ARMISARegisters *id)
     return isar_feature_aa64_predinv(id) || isar_feature_aa32_predinv(id);
 }
 
-static inline bool isar_feature_any_pmu_8_1(const ARMISARegisters *id)
+static inline bool isar_feature_any_pmuv3p1(const ARMISARegisters *id)
 {
-    return isar_feature_aa64_pmu_8_1(id) || isar_feature_aa32_pmu_8_1(id);
+    return isar_feature_aa64_pmuv3p1(id) || isar_feature_aa32_pmuv3p1(id);
 }
 
-static inline bool isar_feature_any_pmu_8_4(const ARMISARegisters *id)
+static inline bool isar_feature_any_pmuv3p4(const ARMISARegisters *id)
 {
-    return isar_feature_aa64_pmu_8_4(id) || isar_feature_aa32_pmu_8_4(id);
+    return isar_feature_aa64_pmuv3p4(id) || isar_feature_aa32_pmuv3p4(id);
 }
 
 static inline bool isar_feature_any_ccidx(const ARMISARegisters *id)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b7a420981f8..9507375b8e2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -879,16 +879,16 @@ static int64_t instructions_ns_per(uint64_t icount)
 }
 #endif
 
-static bool pmu_8_1_events_supported(CPUARMState *env)
+static bool pmuv3p1_events_supported(CPUARMState *env)
 {
     /* For events which are supported in any v8.1 PMU */
-    return cpu_isar_feature(any_pmu_8_1, env_archcpu(env));
+    return cpu_isar_feature(any_pmuv3p1, env_archcpu(env));
 }
 
-static bool pmu_8_4_events_supported(CPUARMState *env)
+static bool pmuv3p4_events_supported(CPUARMState *env)
 {
     /* For events which are supported in any v8.1 PMU */
-    return cpu_isar_feature(any_pmu_8_4, env_archcpu(env));
+    return cpu_isar_feature(any_pmuv3p4, env_archcpu(env));
 }
 
 static uint64_t zero_event_get_count(CPUARMState *env)
@@ -922,17 +922,17 @@ static const pm_event pm_events[] = {
     },
 #endif
     { .number = 0x023, /* STALL_FRONTEND */
-      .supported = pmu_8_1_events_supported,
+      .supported = pmuv3p1_events_supported,
       .get_count = zero_event_get_count,
       .ns_per_count = zero_event_ns_per,
     },
     { .number = 0x024, /* STALL_BACKEND */
-      .supported = pmu_8_1_events_supported,
+      .supported = pmuv3p1_events_supported,
       .get_count = zero_event_get_count,
       .ns_per_count = zero_event_ns_per,
     },
     { .number = 0x03c, /* STALL */
-      .supported = pmu_8_4_events_supported,
+      .supported = pmuv3p4_events_supported,
       .get_count = zero_event_get_count,
       .ns_per_count = zero_event_ns_per,
     },
@@ -6400,7 +6400,7 @@ static void define_pmu_regs(ARMCPU *cpu)
         g_free(pmevtyper_name);
         g_free(pmevtyper_el0_name);
     }
-    if (cpu_isar_feature(aa32_pmu_8_1, cpu)) {
+    if (cpu_isar_feature(aa32_pmuv3p1, cpu)) {
         ARMCPRegInfo v81_pmu_regs[] = {
             { .name = "PMCEID2", .state = ARM_CP_STATE_AA32,
               .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 4,
@@ -6413,7 +6413,7 @@ static void define_pmu_regs(ARMCPU *cpu)
         };
         define_arm_cp_regs(cpu, v81_pmu_regs);
     }
-    if (cpu_isar_feature(any_pmu_8_4, cpu)) {
+    if (cpu_isar_feature(any_pmuv3p4, cpu)) {
         static const ARMCPRegInfo v84_pmmir = {
             .name = "PMMIR_EL1", .state = ARM_CP_STATE_BOTH,
             .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 6,
-- 
2.25.1

Re: [PATCH 07/10] target/arm: Rename pmu_8_n feature test functions

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> Our feature test functions that check the PMU version are named
> isar_feature_{aa32,aa64,any}_pmu_8_{1,4}.  This doesn't match the
> current Arm ARM official feature names, which are FEAT_PMUv3p1 and
> FEAT_PMUv3p4.  Rename these functions to _pmuv3p1 and _pmuv3p4.
> 
> This commit was created with:
>    sed -i -e 's/pmu_8_/pmuv3p/g' target/arm/*.[ch]
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h    | 16 ++++++++--------
>   target/arm/helper.c | 18 +++++++++---------
>   2 files changed, 17 insertions(+), 17 deletions(-)

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

r~

[PATCH 08/10] target/arm: Implement FEAT_PMUv3p5 cycle counter disable bits

[Peter Maydell]

FEAT_PMUv3p5 introduces new bits MDCR_EL2.HCCD and MDCR_EL3.SCCD,
which disable the cycle counter from counting at EL2 and EL3.
Add the code to support these bits.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h    | 20 ++++++++++++++++++++
 target/arm/helper.c | 20 ++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 122ec8a47ec..9e7fe64ceae 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1332,6 +1332,8 @@ FIELD(CPTR_EL3, TTA, 20, 1)
 FIELD(CPTR_EL3, TAM, 30, 1)
 FIELD(CPTR_EL3, TCPAC, 31, 1)
 
+#define MDCR_MCCD     (1ULL << 34) /* MDCR_EL3 */
+#define MDCR_HCCD     (1U << 23)  /* MDCR_EL2 */
 #define MDCR_EPMAD    (1U << 21)
 #define MDCR_EDAD     (1U << 20)
 #define MDCR_SPME     (1U << 17)  /* MDCR_EL3 */
@@ -3724,6 +3726,13 @@ static inline bool isar_feature_aa32_pmuv3p4(const ARMISARegisters *id)
         FIELD_EX32(id->id_dfr0, ID_DFR0, PERFMON) != 0xf;
 }
 
+static inline bool isar_feature_aa32_pmuv3p5(const ARMISARegisters *id)
+{
+    /* 0xf means "non-standard IMPDEF PMU" */
+    return FIELD_EX32(id->id_dfr0, ID_DFR0, PERFMON) >= 6 &&
+        FIELD_EX32(id->id_dfr0, ID_DFR0, PERFMON) != 0xf;
+}
+
 static inline bool isar_feature_aa32_hpd(const ARMISARegisters *id)
 {
     return FIELD_EX32(id->id_mmfr4, ID_MMFR4, HPDS) != 0;
@@ -4048,6 +4057,12 @@ static inline bool isar_feature_aa64_pmuv3p4(const ARMISARegisters *id)
         FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) != 0xf;
 }
 
+static inline bool isar_feature_aa64_pmuv3p5(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) >= 6 &&
+        FIELD_EX64(id->id_aa64dfr0, ID_AA64DFR0, PMUVER) != 0xf;
+}
+
 static inline bool isar_feature_aa64_rcpc_8_3(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, LRCPC) != 0;
@@ -4221,6 +4236,11 @@ static inline bool isar_feature_any_pmuv3p4(const ARMISARegisters *id)
     return isar_feature_aa64_pmuv3p4(id) || isar_feature_aa32_pmuv3p4(id);
 }
 
+static inline bool isar_feature_any_pmuv3p5(const ARMISARegisters *id)
+{
+    return isar_feature_aa64_pmuv3p5(id) || isar_feature_aa32_pmuv3p5(id);
+}
+
 static inline bool isar_feature_any_ccidx(const ARMISARegisters *id)
 {
     return isar_feature_aa64_ccidx(id) || isar_feature_aa32_ccidx(id);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9507375b8e2..f601025cc40 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1084,8 +1084,8 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
  * We use these to decide whether we need to wrap a write to MDCR_EL2
  * or MDCR_EL3 in pmu_op_start()/pmu_op_finish() calls.
  */
-#define MDCR_EL2_PMU_ENABLE_BITS (MDCR_HPME | MDCR_HPMD | MDCR_HPMN)
-#define MDCR_EL3_PMU_ENABLE_BITS (MDCR_SPME)
+#define MDCR_EL2_PMU_ENABLE_BITS (MDCR_HPME | MDCR_HPMD | MDCR_HPMN | MDCR_HCCD)
+#define MDCR_EL3_PMU_ENABLE_BITS (MDCR_SPME | MDCR_MCCD)
 
 /* Returns true if the counter (pass 31 for PMCCNTR) should count events using
  * the current EL, security state, and register configuration.
@@ -1120,8 +1120,20 @@ static bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
         prohibited = prohibited || !(env->cp15.mdcr_el3 & MDCR_SPME);
     }
 
-    if (prohibited && counter == 31) {
-        prohibited = env->cp15.c9_pmcr & PMCRDP;
+    if (counter == 31) {
+        /*
+         * The cycle counter defaults to running. PMCR.DP says "disable
+         * the cycle counter when event counting is prohibited".
+         * Some MDCR bits disable the cycle counter specifically.
+         */
+        prohibited = prohibited && env->cp15.c9_pmcr & PMCRDP;
+        if (cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
+            if (el == 3) {
+                prohibited = prohibited || (env->cp15.mdcr_el3 & MDCR_MCCD);
+            } else if (el == 2) {
+                prohibited = prohibited || (mdcr_el2 & MDCR_HCCD);
+            }
+        }
     }
 
     if (counter == 31) {
-- 
2.25.1

Re: [PATCH 08/10] target/arm: Implement FEAT_PMUv3p5 cycle counter disable bits

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> FEAT_PMUv3p5 introduces new bits MDCR_EL2.HCCD and MDCR_EL3.SCCD,
> which disable the cycle counter from counting at EL2 and EL3.
> Add the code to support these bits.

While HCCD is v3p5, it seems MCCD (typo above) is v3p7.

> +    if (counter == 31) {
> +        /*
> +         * The cycle counter defaults to running. PMCR.DP says "disable
> +         * the cycle counter when event counting is prohibited".
> +         * Some MDCR bits disable the cycle counter specifically.
> +         */
> +        prohibited = prohibited && env->cp15.c9_pmcr & PMCRDP;
> +        if (cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
> +            if (el == 3) {
> +                prohibited = prohibited || (env->cp15.mdcr_el3 & MDCR_MCCD);
> +            } else if (el == 2) {
> +                prohibited = prohibited || (mdcr_el2 & MDCR_HCCD);
> +            }

But modulo the feature test, the behaviour looks right.


r~

Re: [PATCH 08/10] target/arm: Implement FEAT_PMUv3p5 cycle counter disable bits

[Peter Maydell]

On Sat, 20 Aug 2022 at 18:33, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/11/22 10:16, Peter Maydell wrote:
> > FEAT_PMUv3p5 introduces new bits MDCR_EL2.HCCD and MDCR_EL3.SCCD,
> > which disable the cycle counter from counting at EL2 and EL3.
> > Add the code to support these bits.
>
> While HCCD is v3p5, it seems MCCD (typo above) is v3p7.

DDI0487H.a page D13-6158 says
# SCCD, bit [23]
#    When FEAT_PMUv3p5 is implemented:
#    [...]

MDCR_EL3.MCCD is a different bit, bit 34, which is indeed v3p7.

It looks like I've got confused between the two and implemented
the wrong thing.

MDCR_EL3.SCCD: bit 23: v3p5: don't count in Secure state
MDCR_EL3.MCCD: bit 34: v3p7: don't count in EL3

and for completeness
MDCR_EL2.HCCD: bit 23: v3p5: don't count in EL2

I'll redo the patch to implement HCCD and SCCD. (We can leave
MCCD until we get to the PMUv3p7 feature.)

thanks
-- PMM

[PATCH 09/10] target/arm: Support 64-bit event counters for FEAT_PMUv3p5

[Peter Maydell]

With FEAT_PMUv3p5, the event counters are now 64 bit, rather than 32
bit.  (Previously, only the cycle counter could be 64 bit, and other
event counters were always 32 bits).  For any given event counter,
whether the overflow event is noted for overflow from bit 31 or from
bit 63 is controlled by a combination of PMCR.LP, MDCR_EL2.HLP and
MDCR_EL2.HPMN.

Implement the 64-bit event counter handling.  We choose to make our
counters always 64 bits, and mask out the top 32 bits on read or
write of PMXEVCNTR for CPUs which don't have FEAT_PMUv3p5.

(Note that the changes to pmenvcntr_op_start() and
pmenvcntr_op_finish() bring their logic closer into line with that of
pmccntr_op_start() and pmccntr_op_finish(), which already had to cope
with the overflow being either at 32 or 64 bits.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       |  1 +
 target/arm/internals.h |  3 +-
 target/arm/helper.c    | 62 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e7fe64ceae..b268a2f5705 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1333,6 +1333,7 @@ FIELD(CPTR_EL3, TAM, 30, 1)
 FIELD(CPTR_EL3, TCPAC, 31, 1)
 
 #define MDCR_MCCD     (1ULL << 34) /* MDCR_EL3 */
+#define MDCR_HLP      (1U << 26)  /* MDCR_EL2 */
 #define MDCR_HCCD     (1U << 23)  /* MDCR_EL2 */
 #define MDCR_EPMAD    (1U << 21)
 #define MDCR_EDAD     (1U << 20)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 83526166de0..bf60cd5f845 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1256,6 +1256,7 @@ enum MVEECIState {
 /* Definitions for the PMU registers */
 #define PMCRN_MASK  0xf800
 #define PMCRN_SHIFT 11
+#define PMCRLP  0x80
 #define PMCRLC  0x40
 #define PMCRDP  0x20
 #define PMCRX   0x10
@@ -1267,7 +1268,7 @@ enum MVEECIState {
  * Mask of PMCR bits writable by guest (not including WO bits like C, P,
  * which can be written as 1 to trigger behaviour but which stay RAZ).
  */
-#define PMCR_WRITABLE_MASK (PMCRLC | PMCRDP | PMCRX | PMCRD | PMCRE)
+#define PMCR_WRITABLE_MASK (PMCRLP | PMCRLC | PMCRDP | PMCRX | PMCRD | PMCRE)
 
 #define PMXEVTYPER_P          0x80000000
 #define PMXEVTYPER_U          0x40000000
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f601025cc40..cbcf7a1f43c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1084,7 +1084,8 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
  * We use these to decide whether we need to wrap a write to MDCR_EL2
  * or MDCR_EL3 in pmu_op_start()/pmu_op_finish() calls.
  */
-#define MDCR_EL2_PMU_ENABLE_BITS (MDCR_HPME | MDCR_HPMD | MDCR_HPMN | MDCR_HCCD)
+#define MDCR_EL2_PMU_ENABLE_BITS \
+    (MDCR_HPME | MDCR_HPMD | MDCR_HPMN | MDCR_HCCD | MDCR_HLP)
 #define MDCR_EL3_PMU_ENABLE_BITS (MDCR_SPME | MDCR_MCCD)
 
 /* Returns true if the counter (pass 31 for PMCCNTR) should count events using
@@ -1192,6 +1193,32 @@ static bool pmccntr_clockdiv_enabled(CPUARMState *env)
     return (env->cp15.c9_pmcr & (PMCRD | PMCRLC)) == PMCRD;
 }
 
+static bool pmevcntr_is_64_bit(CPUARMState *env, int counter)
+{
+    /* Return true if the specified event counter is configured to be 64 bit */
+
+    /* This isn't intended to be used with the cycle counter */
+    assert(counter < 31);
+
+    if (!cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
+        return false;
+    }
+
+    if (arm_feature(env, ARM_FEATURE_EL2)) {
+        /*
+         * MDCR_EL2.HLP still applies even when EL2 is disabled in the
+         * current security state, so we don't use arm_mdcr_el2_eff() here.
+         */
+        bool hlp = env->cp15.mdcr_el2 & MDCR_HLP;
+        int hpmn = env->cp15.mdcr_el2 & MDCR_HPMN;
+
+        if (hpmn != 0 && counter >= hpmn) {
+            return hlp;
+        }
+    }
+    return env->cp15.c9_pmcr & PMCRLP;
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1268,9 +1295,11 @@ static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
     }
 
     if (pmu_counter_enabled(env, counter)) {
-        uint32_t new_pmevcntr = count - env->cp15.c14_pmevcntr_delta[counter];
+        uint64_t new_pmevcntr = count - env->cp15.c14_pmevcntr_delta[counter];
+        uint64_t overflow_mask = pmevcntr_is_64_bit(env, counter) ?
+            1ULL << 63 : 1ULL << 31;
 
-        if (env->cp15.c14_pmevcntr[counter] & ~new_pmevcntr & INT32_MIN) {
+        if (env->cp15.c14_pmevcntr[counter] & ~new_pmevcntr & overflow_mask) {
             env->cp15.c9_pmovsr |= (1 << counter);
             pmu_update_irq(env);
         }
@@ -1285,9 +1314,13 @@ static void pmevcntr_op_finish(CPUARMState *env, uint8_t counter)
 #ifndef CONFIG_USER_ONLY
         uint16_t event = env->cp15.c14_pmevtyper[counter] & PMXEVTYPER_EVTCOUNT;
         uint16_t event_idx = supported_event_map[event];
-        uint64_t delta = UINT32_MAX -
-            (uint32_t)env->cp15.c14_pmevcntr[counter] + 1;
-        int64_t overflow_in = pm_events[event_idx].ns_per_count(delta);
+        uint64_t delta = -(env->cp15.c14_pmevcntr[counter] + 1);
+        int64_t overflow_in;
+
+        if (!pmevcntr_is_64_bit(env, counter)) {
+            delta = (uint32_t)delta;
+        }
+        overflow_in = pm_events[event_idx].ns_per_count(delta);
 
         if (overflow_in > 0) {
             int64_t overflow_at;
@@ -1374,6 +1407,8 @@ static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
                           uint64_t value)
 {
     unsigned int i;
+    uint64_t overflow_mask, new_pmswinc;
+
     for (i = 0; i < pmu_num_counters(env); i++) {
         /* Increment a counter's count iff: */
         if ((value & (1 << i)) && /* counter's bit is set */
@@ -1387,9 +1422,12 @@ static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
              * Detect if this write causes an overflow since we can't predict
              * PMSWINC overflows like we can for other events
              */
-            uint32_t new_pmswinc = env->cp15.c14_pmevcntr[i] + 1;
+            new_pmswinc = env->cp15.c14_pmevcntr[i] + 1;
 
-            if (env->cp15.c14_pmevcntr[i] & ~new_pmswinc & INT32_MIN) {
+            overflow_mask = pmevcntr_is_64_bit(env, i) ?
+                1ULL << 63 : 1ULL << 31;
+
+            if (env->cp15.c14_pmevcntr[i] & ~new_pmswinc & overflow_mask) {
                 env->cp15.c9_pmovsr |= (1 << i);
                 pmu_update_irq(env);
             }
@@ -1596,6 +1634,10 @@ static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
 static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value, uint8_t counter)
 {
+    if (!cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
+        /* Before FEAT_PMUv3p5, top 32 bits of event counters are RES0 */
+        value &= MAKE_64BIT_MASK(0, 32);
+    }
     if (counter < pmu_num_counters(env)) {
         pmevcntr_op_start(env, counter);
         env->cp15.c14_pmevcntr[counter] = value;
@@ -1615,6 +1657,10 @@ static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
         pmevcntr_op_start(env, counter);
         ret = env->cp15.c14_pmevcntr[counter];
         pmevcntr_op_finish(env, counter);
+        if (!cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
+            /* Before FEAT_PMUv3p5, top 32 bits of event counters are RES0 */
+            ret &= MAKE_64BIT_MASK(0, 32);
+        }
         return ret;
     } else {
       /* We opt to behave as a RAZ/WI when attempts to access PM[X]EVCNTR
-- 
2.25.1

Re: [PATCH 09/10] target/arm: Support 64-bit event counters for FEAT_PMUv3p5

[Richard Henderson]

On 8/11/22 10:16, Peter Maydell wrote:
> +static bool pmevcntr_is_64_bit(CPUARMState *env, int counter)
> +{
> +    /* Return true if the specified event counter is configured to be 64 bit */
> +
> +    /* This isn't intended to be used with the cycle counter */
> +    assert(counter < 31);
> +
> +    if (!cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
> +        return false;
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL2)) {
> +        /*
> +         * MDCR_EL2.HLP still applies even when EL2 is disabled in the
> +         * current security state, so we don't use arm_mdcr_el2_eff() here.
> +         */
> +        bool hlp = env->cp15.mdcr_el2 & MDCR_HLP;
> +        int hpmn = env->cp15.mdcr_el2 & MDCR_HPMN;

The specs could be improved here, as the top of MDCR_EL2 says it doesn't apply if EL2 
isn't enabled in the security state, HLP has the exception noted above, but HPMN does not. 
  I conclude that HPMN is missing the exception, because nothing else makes sense.

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


r~

Re: [PATCH 09/10] target/arm: Support 64-bit event counters for FEAT_PMUv3p5

[Peter Maydell]

On Sat, 20 Aug 2022 at 19:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/11/22 10:16, Peter Maydell wrote:
> > +static bool pmevcntr_is_64_bit(CPUARMState *env, int counter)
> > +{
> > +    /* Return true if the specified event counter is configured to be 64 bit */
> > +
> > +    /* This isn't intended to be used with the cycle counter */
> > +    assert(counter < 31);
> > +
> > +    if (!cpu_isar_feature(any_pmuv3p5, env_archcpu(env))) {
> > +        return false;
> > +    }
> > +
> > +    if (arm_feature(env, ARM_FEATURE_EL2)) {
> > +        /*
> > +         * MDCR_EL2.HLP still applies even when EL2 is disabled in the
> > +         * current security state, so we don't use arm_mdcr_el2_eff() here.
> > +         */
> > +        bool hlp = env->cp15.mdcr_el2 & MDCR_HLP;
> > +        int hpmn = env->cp15.mdcr_el2 & MDCR_HPMN;
>
> The specs could be improved here, as the top of MDCR_EL2 says it doesn't apply if EL2
> isn't enabled in the security state, HLP has the exception noted above, but HPMN does not.
>   I conclude that HPMN is missing the exception, because nothing else makes sense.

I suspect (but haven't thought through) that not all of the things HPMN
controls apply when EL2 is disabled, so it's probably more complicated
than a blanket "applies even when EL2 is disabled" tag the way HLP does it.

thanks
-- PMM

[PATCH 10/10] target/arm: Report FEAT_PMUv3p5 for TCG '-cpu max'

[Peter Maydell]

Update the ID registers for TCG's '-cpu max' to report a FEAT_PMUv3p5
compliant PMU.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu64.c   | 2 +-
 target/arm/cpu_tcg.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 78e27f778ac..fa4b0152706 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -1072,7 +1072,7 @@ static void aarch64_max_initfn(Object *obj)
 
     t = cpu->isar.id_aa64dfr0;
     t = FIELD_DP64(t, ID_AA64DFR0, DEBUGVER, 9);  /* FEAT_Debugv8p4 */
-    t = FIELD_DP64(t, ID_AA64DFR0, PMUVER, 5);    /* FEAT_PMUv3p4 */
+    t = FIELD_DP64(t, ID_AA64DFR0, PMUVER, 6);    /* FEAT_PMUv3p5 */
     cpu->isar.id_aa64dfr0 = t;
 
     t = cpu->isar.id_aa64smfr0;
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index 3099b38e32b..4c71a0b612d 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -81,7 +81,7 @@ void aa32_max_features(ARMCPU *cpu)
     t = cpu->isar.id_dfr0;
     t = FIELD_DP32(t, ID_DFR0, COPDBG, 9);        /* FEAT_Debugv8p4 */
     t = FIELD_DP32(t, ID_DFR0, COPSDBG, 9);       /* FEAT_Debugv8p4 */
-    t = FIELD_DP32(t, ID_DFR0, PERFMON, 5);       /* FEAT_PMUv3p4 */
+    t = FIELD_DP32(t, ID_DFR0, PERFMON, 6);       /* FEAT_PMUv3p5 */
     cpu->isar.id_dfr0 = t;
 }
 
-- 
2.25.1

Re: [PATCH 10/10] target/arm: Report FEAT_PMUv3p5 for TCG '-cpu max'

[Peter Maydell]

On Thu, 11 Aug 2022 at 18:16, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Update the ID registers for TCG's '-cpu max' to report a FEAT_PMUv3p5
> compliant PMU.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Oops, forgot the docs update:

--- a/docs/system/arm/emulation.rst
+++ b/docs/system/arm/emulation.rst
@@ -52,6 +52,7 @@ the following architecture extensions:
 - FEAT_PMULL (PMULL, PMULL2 instructions)
 - FEAT_PMUv3p1 (PMU Extensions v3.1)
 - FEAT_PMUv3p4 (PMU Extensions v3.4)
+- FEAT_PMUv3p5 (PMU Extensions v3.5)
 - FEAT_RAS (Reliability, availability, and serviceability)
 - FEAT_RASv1p1 (RAS Extension v1.1)
 - FEAT_RDM (Advanced SIMD rounding double multiply accumulate instructions)


thanks
-- PMM

Re: [PATCH 10/10] target/arm: Report FEAT_PMUv3p5 for TCG '-cpu max'

[Richard Henderson]

On 8/11/22 10:26, Peter Maydell wrote:
> On Thu, 11 Aug 2022 at 18:16, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> Update the ID registers for TCG's '-cpu max' to report a FEAT_PMUv3p5
>> compliant PMU.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Oops, forgot the docs update:
> 
> --- a/docs/system/arm/emulation.rst
> +++ b/docs/system/arm/emulation.rst
> @@ -52,6 +52,7 @@ the following architecture extensions:
>   - FEAT_PMULL (PMULL, PMULL2 instructions)
>   - FEAT_PMUv3p1 (PMU Extensions v3.1)
>   - FEAT_PMUv3p4 (PMU Extensions v3.4)
> +- FEAT_PMUv3p5 (PMU Extensions v3.5)
>   - FEAT_RAS (Reliability, availability, and serviceability)
>   - FEAT_RASv1p1 (RAS Extension v1.1)
>   - FEAT_RDM (Advanced SIMD rounding double multiply accumulate instructions)

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


r~