Re: [PATCH 06/16] arm64: capabilities: Unify the verification

[Dave Martin]

On Fri, Jan 26, 2018 at 12:10:11PM +0000, Suzuki K Poulose wrote:
> On 26/01/18 11:08, Dave Martin wrote:
> >On Tue, Jan 23, 2018 at 12:27:59PM +0000, Suzuki K Poulose wrote:
> >>Now that each capability describes how to treat the conflicts
> >>of CPU cap state vs System wide cap state, we can unify the
> >>verification logic to a single place.
> >>
> >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >>---
> >>  arch/arm64/kernel/cpufeature.c | 87 ++++++++++++++++++++++++++----------------
> >>  1 file changed, 54 insertions(+), 33 deletions(-)
> >>
> >>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>index 43c7e992d784..79737034a628 100644
> >>--- a/arch/arm64/kernel/cpufeature.c
> >>+++ b/arch/arm64/kernel/cpufeature.c
> >>@@ -1228,6 +1228,54 @@ static void __init enable_cpu_capabilities(const struct arm64_cpu_capabilities *
> >>  }
> >
> >>  /*
> >>+ * Run through the list of capabilities to check for conflicts.
> >>+ * Returns "false" on conflicts.
> >>+ */
> >>+static bool __verify_local_cpu_caps(const struct arm64_cpu_capabilities *caps_list)
> >>+{
> >>+	bool cpu_has_cap, system_has_cap;
> >>+	const struct arm64_cpu_capabilities *caps = caps_list;
> >>+
> >>+	for (; caps->matches; caps++) {
> >>+		cpu_has_cap = __this_cpu_has_cap(caps_list, caps->capability);
> >
> >What's the point of scanning the whole of caps_list?  Don't we already
> >have the pointer to the right cap struct?
> >
> >We already know caps->matches is true.  Can't we just call
> >caps->matches(caps)?  That seemed pretty intuitive to me in the old
> >code.
> >
> 
> This was supposed to be fixed by [1] in the "old code". Given we have multiple
> entries for a "capability", we could be dealing with the one which doesn't
> apply to this CPU and could eventually trigger a wrong conflict below. To
> avoid this, we need to make sure use the right values.

Ah, I see: do we want to do something like this:

	for (each cap corresponding to a bit in cpu_hwcaps) {
		for (each arm64_cpu_capabilities c corresponding to this cap) {
			if (c->matches(c, ...))
				goto ok;
		}

		goto mismatch;

	ok:
		continue;
	}

	return 0;

mismatch:
	/* barf */
	return -1;

An additional comment explaining the purpose of the code might help
(though I could have read the commit message, I guess).

We can't do the above directly, becasue we don't index the capabilities
by the capability field.  The above looks O((number of
arm64_cpu_capability structs) ^ 2), which could become slightly annoying
as the number of structs grows (?)

Could this be solved by making the match criteria a separate struct
and allowing a list of them to be specified per-capability?

Maybe too much effort for this series though.

> 

[...]

> >The role of the ->enable() call is the only real subtlety here.
> >
> >>+			if (cpu_has_cap && !cpucap_late_cpu_have_cap_safe(caps))
> >>+				break;
> >>+		}
> >>+	}
> >>+
> >>+	if (caps->matches) {
> >>+		pr_crit("CPU%d: Detected conflict for capability %d (%s), System: %d, CPU: %d\n",
> >>+			smp_processor_id(), caps->capability,
> >>+			caps->desc ? : "no description",
> >
> >Wouldn't it be a bug for a conflict to occur on a cap with no .desc?
> >
> >Why can't we just let printk print its default "(null)" for %s
> >in this case?
> 
> We could.
> 
> >
> >Alternatively, is there a reason for any cap not to have a description?
> 
> Some of them do. e.g, some of them could be "negative" capabilities. e.g,
> ARM64_NO_FPSIMD.

Is that a reason not to have a description?

> >>+			system_has_cap, cpu_has_cap);
> >>+		return false;
> >>+	}
> >>+
> >>+	return true;
> >>+}
> >
> >Perhaps the capability verification procedure could be made a little
> >clearer by splitting this into two functions:
> >
> 
> As explained above, the code below is not sufficient.

Fair enough: I hadn't understood what the code was trying to achieve.

Given that, it's a bit harder to refactor than I though, and it's
probably not worth it.

[...]

Cheers
---Dave