Re: [PATCH v2 10/28] arm64/sve: Low-level CPU setup

Dave Martin
2017-10-05T11:22:18Z

On Thu, Oct 05, 2017 at 12:04:12PM +0100, Suzuki K Poulose wrote:
> On 05/10/17 11:47, Dave Martin wrote:

[...]

> >>>On Thu, Aug 31, 2017 at 06:00:42PM +0100, Dave P Martin wrote:
> >>>>diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> >>>>index 877d42f..dd22ef2 100644
> >>>>--- a/arch/arm64/mm/proc.S
> >>>>+++ b/arch/arm64/mm/proc.S
> >>>>@@ -27,6 +27,7 @@
> >>>> #include <asm/pgtable-hwdef.h>
> >>>> #include <asm/cpufeature.h>
> >>>> #include <asm/alternative.h>
> >>>>+#include <asm/sysreg.h>
> >>>> #ifdef CONFIG_ARM64_64K_PAGES
> >>>> #define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K
> >>>>@@ -186,8 +187,17 @@ ENTRY(__cpu_setup)
> >>>> tlbi vmalle1 // Invalidate local TLB
> >>>> dsb nsh
> >>>>- mov x0, #3 << 20
> >>>>- msr cpacr_el1, x0 // Enable FP/ASIMD
> >>>>+ mov x0, #3 << 20 // FEN
> >>>>+
> >>>>+ /* SVE */
> >>>>+ mrs x5, id_aa64pfr0_el1
> >>>>+ ubfx x5, x5, #ID_AA64PFR0_SVE_SHIFT, #4
> >>>>+ cbz x5, 1f
> >>>>+
> >>>>+ bic x0, x0, #CPACR_EL1_ZEN
> >>>>+ orr x0, x0, #CPACR_EL1_ZEN_EL1EN // SVE: trap for EL0, not EL1
> >>>>+1: msr cpacr_el1, x0 // Enable FP/ASIMD

[..]

> >I can add a helper el1_enable_sve() say, and call it before probing
> >ZCR_EL1 in the cpufeatures code.
> >
> >This makes enabling SVE a side-effect of the cpufeatures code, which
> >I'm not that comfortable with -- it feels like something that later
> >refactoring could easily break.
> >
> >I can also add an explicit call to el1_enable_sve() in sve_setup(),
> >but this only works on the boot cpu.
> >
> >For secondaries, I could add something in secondary_start_kernel(),
> >but this looks incongruous since there's no other call to do something
> >similar yet.
> >
> >
> >** Suzuki, do we have any other case where the trap for a CPU feature is
> >turned off by the cpufeatures code? If there's already a template for
> >this then I'm happy to follow it.
>
> The closest thing you have is an "enable" callback for each "available"
> capability, which gets invoked on all CPUs (boot time active CPUs and the
> ones which are brought up later). This is used by things like, PAN to
> do some CPU specific setups.
>
> See :
> enable_cpu_capabilities() // For all boot time active CPUs
> and
> verify_local_cpu_features() // For CPUs brought up later

That may allow me to do something tidier, provided the enable method is
called early enough.

I'll take a look.

Cheers
---Dave