lkml.org 
[lkml]   [2018]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 06/16] arm64: capabilities: Unify the verification
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@arm.com>
> >>---
> >> 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
\
 
 \ /
  Last update: 2018-01-29 17:58    [W:0.110 / U:0.836 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site