[PATCH 0/3] arm64: errata: Update AMU workarounds

[PATCH 0/3] arm64: errata: Update AMU workarounds

[Beata Michalska]

Hi All,

This short series updates errata for AMU core & const counters.
The last patch in the series tries to optimize a bit the paired
readings of both counters, so that the workaround checks do not
skew the results. It is also intentionally the last one, as it
will not apply on older stable kernels.


Beata Michalska (3):
  arm64: errata: Extend the workaround for broken AMEVCNTR01 counter
  arm64: errata: Add Cortex-A715/A720 AMU core counter workaround
  arm64: topology: Reduce the skew for AMU feedback counter sampling

 Documentation/arch/arm64/silicon-errata.rst | 12 ++++
 arch/arm64/Kconfig                          | 44 ++++++++++++-
 arch/arm64/kernel/cpu_errata.c              | 29 ++++++++
 arch/arm64/kernel/cpufeature.c              |  3 +-
 arch/arm64/kernel/topology.c                | 73 ++++++++++++++-------
 arch/arm64/tools/cpucaps                    |  1 +
 6 files changed, 133 insertions(+), 29 deletions(-)

-- 
2.43.0

[PATCH 1/3] arm64: errata: Extend the workaround for broken AMEVCNTR01 counter

[Beata Michalska]

The erratum causing significant inaccuracies in the AMU CNT_CYCLES
counter due to WFI/WFE has been identified in additional
microarchitectures.
Extend the existing workaround to cover these implementations,
which are affected by the following errata:

 - ARM Cortex-A725 erratum 3821522 (SDEN-2832921)
 - ARM Cortex-A720 erratum 4689339 (SDEN-2439421)
 - ARM Cortex-A720AE erratum 4689340 (SDEN-3090091)
 - ARM Neoverse N3 erratum 4689341 (SDEN-3050973)
 - ARM C1-Pro erratum 4689344 (SDEN-3273080)

Cc: stable@vger.kernel.org
Signed-off-by: Beata Michalska <beata.michalska@arm.com>
---
 Documentation/arch/arm64/silicon-errata.rst |  8 ++++++++
 arch/arm64/Kconfig                          | 12 +++++++++---
 arch/arm64/kernel/cpu_errata.c              |  9 +++++++++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
index afe109afe692..09fe032e1aea 100644
--- a/Documentation/arch/arm64/silicon-errata.rst
+++ b/Documentation/arch/arm64/silicon-errata.rst
@@ -175,6 +175,10 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A720     | #3456091        | ARM64_ERRATUM_3194386       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Cortex-A720     | #4689339        | ARM64_ERRATUM_3821522       |
++----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Cortex-A720AE   | #4689340        | ARM64_ERRATUM_3821522       |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A725     | #3456106        | ARM64_ERRATUM_3194386       |
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A725     | #3821522        | ARM64_ERRATUM_3821522       |
@@ -233,6 +237,8 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-N3     | #3456111        | ARM64_ERRATUM_3194386       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Neoverse-N3     | #4689341        | ARM64_ERRATUM_3821522       |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-V1     | #1619801        | N/A                         |
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-V1     | #3324341        | ARM64_ERRATUM_3194386       |
@@ -257,6 +263,8 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | C1-Pro          | #4193714        | ARM64_ERRATUM_4193714       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | C1-Pro          | #4689344        | ARM64_ERRATUM_3821522       |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | C1-Ultra        | #3683289        | N/A                         |
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | C1-Ultra        | #4193780        | ARM64_ERRATUM_4118414       |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1075bc661833..cf50f3051edc 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1086,14 +1086,20 @@ config ARM64_ERRATUM_2645198
 	  If unsure, say Y.
 
 config ARM64_ERRATUM_3821522
-	bool "Cortex-A725: 3821522: workaround for possible CNT_CYCLES increment error due to WFE/WFI"
+	bool "Cortex-*/Neoverse-*/C1-*: workaround for possible CNT_CYCLES increment error due to WFE/WFI"
 	depends on ARM64_AMU_EXTN
 	select ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT
 	default y
 	help
-	  This option adds the workaround for ARM Cortex-A725 erratum 3821522.
+	  This option adds the workaround for the following errata:
+
+	  * ARM Cortex-A725 erratum 3821522
+	  * ARM Cortex-A720 erratum 4689339
+	  * ARM Cortex-A720AE erratum 4689340
+	  * ARM Neoverse N3 erratum 4689341
+	  * ARM C1-Pro erratum 4689344
 
-	  On affected A725 cores, the CNT_CYCLES event may incur a significant
+	  On affected cores, the CNT_CYCLES event may incur a significant
 	  increment error when entering and subsequently exiting WFx.
 	  As a result, the CNT_CYCLES may diverge from the system counter
 	  frequency at which it is expected to increment.
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 38abd123c740..b20da8b24be3 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -420,8 +420,17 @@ static const struct midr_range workaround_amu_constcnt_list[] = {
 	MIDR_RANGE(MIDR_CORTEX_A510, 0, 0, 1, 1),
 #endif
 #ifdef CONFIG_ARM64_ERRATUM_3821522
+	/* Cortex-A720 r0p0 - r0p2 */
+	MIDR_RANGE(MIDR_CORTEX_A720, 0, 0, 0, 2),
+	/* Cortex-A720AE r0p0 - r0p1 */
+	MIDR_RANGE(MIDR_CORTEX_A720AE, 0, 0, 0, 1),
 	/* Cortex-A725 r0p0 - r0p2 */
 	MIDR_RANGE(MIDR_CORTEX_A725, 0, 0, 0, 2),
+	/* Neoverse N3 r0p0 - r0p1 */
+	MIDR_RANGE(MIDR_NEOVERSE_N3, 0, 0, 0, 1),
+	/* C1-Pro r0p0 - r1p3 */
+	MIDR_RANGE(MIDR_C1_PRO, 0, 0, 1, 3),
+
 #endif
 	{}
 };
-- 
2.43.0

[PATCH 2/3] arm64: errata: Add Cortex-A715/A720 AMU core counter workaround

[Beata Michalska]

On Cortex-A715 (erratum 2738462) and Cortex-A720 (erratum 2738461),
the AMEVCNTR00 counter may continue incrementing while the core
remains in WFE with the clocks being enabled. The resulting core
cycle count may become unreliable.

Add a workaround that effectively treats the AMU counter
as unavailable.

Cc: stable@vger.kernel.org
Signed-off-by: Beata Michalska <beata.michalska@arm.com>
---
 Documentation/arch/arm64/silicon-errata.rst |  4 +++
 arch/arm64/Kconfig                          | 32 +++++++++++++++++++++
 arch/arm64/kernel/cpu_errata.c              | 20 +++++++++++++
 arch/arm64/kernel/cpufeature.c              |  3 +-
 arch/arm64/kernel/topology.c                |  3 +-
 arch/arm64/tools/cpucaps                    |  1 +
 6 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
index 09fe032e1aea..5801597a387c 100644
--- a/Documentation/arch/arm64/silicon-errata.rst
+++ b/Documentation/arch/arm64/silicon-errata.rst
@@ -171,8 +171,12 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A715     | #2645198        | ARM64_ERRATUM_2645198       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Cortex-A715     | #2738462        |  ARM64_ERRATUM_2738461      |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A715     | #3456084        | ARM64_ERRATUM_3194386       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Cortex-A720     | #2738461        | ARM64_ERRATUM_2738461       |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A720     | #3456091        | ARM64_ERRATUM_3194386       |
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A720     | #4689339        | ARM64_ERRATUM_3821522       |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index cf50f3051edc..afc6226689be 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1051,6 +1051,38 @@ config ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT
 	# making it unreliable. As a result, AMU users should treat it as
 	# unavailable rather than relying on misleading counter values.
 
+config ARM64_WORKAROUND_BROKEN_AMU_CORECNT
+	bool
+	# Targeting errata affecting AMEVCNTR00, the AMU core cycle counter.
+	# On affected CPUs the counter may continue incrementing while core
+	# remains in WFE with the clocks enabled.
+	# This may lead to readings of said counter being unreliable.
+	# How much though depends on the sampling intervals.
+
+config ARM64_ERRATUM_2738461
+	bool "Cortex-A715/Cortex-A720: workaround for AMEVCNTR00 incrementing incorrectly"
+	depends on ARM64_AMU_EXTN
+	select ARM64_WORKAROUND_BROKEN_AMU_CORECNT
+	default y
+	help
+	   This option adds the workaround for the following errata:
+
+	   * ARM Cortex-A715 erratum 2738462
+	   * ARM Cortex-A720 erratum 2738461
+
+	   On affected cores the AMEVCNTR00 counter (AMU core frequency cycles
+	   counter) may continue incrementing while the core remains in WFE
+	   with the clocks being enabled.
+	   This may result in inaccurate counter readings, with the magnitude
+	   of the discrepancy depending on the sampling interval.
+
+	   With this workaround, reads of the affected counter return 0 in
+	   the relevant paths. This causes users of the counter to treat it
+	   as unavailable and is functionally equivalent to firmware disabling
+	   the affected counter.
+
+	   If unsure, say Y.
+
 config ARM64_ERRATUM_2457168
 	bool "Cortex-A510: 2457168: workaround for AMEVCNTR01 incrementing incorrectly"
 	depends on ARM64_AMU_EXTN
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index b20da8b24be3..957917cfab70 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -436,6 +436,18 @@ static const struct midr_range workaround_amu_constcnt_list[] = {
 };
 #endif /* CONFIG_ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT */
 
+#ifdef CONFIG_ARM64_WORKAROUND_BROKEN_AMU_CORECNT
+static const struct midr_range workaround_amu_corecnt_list[] = {
+#ifdef CONFIG_ARM64_ERRATUM_2738461
+	/* Cortex-A715 r0p0 - r1p3 */
+	MIDR_RANGE(MIDR_CORTEX_A715, 0, 0, 1, 3),
+	/* Cortex-A720 r0p0 - r0p1 */
+	MIDR_RANGE(MIDR_CORTEX_A720, 0, 0, 0, 1),
+#endif
+	{}
+};
+#endif /* CONFIG_ARM64_WORKAROUND_BROKEN_AMU_CORECNT */
+
 #ifdef CONFIG_CAVIUM_ERRATUM_23154
 static const struct midr_range cavium_erratum_23154_cpus[] = {
 	MIDR_ALL_VERSIONS(MIDR_THUNDERX),
@@ -979,6 +991,14 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		CAP_MIDR_RANGE_LIST(workaround_amu_constcnt_list)
 	},
 #endif
+#ifdef CONFIG_ARM64_WORKAROUND_BROKEN_AMU_CORECNT
+	{
+		.desc = "Broken AMU AMEVCNTR00 (core cycle counter)",
+		.capability = ARM64_WORKAROUND_BROKEN_AMU_CORECNT,
+		.type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE,
+		CAP_MIDR_RANGE_LIST(workaround_amu_corecnt_list)
+	},
+#endif
 #ifdef CONFIG_ARM64_ERRATUM_2038923
 	{
 		.desc = "ARM erratum 2038923",
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 34c5c03c0c9b..0e8a9aedb84b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1938,7 +1938,8 @@ static void cpu_amu_enable(struct arm64_cpu_capabilities const *cap)
 		cpumask_set_cpu(smp_processor_id(), &amu_cpus);
 
 		/* 0 reference values signal broken/disabled counters */
-		if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT))
+		if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)
+		    && !this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT))
 			update_freq_counters_refs();
 	}
 }
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 44305b997a02..66ac67bd025e 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -389,7 +389,8 @@ static void cpu_read_corecnt(void *val)
 	 * counter read is properly handled as an error case by the users of the
 	 * counter.
 	 */
-	*(u64 *)val = read_corecnt();
+	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
+		0UL : read_corecnt();
 }
 
 static void cpu_read_constcnt(void *val)
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index 20cc185649f9..3125695122a0 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -133,3 +133,4 @@ WORKAROUND_SPECULATIVE_AT
 WORKAROUND_SPECULATIVE_SSBS
 WORKAROUND_SPECULATIVE_UNPRIV_LOAD
 WORKAROUND_BROKEN_AMU_CONSTCNT
+WORKAROUND_BROKEN_AMU_CORECNT
-- 
2.43.0

[PATCH 3/3] arm64: topology: Reduce the skew for AMU feedback counter sampling

[Beata Michalska]

With the FFH support for CPPC, the AMU feedback counters, core and const
counter, are being sampled to measure CPU performance. Checking for errata
(which can affect both counters) between the reads delays the second
sample and can skew the result.

Take both samples in succession, then discard values from affected
counters. This narrows the sampling interval used for CPPC feedback.

Signed-off-by: Beata Michalska <beata.michalska@arm.com>
---
 arch/arm64/kernel/topology.c | 76 +++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 66ac67bd025e..c968fa5b0c88 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -381,29 +381,45 @@ enum cpc_ffh_ctr_id {
 	CPC_FFH_CTR_CONST = 0x1,
 };
 
-static void cpu_read_corecnt(void *val)
+static inline void cpu_read_corecnt(void *val)
 {
-	/*
-	 * A value of 0 can be returned if the current CPU does not support AMUs
-	 * or if the counter is disabled for this CPU. A return value of 0 at
-	 * counter read is properly handled as an error case by the users of the
-	 * counter.
-	 */
-	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
-		0UL : read_corecnt();
+	*(u64 *)val = read_corecnt();
 }
 
-static void cpu_read_constcnt(void *val)
+static void cpu_try_read_corecnt(void *val)
 {
-	/*
-	 * Return 0 if the current CPU is affected by a HW erratum.
-	 * A value of 0 is also returned if the current CPU does not
-	 * support AMUs or if the counter is disabled. A return
-	 * value of 0 at counter read is properly handled as an error
-	 * case by the users of the counter.
-	 */
-	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
-		      0UL : read_constcnt();
+	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT)) {
+		cpu_read_corecnt(val);
+	} else {
+		/*
+		 * A value of 0 can be returned if the current CPU does not support AMUs
+		 * or if the counter is disabled for this CPU. A return value of 0 at
+		 * counter read is properly handled as an error case by the users of the
+		 * counter.
+		 */
+		*(u64 *)val = 0UL;
+	}
+}
+
+static inline void cpu_read_constcnt(void *val)
+{
+	*(u64 *)val = read_constcnt();
+}
+
+static void cpu_try_read_constcnt(void *val)
+{
+	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)) {
+		cpu_read_constcnt(val);
+	} else {
+		/*
+		 * Return 0 if the current CPU is affected by a HW erratum.
+		 * A value of 0 is also returned if the current CPU does not
+		 * support AMUs or if the counter is disabled. A return
+		 * value of 0 at counter read is properly handled as an error
+		 * case by the users of the counter.
+		 */
+		*(u64 *)val = 0UL;
+	}
 }
 
 static inline
@@ -461,14 +477,20 @@ static void amu_read_core_const_ctrs(void *val)
 {
 	struct amu_ffh_ctrs *ctrs = val;
 
+	cpu_read_corecnt(&ctrs->corecnt);
+	cpu_read_constcnt(&ctrs->constcnt);
+
 	/*
-	 * cpu_read_constcnt() incurs slight latency due to the
-	 * ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT check.
-	 * Read it first to minimize the sampling skew between the const
-	 * and core counters.
+	 * Return 0 if the current CPU is affected by a HW erratum.
+	 * A value of 0 is also returned if the current CPU does not
+	 * support AMUs or if the counter is disabled. A return
+	 * value of 0 at counter read is properly handled as an error
+	 * case by the users of the counter.
 	 */
-	cpu_read_constcnt(&ctrs->constcnt);
-	cpu_read_corecnt(&ctrs->corecnt);
+	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT))
+		ctrs->corecnt = 0UL;
+	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT))
+		ctrs->constcnt = 0UL;
 }
 
 static bool cpc_ffh_reg_valid(const struct cpc_reg *reg)
@@ -543,10 +565,10 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
 
 	switch ((u64)reg->address) {
 	case CPC_FFH_CTR_CORE:
-		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
+		ret = counters_read_on_cpu(cpu, cpu_try_read_corecnt, val);
 		break;
 	case CPC_FFH_CTR_CONST:
-		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
+		ret = counters_read_on_cpu(cpu, cpu_try_read_constcnt, val);
 		break;
 	}
 
-- 
2.43.0

Re: [PATCH 3/3] arm64: topology: Reduce the skew for AMU feedback counter sampling

[Christian Loehle]

On 11/11/26 12:39, Beata Michalska wrote:
> With the FFH support for CPPC, the AMU feedback counters, core and const
> counter, are being sampled to measure CPU performance. Checking for errata
> (which can affect both counters) between the reads delays the second
> sample and can skew the result.
> 
> Take both samples in succession, then discard values from affected
> counters. This narrows the sampling interval used for CPPC feedback.
> 
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> ---
>  arch/arm64/kernel/topology.c | 76 +++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 66ac67bd025e..c968fa5b0c88 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -381,29 +381,45 @@ enum cpc_ffh_ctr_id {
>  	CPC_FFH_CTR_CONST = 0x1,
>  };
>  
> -static void cpu_read_corecnt(void *val)
> +static inline void cpu_read_corecnt(void *val)
>  {
> -	/*
> -	 * A value of 0 can be returned if the current CPU does not support AMUs
> -	 * or if the counter is disabled for this CPU. A return value of 0 at
> -	 * counter read is properly handled as an error case by the users of the
> -	 * counter.
> -	 */
> -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
> -		0UL : read_corecnt();
> +	*(u64 *)val = read_corecnt();
>  }
>  
> -static void cpu_read_constcnt(void *val)
> +static void cpu_try_read_corecnt(void *val)
>  {
> -	/*
> -	 * Return 0 if the current CPU is affected by a HW erratum.
> -	 * A value of 0 is also returned if the current CPU does not
> -	 * support AMUs or if the counter is disabled. A return
> -	 * value of 0 at counter read is properly handled as an error
> -	 * case by the users of the counter.
> -	 */
> -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
> -		      0UL : read_constcnt();
> +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT)) {
> +		cpu_read_corecnt(val);
> +	} else {
> +		/*
> +		 * A value of 0 can be returned if the current CPU does not support AMUs
> +		 * or if the counter is disabled for this CPU. A return value of 0 at
> +		 * counter read is properly handled as an error case by the users of the
> +		 * counter.
> +		 */
> +		*(u64 *)val = 0UL;
> +	}
> +}
> +
> +static inline void cpu_read_constcnt(void *val)
> +{
> +	*(u64 *)val = read_constcnt();
> +}
> +
> +static void cpu_try_read_constcnt(void *val)
> +{
> +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)) {
> +		cpu_read_constcnt(val);
> +	} else {
> +		/*
> +		 * Return 0 if the current CPU is affected by a HW erratum.
> +		 * A value of 0 is also returned if the current CPU does not
> +		 * support AMUs or if the counter is disabled. A return
> +		 * value of 0 at counter read is properly handled as an error
> +		 * case by the users of the counter.
> +		 */
> +		*(u64 *)val = 0UL;
> +	}
>  }
>  
>  static inline
> @@ -461,14 +477,20 @@ static void amu_read_core_const_ctrs(void *val)
>  {
>  	struct amu_ffh_ctrs *ctrs = val;
>  
> +	cpu_read_corecnt(&ctrs->corecnt);
> +	cpu_read_constcnt(&ctrs->constcnt);
> +
>  	/*
> -	 * cpu_read_constcnt() incurs slight latency due to the
> -	 * ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT check.
> -	 * Read it first to minimize the sampling skew between the const
> -	 * and core counters.
> +	 * Return 0 if the current CPU is affected by a HW erratum.
> +	 * A value of 0 is also returned if the current CPU does not
> +	 * support AMUs or if the counter is disabled. A return
> +	 * value of 0 at counter read is properly handled as an error
> +	 * case by the users of the counter.

Why do we read them first then on 
ARM64_WORKAROUND_BROKEN_AMU_CORECNT or ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT
respectively?

>  	 */
> -	cpu_read_constcnt(&ctrs->constcnt);
> -	cpu_read_corecnt(&ctrs->corecnt);
> +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT))
> +		ctrs->corecnt = 0UL;
> +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT))
> +		ctrs->constcnt = 0UL;
>  }
>  
>  static bool cpc_ffh_reg_valid(const struct cpc_reg *reg)
> @@ -543,10 +565,10 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
>  
>  	switch ((u64)reg->address) {
>  	case CPC_FFH_CTR_CORE:
> -		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
> +		ret = counters_read_on_cpu(cpu, cpu_try_read_corecnt, val);
>  		break;
>  	case CPC_FFH_CTR_CONST:
> -		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
> +		ret = counters_read_on_cpu(cpu, cpu_try_read_constcnt, val);
>  		break;
>  	}
>  

Re: [PATCH 3/3] arm64: topology: Reduce the skew for AMU feedback counter sampling

[Christian Loehle]

On 11/11/26 12:39, Beata Michalska wrote:
> With the FFH support for CPPC, the AMU feedback counters, core and const
> counter, are being sampled to measure CPU performance. Checking for errata
> (which can affect both counters) between the reads delays the second
> sample and can skew the result.
> 
> Take both samples in succession, then discard values from affected
> counters. This narrows the sampling interval used for CPPC feedback.
> 
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> ---
>  arch/arm64/kernel/topology.c | 76 +++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 66ac67bd025e..c968fa5b0c88 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -381,29 +381,45 @@ enum cpc_ffh_ctr_id {
>  	CPC_FFH_CTR_CONST = 0x1,
>  };
>  
> -static void cpu_read_corecnt(void *val)
> +static inline void cpu_read_corecnt(void *val)
>  {
> -	/*
> -	 * A value of 0 can be returned if the current CPU does not support AMUs
> -	 * or if the counter is disabled for this CPU. A return value of 0 at
> -	 * counter read is properly handled as an error case by the users of the
> -	 * counter.
> -	 */
> -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
> -		0UL : read_corecnt();
> +	*(u64 *)val = read_corecnt();
>  }
>  
> -static void cpu_read_constcnt(void *val)
> +static void cpu_try_read_corecnt(void *val)
>  {
> -	/*
> -	 * Return 0 if the current CPU is affected by a HW erratum.
> -	 * A value of 0 is also returned if the current CPU does not
> -	 * support AMUs or if the counter is disabled. A return
> -	 * value of 0 at counter read is properly handled as an error
> -	 * case by the users of the counter.

Since you're moving this comment around, AFAICT it's not actually true?
cpu_has_amu_feat() is checked (which will return -EOPNOTSUPP) before
landing here, right?


> -	 */
> -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
> -		      0UL : read_constcnt();
> +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT)) {
> +		cpu_read_corecnt(val);
> +	} else {
> +		/*
> +		 * A value of 0 can be returned if the current CPU does not support AMUs
> +		 * or if the counter is disabled for this CPU. A return value of 0 at
> +		 * counter read is properly handled as an error case by the users of the
> +		 * counter.
> +		 */
> +		*(u64 *)val = 0UL;
> +	}
> +}
> +
> +static inline void cpu_read_constcnt(void *val)
> +{
> +	*(u64 *)val = read_constcnt();
> +}
> +
> +static void cpu_try_read_constcnt(void *val)
> +{
> +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)) {
> +		cpu_read_constcnt(val);
> +	} else {
> +		/*
> +		 * Return 0 if the current CPU is affected by a HW erratum.
> +		 * A value of 0 is also returned if the current CPU does not
> +		 * support AMUs or if the counter is disabled. A return
> +		 * value of 0 at counter read is properly handled as an error
> +		 * case by the users of the counter.
> +		 */
> +		*(u64 *)val = 0UL;
> +	}
>  }
>  
>  static inline
> @@ -461,14 +477,20 @@ static void amu_read_core_const_ctrs(void *val)
>  {
>  	struct amu_ffh_ctrs *ctrs = val;
>  
> +	cpu_read_corecnt(&ctrs->corecnt);
> +	cpu_read_constcnt(&ctrs->constcnt);
> +
>  	/*
> -	 * cpu_read_constcnt() incurs slight latency due to the
> -	 * ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT check.
> -	 * Read it first to minimize the sampling skew between the const
> -	 * and core counters.
> +	 * Return 0 if the current CPU is affected by a HW erratum.
> +	 * A value of 0 is also returned if the current CPU does not
> +	 * support AMUs or if the counter is disabled. A return
> +	 * value of 0 at counter read is properly handled as an error
> +	 * case by the users of the counter.
>  	 */
> -	cpu_read_constcnt(&ctrs->constcnt);
> -	cpu_read_corecnt(&ctrs->corecnt);
> +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT))
> +		ctrs->corecnt = 0UL;
> +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT))
> +		ctrs->constcnt = 0UL;
>  }
>  
>  static bool cpc_ffh_reg_valid(const struct cpc_reg *reg)
> @@ -543,10 +565,10 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
>  
>  	switch ((u64)reg->address) {
>  	case CPC_FFH_CTR_CORE:
> -		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
> +		ret = counters_read_on_cpu(cpu, cpu_try_read_corecnt, val);
>  		break;
>  	case CPC_FFH_CTR_CONST:
> -		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
> +		ret = counters_read_on_cpu(cpu, cpu_try_read_constcnt, val);
>  		break;
>  	}
>  

Re: [PATCH 3/3] arm64: topology: Reduce the skew for AMU feedback counter sampling

[Beata Michalska]

Hi Christian,
On Wed, Nov 11, 2026 at 01:56:49PM +0000, Christian Loehle wrote:
> On 11/11/26 12:39, Beata Michalska wrote:
> > With the FFH support for CPPC, the AMU feedback counters, core and const
> > counter, are being sampled to measure CPU performance. Checking for errata
> > (which can affect both counters) between the reads delays the second
> > sample and can skew the result.
> > 
> > Take both samples in succession, then discard values from affected
> > counters. This narrows the sampling interval used for CPPC feedback.
> > 
> > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> > ---
> >  arch/arm64/kernel/topology.c | 76 +++++++++++++++++++++++-------------
> >  1 file changed, 49 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 66ac67bd025e..c968fa5b0c88 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -381,29 +381,45 @@ enum cpc_ffh_ctr_id {
> >  	CPC_FFH_CTR_CONST = 0x1,
> >  };
> >  
> > -static void cpu_read_corecnt(void *val)
> > +static inline void cpu_read_corecnt(void *val)
> >  {
> > -	/*
> > -	 * A value of 0 can be returned if the current CPU does not support AMUs
> > -	 * or if the counter is disabled for this CPU. A return value of 0 at
> > -	 * counter read is properly handled as an error case by the users of the
> > -	 * counter.
> > -	 */
> > -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
> > -		0UL : read_corecnt();
> > +	*(u64 *)val = read_corecnt();
> >  }
> >  
> > -static void cpu_read_constcnt(void *val)
> > +static void cpu_try_read_corecnt(void *val)
> >  {
> > -	/*
> > -	 * Return 0 if the current CPU is affected by a HW erratum.
> > -	 * A value of 0 is also returned if the current CPU does not
> > -	 * support AMUs or if the counter is disabled. A return
> > -	 * value of 0 at counter read is properly handled as an error
> > -	 * case by the users of the counter.
> > -	 */
> > -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
> > -		      0UL : read_constcnt();
> > +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT)) {
> > +		cpu_read_corecnt(val);
> > +	} else {
> > +		/*
> > +		 * A value of 0 can be returned if the current CPU does not support AMUs
> > +		 * or if the counter is disabled for this CPU. A return value of 0 at
> > +		 * counter read is properly handled as an error case by the users of the
> > +		 * counter.
> > +		 */
> > +		*(u64 *)val = 0UL;
> > +	}
> > +}
> > +
> > +static inline void cpu_read_constcnt(void *val)
> > +{
> > +	*(u64 *)val = read_constcnt();
> > +}
> > +
> > +static void cpu_try_read_constcnt(void *val)
> > +{
> > +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)) {
> > +		cpu_read_constcnt(val);
> > +	} else {
> > +		/*
> > +		 * Return 0 if the current CPU is affected by a HW erratum.
> > +		 * A value of 0 is also returned if the current CPU does not
> > +		 * support AMUs or if the counter is disabled. A return
> > +		 * value of 0 at counter read is properly handled as an error
> > +		 * case by the users of the counter.
> > +		 */
> > +		*(u64 *)val = 0UL;
> > +	}
> >  }
> >  
> >  static inline
> > @@ -461,14 +477,20 @@ static void amu_read_core_const_ctrs(void *val)
> >  {
> >  	struct amu_ffh_ctrs *ctrs = val;
> >  
> > +	cpu_read_corecnt(&ctrs->corecnt);
> > +	cpu_read_constcnt(&ctrs->constcnt);
> > +
> >  	/*
> > -	 * cpu_read_constcnt() incurs slight latency due to the
> > -	 * ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT check.
> > -	 * Read it first to minimize the sampling skew between the const
> > -	 * and core counters.
> > +	 * Return 0 if the current CPU is affected by a HW erratum.
> > +	 * A value of 0 is also returned if the current CPU does not
> > +	 * support AMUs or if the counter is disabled. A return
> > +	 * value of 0 at counter read is properly handled as an error
> > +	 * case by the users of the counter.
> 
> Why do we read them first then on 
> ARM64_WORKAROUND_BROKEN_AMU_CORECNT or ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT
> respectively?
>
That's what this patch is all about - isn't it ?
It allows direct read of both counters (those being read first),
and only once that's done, it verifies whether the workaround should be applied
or not. For cores that do not have either it eliminates the check between
reading the counters.
Or you had smth else in mind I did not quite manage to grasp ?

---
BR
Beata
> >  	 */
> > -	cpu_read_constcnt(&ctrs->constcnt);
> > -	cpu_read_corecnt(&ctrs->corecnt);
> > +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT))
> > +		ctrs->corecnt = 0UL;
> > +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT))
> > +		ctrs->constcnt = 0UL;
> >  }
> >  
> >  static bool cpc_ffh_reg_valid(const struct cpc_reg *reg)
> > @@ -543,10 +565,10 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> >  
> >  	switch ((u64)reg->address) {
> >  	case CPC_FFH_CTR_CORE:
> > -		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
> > +		ret = counters_read_on_cpu(cpu, cpu_try_read_corecnt, val);
> >  		break;
> >  	case CPC_FFH_CTR_CONST:
> > -		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
> > +		ret = counters_read_on_cpu(cpu, cpu_try_read_constcnt, val);
> >  		break;
> >  	}
> >  
> 

Re: [PATCH 3/3] arm64: topology: Reduce the skew for AMU feedback counter sampling

[Beata Michalska]

On Wed, Nov 11, 2026 at 02:08:11PM +0000, Christian Loehle wrote:
> On 11/11/26 12:39, Beata Michalska wrote:
> > With the FFH support for CPPC, the AMU feedback counters, core and const
> > counter, are being sampled to measure CPU performance. Checking for errata
> > (which can affect both counters) between the reads delays the second
> > sample and can skew the result.
> > 
> > Take both samples in succession, then discard values from affected
> > counters. This narrows the sampling interval used for CPPC feedback.
> > 
> > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> > ---
> >  arch/arm64/kernel/topology.c | 76 +++++++++++++++++++++++-------------
> >  1 file changed, 49 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 66ac67bd025e..c968fa5b0c88 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -381,29 +381,45 @@ enum cpc_ffh_ctr_id {
> >  	CPC_FFH_CTR_CONST = 0x1,
> >  };
> >  
> > -static void cpu_read_corecnt(void *val)
> > +static inline void cpu_read_corecnt(void *val)
> >  {
> > -	/*
> > -	 * A value of 0 can be returned if the current CPU does not support AMUs
> > -	 * or if the counter is disabled for this CPU. A return value of 0 at
> > -	 * counter read is properly handled as an error case by the users of the
> > -	 * counter.
> > -	 */
> > -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT) ?
> > -		0UL : read_corecnt();
> > +	*(u64 *)val = read_corecnt();
> >  }
> >  
> > -static void cpu_read_constcnt(void *val)
> > +static void cpu_try_read_corecnt(void *val)
> >  {
> > -	/*
> > -	 * Return 0 if the current CPU is affected by a HW erratum.
> > -	 * A value of 0 is also returned if the current CPU does not
> > -	 * support AMUs or if the counter is disabled. A return
> > -	 * value of 0 at counter read is properly handled as an error
> > -	 * case by the users of the counter.
> 
> Since you're moving this comment around, AFAICT it's not actually true?
> cpu_has_amu_feat() is checked (which will return -EOPNOTSUPP) before
> landing here, right?
>
Not entirely, no.
`cpu_has_amu_feat()` verifies the support for AMUs, the extension itself
(ID_AA64PFR0_EL1). Workarounds are applied on top of it.

---
BR
Beata
> 
> > -	 */
> > -	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT) ?
> > -		      0UL : read_constcnt();
> > +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT)) {
> > +		cpu_read_corecnt(val);
> > +	} else {
> > +		/*
> > +		 * A value of 0 can be returned if the current CPU does not support AMUs
> > +		 * or if the counter is disabled for this CPU. A return value of 0 at
> > +		 * counter read is properly handled as an error case by the users of the
> > +		 * counter.
> > +		 */
> > +		*(u64 *)val = 0UL;
> > +	}
> > +}
> > +
> > +static inline void cpu_read_constcnt(void *val)
> > +{
> > +	*(u64 *)val = read_constcnt();
> > +}
> > +
> > +static void cpu_try_read_constcnt(void *val)
> > +{
> > +	if (!this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT)) {
> > +		cpu_read_constcnt(val);
> > +	} else {
> > +		/*
> > +		 * Return 0 if the current CPU is affected by a HW erratum.
> > +		 * A value of 0 is also returned if the current CPU does not
> > +		 * support AMUs or if the counter is disabled. A return
> > +		 * value of 0 at counter read is properly handled as an error
> > +		 * case by the users of the counter.
> > +		 */
> > +		*(u64 *)val = 0UL;
> > +	}
> >  }
> >  
> >  static inline
> > @@ -461,14 +477,20 @@ static void amu_read_core_const_ctrs(void *val)
> >  {
> >  	struct amu_ffh_ctrs *ctrs = val;
> >  
> > +	cpu_read_corecnt(&ctrs->corecnt);
> > +	cpu_read_constcnt(&ctrs->constcnt);
> > +
> >  	/*
> > -	 * cpu_read_constcnt() incurs slight latency due to the
> > -	 * ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT check.
> > -	 * Read it first to minimize the sampling skew between the const
> > -	 * and core counters.
> > +	 * Return 0 if the current CPU is affected by a HW erratum.
> > +	 * A value of 0 is also returned if the current CPU does not
> > +	 * support AMUs or if the counter is disabled. A return
> > +	 * value of 0 at counter read is properly handled as an error
> > +	 * case by the users of the counter.
> >  	 */
> > -	cpu_read_constcnt(&ctrs->constcnt);
> > -	cpu_read_corecnt(&ctrs->corecnt);
> > +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CORECNT))
> > +		ctrs->corecnt = 0UL;
> > +	if (this_cpu_has_cap(ARM64_WORKAROUND_BROKEN_AMU_CONSTCNT))
> > +		ctrs->constcnt = 0UL;
> >  }
> >  
> >  static bool cpc_ffh_reg_valid(const struct cpc_reg *reg)
> > @@ -543,10 +565,10 @@ int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> >  
> >  	switch ((u64)reg->address) {
> >  	case CPC_FFH_CTR_CORE:
> > -		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
> > +		ret = counters_read_on_cpu(cpu, cpu_try_read_corecnt, val);
> >  		break;
> >  	case CPC_FFH_CTR_CONST:
> > -		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
> > +		ret = counters_read_on_cpu(cpu, cpu_try_read_constcnt, val);
> >  		break;
> >  	}
> >  
>