lkml.org 
[lkml]   [2018]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 16/20] arm64: Handle shared capability entries
From
Date
On 08/02/18 12:01, Dave Martin wrote:
> On Thu, Feb 08, 2018 at 10:53:52AM +0000, Suzuki K Poulose wrote:
>> 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.
>
> Could we say something like:
>
> "Where a single capability has multiple entries, mutiple cpu_enable()
> methods may be called if more than one entry matches. Where this is
> not desired, care should be taken to ensure that the entries are
> mutually exclusive: for example, two entries for a single capability
> that match on MIDR should be configured to match MIDR ranges that do
> not overlap."
>
> This is more verbose than I would like however. Maybe there's a simpler
> way to say it.

If we're not also talking about a capability having mutually exclusive
enable methods (it doesn't seem so, but I'm not necessarily 100% clear),
how about:

"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."

?

Robin.

>>>> 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@arm.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@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.
>
> Yes, that's true. matches() could take a pointer to the cap struct
> _and_ the relevant match entry, but I'm not sure it's worth it. In any
> case, my previous objection below doesn't make as much sense as I
> thought.
>
>>>> .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.
>
> Ah, I'm talking nonsense here. Patch 6 iterates over the entire
> capability list via the call to __this_cpu_has_cap() in
> __verify_local_cpu_caps(), but this patch now changes the behaviour so
> that this doesn't happen any more.
>
> The only other users of this_cpu_has_cap() don't have the right cap
> pointer already, so a scan over the whole list is required in those
> cases -- and anyway, those look like fast paths (RAS exceptions).
>
> [...]
>
> Cheers
> ---Dave
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

\
 
 \ /
  Last update: 2018-02-08 13:33    [W:0.123 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site