Re: [PATCH v3 19/22] arm64: capabilities: Handle shared entries
From: Suzuki K Poulose
Date: Thu Mar 08 2018 - 07:16:52 EST
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@xxxxxxx>
Cc: Robin Murphy <robin.murphy@xxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
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@xxxxxxx>
Thanks
Suzuki