Re: [RESEND PATCH v2] arm64: errata: add detection for AMEVCNTR01 incrementing incorrectly

[Catalin Marinas]

On Thu, Aug 18, 2022 at 01:03:51PM +0100, Ionela Voinescu wrote:
> On Wednesday 17 Aug 2022 at 17:59:01 (+0100), Catalin Marinas wrote:
> > On Wed, Aug 17, 2022 at 01:15:51PM +0100, Ionela Voinescu wrote:
> > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > > index 869ffc4d4484..5d7efb15f7cf 100644
> > > --- a/arch/arm64/kernel/topology.c
> > > +++ b/arch/arm64/kernel/topology.c
> > > @@ -301,7 +301,8 @@ static void cpu_read_corecnt(void *val)
> > >  
> > >  static void cpu_read_constcnt(void *val)
> > >  {
> > > -	*(u64 *)val = read_constcnt();
> > > +	*(u64 *)val = this_cpu_has_cap(ARM64_WORKAROUND_2457168) ?
> > > +		      0UL : read_constcnt();
> > >  }
> > >  
> > >  static inline
> > > @@ -328,7 +329,12 @@ int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
> > >   */
> > >  bool cpc_ffh_supported(void)
> > >  {
> > > -	return freq_counters_valid(get_cpu_with_amu_feat());
> > > +	int cpu = get_cpu_with_amu_feat();
> > > +
> > > +	if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask))
> > > +		return false;
> > > +
> > > +	return true;
> > >  }
> > 
> > So here we tell the core code that FFH is supported but always return 0
> > via cpc_read_ffh() if the const counter is requested. I assume the core
> > code figures this out and doesn't use the value on the affected CPUs. I
> > was hoping cpc_ffh_supported() would be per-CPU and the core code simply
> > skips calling cpc_read() on the broken cores.
> 
> I used to think the same, but I've realised that the current approach is
> best, in my opinion.
> 
> There are two users of these counters exposed though FFH in the kernel:
> CPPC-based frequency invariance(FIE) and reading current frequency through
> sysfs. If AMU counters are disabled or the CPU is affected by this
> erratum, a single read of 0 for any of the counters will result in
> cppc_get_perf_ctrs() returning -EFAULT which:
> 
> - (cppc_cpufreq_cpu_fie_init()) Will disable the use of FIE for that
>   policy, and those counters will never be read again for that CPU, for
>   the purpose of FIE. This is the operation that would result in reading
>   those counters most often, which in this case won't happen.
> 
> - Will return -EFAULT from cppc_cpufreq_get_rate() signaling to the user
>   that it cannot return a proper frequency using those counters. That's
>   cast to unsigned int so the user would have to be knowledgeable on the
>   matter :), but that's an existing problem.
> 
> Therefore, error checking based on a counter read of 0 would be
> equivalent here to checking a potential ffh_supported(cpu). Also, in the
> future we might use FFH to not only read these counters. So it's better
> to keep ffh_supported() to just reflect whether generically FFH is
> supported, even if in some cases the "backend" (AMUs here) is disabled
> or broken.

This works for me as long as the callers are aware of what a return of 0
when reading the counter means.

> > Is the other register read by cpc_read_ffh() still useful without the
> > const one?
> 
> Not for the current uses, and unlikely to be in the future - I don't see
> how the core counter value can be useful without a constant reference.

I was thinking of return 0 directly from cpc_read_ffh() since the other
counter is not used independently but I guess your approach matches the
erratum better since it's only the const counter that's broken.

> > While the Kconfig entry describes the behaviour, I'd rather have a
> > comment in cpc_ffh_supported() and maybe cpu_read_constcnt() on why we
> > do these tricks.
> 
> Will do!

Thanks. With comments added, feel free to add:

Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx>