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

[Dave Martin <Dave.Martin@arm.com>]

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.

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