lkml.org 
[lkml]   [2022]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RESEND PATCH v2] arm64: errata: add detection for AMEVCNTR01 incrementing incorrectly
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@arm.com>

\
 
 \ /
  Last update: 2022-09-17 16:27    [W:0.042 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site