[PATCH] arm64: errata: Fix handling of 1418040 with late CPU onlining

[Will Deacon]

On Fri, Nov 06, 2020 at 12:18:32PM +0000, Suzuki K Poulose wrote:
> On 11/6/20 11:49 AM, Will Deacon wrote:
> > In a surprising turn of events, it transpires that CPU capabilities
> > configured as ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE are never set as the
> > result of late-onlining. Therefore our handling of erratum 1418040 does
> > not get activated if it is not required by any of the boot CPUs, even
> > though we allow late-onlining of an affected CPU.
> 
> The capability state is not altered after the SMP boot for all types
> of caps. The weak caps are there to allow a late CPU to turn online
> without getting "banned". This may be something we could relax with
> a new flag in the scope.

I did look briefly into that, but it's really difficult to enable the
static key as this operation can block so you end up having to defer it
past the point of signalling the completion back to the CPU doing cpu_up().

So I figured I'd focus on fixing the current problem for now.

> > In order to get things working again, replace the cpus_have_const_cap()
> > invocation with an explicit check for the current CPU using
> > this_cpu_has_cap().
> > 
> > Cc: Marc Zyngier <maz at kernel.org>
> > Cc: Sai Prakash Ranjan <saiprakash.ranjan at codeaurora.org>
> > Cc: Stephen Boyd <swboyd at chromium.org>
> > Cc: Catalin Marinas <catalin.marinas at arm.com>
> > Cc: Mark Rutland <mark.rutland at arm.com>
> > Cc: Suzuki Poulose <suzuki.poulose at arm.com>
> > Signed-off-by: Will Deacon <will at kernel.org>
> > ---
> > 
> > Found by code inspection and compile-tested only, so I would really
> > appreciate a second look.
> > 
> >   arch/arm64/include/asm/cpufeature.h | 2 ++
> >   arch/arm64/kernel/process.c         | 5 ++---
> >   2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > index f7e7144af174..c59c16a6ea8b 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -268,6 +268,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
> >   /*
> >    * CPU feature detected at boot time based on feature of one or more CPUs.
> >    * All possible conflicts for a late CPU are ignored.
> > + * NOTE: this means that a late CPU with the feature will *not* cause the
> > + * capability to be advertised by cpus_have_*cap()!
> 
> This comment applies to all the types, so it may be confusing. And the
> comment already mentions that the feature is detected at boot time.

The other types don't allow a late CPU to come online with the feature
though, so I don't think it's quite the same. Given that we made this
mistake for this erratum workaround and previously for SSBS, I wanted to add
something to the comment to try to avoid others falling into this trap.

Ideally, we'd warn when calling cpus_have_*cap() for one of these things,
but that adds runtime cost that we'd probably have to gate behind a DEBUG
option.

> >    */
> >   #define ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE		\
> >   	(ARM64_CPUCAP_SCOPE_LOCAL_CPU		|	\
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 4784011cecac..a47a40ec6ad9 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -522,14 +522,13 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,
> >   	bool prev32, next32;
> >   	u64 val;
> > -	if (!(IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) &&
> > -	      cpus_have_const_cap(ARM64_WORKAROUND_1418040)))
> > +	if (!IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040))
> >   		return;
> >   	prev32 = is_compat_thread(task_thread_info(prev));
> >   	next32 = is_compat_thread(task_thread_info(next));
> > -	if (prev32 == next32)
> > +	if (prev32 == next32 || !this_cpu_has_cap(ARM64_WORKAROUND_1418040))
> >   		return;
> >   	val = read_sysreg(cntkctl_el1);
> > 
> 
> 
> This change as such looks good to me.
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose at arm.com>

Thanks!

Will