Messages in this thread | | | Subject | Re: [PATCH v2 0/2] arm64: Run enable method for errata work arounds on late CPUs | From | Robin Murphy <> | Date | Thu, 18 Jan 2018 12:00:07 +0000 |
| |
On 18/01/18 11:45, Dave Martin wrote: > On Wed, Jan 17, 2018 at 05:42:19PM +0000, Suzuki K Poulose wrote: >> We issue the enable() call back for all CPU hwcaps capabilities >> available on the system, on all the CPUs. So far we have ignored >> the argument passed to the call back, which had a prototype to >> accept a "void *" for use with on_each_cpu() and later with >> stop_machine(). However, with commit 0a0d111d40fd1 >> ("arm64: cpufeature: Pass capability structure to ->enable callback"), >> there are some users of the argument who wants the matching capability >> struct pointer where there are multiple matching criteria for a single >> capability. Changing the prototype is quite an invasive change and > > It's not that bad, though it's debatable whether it's really worth > it. See the appended patch below. > >> will be part of a future series. For now, add a comment to clarify >> what is expected. > > With the type change, it becomes more obvious what should be passed, > and the comment can probably be trimmed down. I omit the comment > from my patch (I'm lazy). > > Without it, I would prefer a comment alongside the (void *) cast, > something like "enable methods expect this to be passed as a void *, > for compatibility with stop_machine()". > > For consistency, the stop_machine() call should also be changed to > pass the cap pointer instead of NULL, even if we don't actually rely > on that for any purpose today -- it will help avoid surprises in the > future. (My patch does makes an equivalent change.) > > [...] > >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h >> index ac67cfc2585a..c049e28274d4 100644 >> --- a/arch/arm64/include/asm/cpufeature.h >> +++ b/arch/arm64/include/asm/cpufeature.h >> @@ -97,7 +97,14 @@ struct arm64_cpu_capabilities { >> u16 capability; >> int def_scope; /* default scope */ >> bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope); >> - int (*enable)(void *); /* Called on all active CPUs */ >> + /* >> + * For each @capability set in CPU hwcaps, @enable() is called on all >> + * active CPUs with const struct arm64_cpu_capabilities * as argument. > > The declaration tells us the type, so we don't need that in the comment. > But what object should the pointer point to? > >> + * It is upto the callback (especially when multiple entries for the > > up to > >> + * same capability exists) to determine if any action should be taken >> + * based on @matches() applies to thie CPU. >> + */ >> + int (*enable)(void *caps); >> union { >> struct { /* To be used for erratum handling only */ >> u32 midr_model; > > Cheers > ---Dave > > > --8<-- > > From 04a4bd998d02e339341db250e66b0d34a7c4e042 Mon Sep 17 00:00:00 2001 > From: Dave Martin <Dave.Martin@arm.com> > Date: Thu, 18 Jan 2018 11:29:14 +0000 > Subject: [PATCH] arm64: cpufeature: Formalise capability enable method type > > Currently, all cpufeature enable methods specify their argument as > a void *, while what is actually passed is a pointer to a const > struct arm64_cpu_capabilities object. > > Hiding the type information from the compiler increases the risk > of incorrect code. > > Instead of requiring care to be taken in every enable method, this > patch makes the type of the argument explicitly struct > arm64_cpu_capabilities const *. > > Since we need to call these methods via stop_machine(), a single > proxy function __cpu_enable_capability() is added which is passed > to stop_machine() to allow it to call feature enable methods > without defeating type checking in the compiler (except in this > one place). > > This patch should not change the behaviour of the kernel. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > --- > > Build-tested only. > > arch/arm64/include/asm/cpufeature.h | 4 +++- > arch/arm64/include/asm/fpsimd.h | 4 +++- > arch/arm64/include/asm/processor.h | 5 +++-- > arch/arm64/kernel/cpu_errata.c | 3 ++- > arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- > arch/arm64/kernel/fpsimd.c | 3 ++- > arch/arm64/kernel/traps.c | 3 ++- > arch/arm64/mm/fault.c | 2 +- > 8 files changed, 35 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 060e3a4..98d22ce 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -100,7 +100,9 @@ struct arm64_cpu_capabilities { > u16 capability; > int def_scope; /* default scope */ > bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope); > - int (*enable)(void *); /* Called on all active CPUs */ > + /* Called on all active CPUs: */ > + int (*enable)(struct arm64_cpu_capabilities const *); > + > union { > struct { /* To be used for erratum handling only */ > u32 midr_model; > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index 74f3439..ac9902d 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -83,7 +83,9 @@ extern void sve_save_state(void *state, u32 *pfpsr); > extern void sve_load_state(void const *state, u32 const *pfpsr, > unsigned long vq_minus_1); > extern unsigned int sve_get_vl(void); > -extern int sve_kernel_enable(void *); > + > +struct arm64_cpu_capabilities; > +extern int sve_kernel_enable(struct arm64_cpu_capabilities const *); > > extern int __ro_after_init sve_max_vl; > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index 023cacb..46959651a 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -34,6 +34,7 @@ > #include <linux/string.h> > > #include <asm/alternative.h> > +#include <asm/cpufeature.h> > #include <asm/fpsimd.h> > #include <asm/hw_breakpoint.h> > #include <asm/lse.h> > @@ -214,8 +215,8 @@ static inline void spin_lock_prefetch(const void *ptr) > > #endif > > -int cpu_enable_pan(void *__unused); > -int cpu_enable_cache_maint_trap(void *__unused); > +int cpu_enable_pan(struct arm64_cpu_capabilities const *__unused); > +int cpu_enable_cache_maint_trap(struct arm64_cpu_capabilities const *__unused); > > /* Userspace interface for PR_SVE_{SET,GET}_VL prctl()s: */ > #define SVE_SET_VL(arg) sve_set_current_vl(arg) > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c > index 0e27f86..addd7fd 100644 > --- a/arch/arm64/kernel/cpu_errata.c > +++ b/arch/arm64/kernel/cpu_errata.c > @@ -39,7 +39,8 @@ has_mismatched_cache_line_size(const struct arm64_cpu_capabilities *entry, > (arm64_ftr_reg_ctrel0.sys_val & arm64_ftr_reg_ctrel0.strict_mask); > } > > -static int cpu_enable_trap_ctr_access(void *__unused) > +static int cpu_enable_trap_ctr_access( > + struct arm64_cpu_capabilities const *__unused) > { > /* Clear SCTLR_EL1.UCT */ > config_sctlr_el1(SCTLR_EL1_UCT, 0); > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index a73a592..05486a9 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1084,6 +1084,18 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps, > } > } > > +struct enable_arg { > + int (*enable)(struct arm64_cpu_capabilities const *); > + struct arm64_cpu_capabilities const *cap; > +}; > + > +static int __enable_cpu_capability(void *arg) > +{ > + struct enable_arg const *e = arg; > + > + return e->enable(e->cap); > +}
AFAICS, you shouldn't even need the intermediate struct - if you were instead to call stop_machine(&caps->enable, ...), the wrapper could be:
<type> **fn = arg; *fn(container_of(fn, struct arm64_cpu_capabilities, enable));
(cheaty pseudocode because there's no way I'm going to write a pointer-to-function-pointer type correctly in an email client...)
That's certainly a fair bit simpler in terms of diffstat; whether it's really as intuitive as I think it is is perhaps another matter, though.
Robin.
> + > /* > * Run through the enabled capabilities and enable() it on all active > * CPUs > @@ -1100,13 +1112,19 @@ void __init enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps) > static_branch_enable(&cpu_hwcap_keys[num]); > > if (caps->enable) { > + struct enable_arg e = { > + .enable = caps->enable, > + .cap = caps, > + }; > + > /* > * Use stop_machine() as it schedules the work allowing > * us to modify PSTATE, instead of on_each_cpu() which > * uses an IPI, giving us a PSTATE that disappears when > * we return. > */ > - stop_machine(caps->enable, NULL, cpu_online_mask); > + stop_machine(__enable_cpu_capability, &e, > + cpu_online_mask); > } > } > } > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index fae81f7..b6dcfa3 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -40,6 +40,7 @@ > #include <linux/sysctl.h> > > #include <asm/fpsimd.h> > +#include <asm/cpufeature.h> > #include <asm/cputype.h> > #include <asm/simd.h> > #include <asm/sigcontext.h> > @@ -757,7 +758,7 @@ static void __init sve_efi_setup(void) > * Enable SVE for EL1. > * Intended for use by the cpufeatures code during CPU boot. > */ > -int sve_kernel_enable(void *__always_unused p) > +int sve_kernel_enable(struct arm64_cpu_capabilities const *__always_unused p) > { > write_sysreg(read_sysreg(CPACR_EL1) | CPACR_EL1_ZEN_EL1EN, CPACR_EL1); > isb(); > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index 3d3588f..2457514 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -38,6 +38,7 @@ > > #include <asm/atomic.h> > #include <asm/bug.h> > +#include <asm/cpufeature.h> > #include <asm/daifflags.h> > #include <asm/debug-monitors.h> > #include <asm/esr.h> > @@ -374,7 +375,7 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs) > force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > } > > -int cpu_enable_cache_maint_trap(void *__unused) > +int cpu_enable_cache_maint_trap(struct arm64_cpu_capabilities const *__unused) > { > config_sctlr_el1(SCTLR_EL1_UCI, 0); > return 0; > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 9b7f89d..82f6729 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -795,7 +795,7 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, > NOKPROBE_SYMBOL(do_debug_exception); > > #ifdef CONFIG_ARM64_PAN > -int cpu_enable_pan(void *__unused) > +int cpu_enable_pan(struct arm64_cpu_capabilities const *__unused) > { > /* > * We modify PSTATE. This won't work from irq context as the PSTATE >
| |