Re: [PATCH v2 16/20] arm64: Handle shared capability entries
From: Robin Murphy
Date: Thu Feb 08 2018 - 07:33:04 EST
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@xxxxxxx>
Cc: Will Deacon <will.deacon@xxxxxxx>
Cc: Mark Rutland <mark.rutland@xxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel