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

[Suzuki K Poulose <Suzuki.Poulose@arm.com>]

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.

> What about late cpus?
> 

We don't detect any new capabilities on them. They continue to get
verified against the enabled capabilities.

>>   1) with SCOPE_LOCAL_CPU filter on each boot time enabeld CPUs,
>>      via update_cpu_local_capabilities().
> 
> "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.

OK


>>   /*
>> - * The CPU Errata work arounds are detected and applied at boot time
>> - * and the related information is freed soon after. If the new CPU requires
>> - * an errata not detected at boot, fail this CPU.
>> + * Check for all capablities within the scope of local CPU.
>> + * This is run on all boot time activated CPUs.
>>    */
>> -static void verify_local_cpu_errata_workarounds(void)
>> +static void update_cpu_local_capabilities(void)
> 
> [Gaah, stupid git diff making function insertion look like function
> modification.  Sometimes --patience does a better job, but there seems
> no foolproof solution...  If you do a respin, it might be worth trying
> it.]

Will try, thanks for the suggestion. I didn't know about that :-)

>> -static void __init setup_feature_capabilities(void)
>> +static void __init setup_system_capabilities(void)
>>   {
>> -	update_cpu_capabilities(arm64_features,
>> -				ARM64_CPUCAP_TYPE_ALL, "detected feature:");
>> -	enable_cpu_capabilities(arm64_features, ARM64_CPUCAP_TYPE_ALL);
>> +	/*
>> +	 * We have finalised the system wide safe feature registers,
>> +	 * finalise the capabilities that depend on it.
>> +	 */
>> +	update_system_capabilities();
>> +	/* Enable all the available capabilities */
>> +	enable_cpu_capabilities(ARM64_CPUCAP_TYPE_ALL);
> 
> So setup_system_capabilities() enables _non_ system-wide capabilities/
> errata workarounds too?

> Maybe this function should just have a different name, like
> "setup_boot_capabilities" or similar?

The problem with setup_boot_capabilities() is that it could conflict with
"coming soon" setup_boot_cpu_capabilities(). May be,

setup_boot_time_system_capabilities().


> 
>    }
>>   
>>   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).

Suzuki