[PATCH v2 16/20] arm64: Handle shared capability entries

[Suzuki K Poulose]

On 07/02/18 10:39, Dave Martin wrote:
> On Wed, Jan 31, 2018 at 06:28:03PM +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 and ensures :
>>   1) The capabilitiy is set when at least one of the entry detects
>>   2) Action is only taken for the entries that detects.
> 
> I guess this means that where we have a single cpu_enable() method
> but complex match criteria that require multiple entries, then that
> cpu_enable() method might get called multiple times on a given CPU.

A CPU executes cpu_enable() only for the "matching" entries in the list,
unlike earlier. So as long as there is a single entry "matching" the given
CPU, the cpu_enable() will not be duplicated. May be we *should* mandate
that a CPU cannot be matched by multiple entries.

> 
> Could be worth a comment if cpu_enable() methods must be robust
> against this.
> 
>> This avoids explicit checks in the call backs. The only constraint
>> here is that, all the entries should have the same "type".
>>
>> Cc: Dave Martin <dave.martin at arm.com>
>> Cc: Will Deacon <will.deacon at arm.com>
>> Cc: Mark Rutland <mark.rutland at arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose at arm.com>
>> ---
>>   arch/arm64/include/asm/cpufeature.h |  1 +
>>   arch/arm64/kernel/cpu_errata.c      | 53 ++++++++++++++++++++++++++++++++-----
>>   arch/arm64/kernel/cpufeature.c      |  7 +++--
>>   3 files changed, 50 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 462c35d1a38c..b73247c27f00 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -290,6 +290,7 @@ struct arm64_cpu_capabilities {
>>   			bool sign;
>>   			unsigned long hwcap;
>>   		};
>> +		const struct arm64_cpu_capabilities *cap_list;
> 
> Should desc, capability, def_scope and/or cpu_enable match for every cap
> in such a group?

As mentioned above, the "type" field should match, which implies scope
must match. The code ignores the scope, capability and desc of the individual
entries in the list. They should be shared by the parent entry.

cpu_enable could be duplicated as long as a CPU is not matched by multiple
entries.

> 
> I'd expected something maybe like this:
> 
> struct arm64_cpu_capabilities {
> 	const char *desc;
> 	u16 capability;
> 	struct arm64_capability_match {
> 		bool (*matches)(const struct arm64_cpu_capabilities *, int);
> 		int (*cpu_enable)(void);
> 		union {
> 			struct { ... midr ... };
> 			struct { ... sysreg ... };
> 			const struct arm64_capability_match *list;
> 		};
>>   	};
>>   };

Yes, thats makes it more explicit. However, it makes the "matches()"
complicated, as we have to change the prototype for matches to accept
struct arm64_capability_match *, to point to the right "matches" for
items in the list. And that makes a bit more of an invasive change, where
each matches() would then loose a way to get to the "capabilities" entry,
as they could be called with either the "match" in the top level or
the one in the list.

>>   		.capability = ARM64_HARDEN_BP_POST_GUEST_EXIT,
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 65a8e5cc600c..13e30c1b1e99 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1181,9 +1181,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);
> 
> If we went for my capability { cap; match criteria or list; } approach,
> would it still be necessary to iterate over the whole list here?

Sorry, I couldn't follow this. With this patch, we already stop scanning
the list as soon as we find the first entry. It is upto "the entry" to run
individual match criteria to decide.

> 
> This seems preferable if this function is used by other paths that
> don't expect it to be so costly.  Currently I only see a call in
> arch/arm64/kvm/handle_exit.c:handle_exit_early() for the SError case --
> which is probably not expected to be a fast path.
uu>
> [...]
> 
> Cheers
> ---Dave
> 

Cheers
Suzuki