Re: [PATCH v2 0/2] arm64: Run enable method for errata work arounds on late CPUs

[Robin Murphy]

On 18/01/18 12:00, Robin Murphy wrote:
[...]
> > +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.

Ah, right, but then you'd be back to casting away const, and at that 
point it makes no sense to do the container_of dance instead of just 
passing the struct pointer itself around...

I shall now excuse myself from this discussion, as I'm clearly not 
helping :)

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
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel