Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

[Suzuki K Poulose]

On 17/01/18 12:25, Dave Martin wrote:
> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
> > When a CPU is brought up after we have finalised the system
> > wide capabilities (i.e, features and errata), we make sure the
> > new CPU doesn't need a new errata work around which has not been
> > detected already. However we don't run enable() method on the new
> > CPU for the errata work arounds already detected. This could
> > cause the new CPU running without potential work arounds.
> > It is upto the "enable()" method to decide if this CPU should
> > do something about the errata.
> >
> > Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work arounds on hotplugged CPU")
> > Cc: Will Deacon <will.deacon@xxxxxxx>
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > Cc: Andre Przywara <andre.przywara@xxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Dave Martin <dave.martin@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---
> >   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> > index 90a9e465339c..54e41dfe41f6 100644
> > --- a/arch/arm64/kernel/cpu_errata.c
> > +++ b/arch/arm64/kernel/cpu_errata.c
> > @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
> >   {
> >   	const struct arm64_cpu_capabilities *caps = arm64_errata;
> >   
> > -	for (; caps->matches; caps++)
> > -		if (!cpus_have_cap(caps->capability) &&
> > -			caps->matches(caps, SCOPE_LOCAL_CPU)) {
> > +	for (; caps->matches; caps++) {
> > +		if (cpus_have_cap(caps->capability)) {
> > +			if (caps->enable)
> > +				caps->enable((void *)caps);
>
> Do we really need this cast?

Yes, otherwise we would be passing a "const *" where a "void *" is expected,
and the compiler warns. Or we could simply change the prototype of the
enable() method to accept a const capability ptr.

> Can enable() fail, or do we already guarantee that it succeeds (by
> having detected the cap in the first place)?

enable() can't fail, since as you said the cap is already detected
by the scope of the capability. It is just the matter of enable() deciding
to do some action on the calling CPU depending on the type of the
work around (e.g, enable SCTLR bit or enable trapping etc). It is left
to the capability to decide whether the calling CPU needs any action
or not (e.g, bp hardening).

Cheers
Suzuki

> > +		} else if (caps->matches(caps, SCOPE_LOCAL_CPU)) {
>
> [...]
>
> Cheers
> ---Dave