[PATCH v3 19/22] arm64: capabilities: Handle shared entries

[Suzuki K Poulose]

On 12/02/18 17:17, Dave Martin wrote:
> On Fri, Feb 09, 2018 at 05:55:08PM +0000, Suzuki K Poulose wrote:
>> Some capabilities have different criteria for detection and associated
>> actions based on the matching criteria, even though they all share the
>> same capability bit. So far we have used multiple entries with the same
>> capability bit to handle this. This is prone to errors, as the
>> cpu_enable is invoked for each entry, irrespective of whether the
>> detection rule applies to the CPU or not. And also this complicates
>> other helpers, e.g, __this_cpu_has_cap.
>>
>> This patch adds a wrapper entry to cover all the possible variations
>> of a capability by maintaining list of matches + cpu_enable callbacks.
>> To avoid complicating the prototypes for the "matches()", we use
>> arm64_cpu_capabilities maintain the list and we ignore all the other
>> fields except the matches & cpu_enable.
>>
>> This ensures :
>>
>>   1) The capabilitiy is set when at least one of the entry detects
>>   2) Action is only taken for the entries that "matches".
>>
>> This avoids explicit checks in the cpu_enable() take some action.
>> The only constraint here is that, all the entries should have the
>> same "type" (i.e, scope and conflict rules).
>>
>> If a cpu_enable() method is associated with multiple matches for a
>> single capability, care should be taken that either the match criteria
>> are mutually exclusive, or that the method is robust against being
>> called multiple times.
>>
>> This also reverts the changes introduced by commit 67948af41f2e6818ed
>> ("arm64: capabilities: Handle duplicate entries for a capability").
>>
>> Cc: Dave Martin <dave.martin at arm.com>
>> Cc: Robin Murphy <robin.murphy at arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose at arm.com>
>> ---
>>   arch/arm64/include/asm/cpufeature.h | 11 ++++++++
>>   arch/arm64/kernel/cpu_errata.c      | 53 ++++++++++++++++++++++++++++++++-----
>>   arch/arm64/kernel/cpufeature.c      | 10 +++----
>>   3 files changed, 61 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 3ab1c3422f14..074537acc08b 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -306,6 +306,17 @@ struct arm64_cpu_capabilities {
>>   			bool sign;
>>   			unsigned long hwcap;
>>   		};
>> +		/*
>> +		 * A list of "matches/cpu_enable" pair for the same "capability"
>> +		 * of the same "type" as described by the parent. All the
>> +		 * fields, except "matches"/"cpu_enable" are ignored in the list.
> 
> Nit: This is not quite true: other fields may be needed, for use by the
> matches() method -- for example, if matches == has_cpuid_feature, then
> sys_reg, field_pos etc. will be used.
> 
> To keep things simple, maybe say "Only matches(), cpu_enable() and
> fields relevant to these methods are significant in the list." ?

Will fix it.

> 
>> +		 * The cpu_enable is invoked only if the corresponding entry
>> +		 * "matches()". However, if a cpu_enable() method is associated
>> +		 * with multiple matches, care should be taken that either the
>> +		 * match criteria are mutually exclusive, or that the method is
>> +		 * robust against being called multiple times.
>> +		 */
>> +		const struct arm64_cpu_capabilities *cap_list;
> 
> Nit: this is not really a list of capabilities, as noted above.
> 
> Can we call it something like "match_list"?

Ok.

> 
>>   	};
>>   };
>>   
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index a602a3049404..902d281ea26f 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -264,6 +264,36 @@ qcom_enable_link_stack_sanitization(const struct arm64_cpu_capabilities *entry)
>>   	.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,			\
>>   	CAP_MIDR_RANGE_LIST(midr_list)
>>   
>> +/*
>> + * Generic helper for handling capabilties with multiple (match,enable) pairs
>> + * of call backs, sharing the same capability bit.
>> + * Iterate over each entry to see if at least one matches.
>> + */
>> +static bool multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry,
>> +				    int scope)
>> +{
>> +	const struct arm64_cpu_capabilities *caps = entry->cap_list;
>> +
>> +	for (; caps->matches; caps++)
>> +		if (caps->matches(caps, scope))
>> +			return true;
> 
> Nit: add blank line?

  

>> +	return false;
>> +}
>> +
>> +/*
>> + * Take appropriate action for all matching entries in the shared capability
>> + * entry.
>> + */
>> +static void multi_entry_cap_cpu_enable(const struct arm64_cpu_capabilities *entry)
>> +{
>> +	const struct arm64_cpu_capabilities *caps = entry->cap_list;
>> +
>> +	for (; caps->matches; caps++)
> 
> Nit: can we move the initialiser into the for(); so
> 
> 	for (entry->cap_list; caps->matches; [...]


> 
> IMHO it's more readable to avoid empty expressions in for() unless
> there's a good reason.
> 
>> +		if (caps->matches(caps, SCOPE_LOCAL_CPU) &&

	.capability = ARM64_HARDEN_BP_POST_GUEST_EXIT,
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 9eb9e9570468..d8663822c604 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1225,9 +1225,8 @@ static bool __this_cpu_has_cap(const struct arm64_cpu_capabilities *cap_array,
>>   		return false;
>>   
>>   	for (caps = cap_array; caps->matches; caps++)
>> -		if (caps->capability == cap &&
>> -		    caps->matches(caps, SCOPE_LOCAL_CPU))
>> -			return true;
>> +		if (caps->capability == cap)
>> +			return caps->matches(caps, SCOPE_LOCAL_CPU);
> 
> Nit: add blank line?
> 
>>   	return false;

> 
> With fair consideration to the nits above,

Will do

> 
> Reviewed-by: Dave Martin <Dave.Martin at arm.com>

Thanks
Suzuki
>