Re: [PATCH 08/16] arm64: capabilities: Group handling of features and errata

[Dave Martin]

On Mon, Jan 29, 2018 at 05:22:26PM +0000, Suzuki K Poulose wrote:
> On 29/01/18 17:14, Dave Martin wrote:
> >On Fri, Jan 26, 2018 at 12:31:18PM +0000, Suzuki K Poulose wrote:
> >>On 26/01/18 11:47, Dave Martin wrote:
> >>>On Tue, Jan 23, 2018 at 12:28:01PM +0000, Suzuki K Poulose wrote:
> >>>>So far we have had separate routes for triggering errata and feature
> >>>
> >>>"triggering errata" ? ;)
> >>>
> >>
> >>:-). Should have been "triggering errata and feature capability *checks*.
> >>
> >>>Maybe "[...] for determining whether to activate errata workarounds and
> >>>whether to enable feature capabilities."
> >>>
> >>
> >>
> >>>>capabilities. Also, we never allowed "features" based on local CPU
> >>>>and "errata" based on System wide safe registers. This patch
> >>>>groups the handling of errata and features and also allows them
> >>>>to have all the possible scopes.
> >>>>
> >>>>So, we now run through the arm64_features and arm64_errata:
> >>>
> >>>when?
> >>
> >>with this patch.
> >
> >I mean, when at runtime?
> 
> Sorry, I thought that was evident from the comment below :

Sort of, but you talk about things being done on boot-time enabled CPUs,
rather than saying the check actually happens at boot time.  The
description isn't wrong, but I just ended up a bit unsure about exactly
what was being guaranteed.

My confusion was partly due to my understanding of the code still being
incomplete when I responsed to this patch.


Maybe say "we run through arm64_features and arm64_errata in two phases
during the boot process"

> 
> >
> >>>What about late cpus?
> >>>
> >>
> >>We don't detect any new capabilities on them. They continue to get
> >>verified against the enabled capabilities.

If this patch doesn't intentionally change what happens for late CPUs
after kernel boot, but nonetheless does unifiy the errata/feature
verification code for late CPUs then this may also be worth noting here.

> >>
> >>>>  1) with SCOPE_LOCAL_CPU filter on each boot time enabeld CPUs,
> >>>>     via update_cpu_local_capabilities().
> 
> Here ^^.  Earlier we ran only through the errata list. But now, we
> run through errata and the features, using a type filter of SCOPE_LOCAL_CPU.
> 
> It also said :
> 
>   2) with SCOPE_SYSTEM filter only once after all boot time enabled
>     CPUs are active.
> 
> (2) happens from setup_cpu_features(), just like it was done earlier, but
> with a filter of SCOPE_SYSTEM.
> 
> >>>
> >>>"each [...] enabeld CPUs" -> "each [...] enabled CPU"
> >>>
> >>>Also, changing "boot time" -> "boot-time" helps avoid this being misread
> >>>as "on each boot", which could be taken to mean "each time a CPU comes
> >>>online".  I'm guessing that's not the intended meaning here.
> >>
> 
> >>>
> >>>   }
> >>>>  DEFINE_STATIC_KEY_FALSE(arm64_const_caps_ready);
> >>>>@@ -1422,9 +1435,7 @@ void __init setup_cpu_features(void)
> >>>>  	u32 cwg;
> >>>>  	int cls;
> >>>>-	/* Set the CPU feature capabilies */
> >>>>-	setup_feature_capabilities();
> >>>>-	enable_errata_workarounds();
> >>>>+	setup_system_capabilities();
> >>>>  	mark_const_caps_ready();
> >>>>  	setup_elf_hwcaps(arm64_elf_hwcaps);
> >>>
> >>>I wonder whether we could unify the elf hwcaps handling too.
> >>
> >>I was thinking about it today. The only catch is how do we know
> >>if we have "the capability", as it is spread across multiple bitmasks.
> >>(HWCAP, COMPAT_HWCAP, COMPAT_HWCAP2).
> >
> >An easy-ish solution might be to maintain our own bitmap in the style
> >of cpu_hwcaps, and set bits in parallel with the elf_hwcap etc. bits.
> >Or, add a method that knows how to set/query the appropriate bit.
> >
> >I guess we could do this later.  It's certainly not urgent.
> 
> Yes, I understand.

OK, cool

Cheers
---Dave