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

[Beata Michalska <beata.michalska@arm.com>]

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;
> >  	}
> >  
>