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

Dave Martin
2017-10-05T10:47:17Z

On Wed, Sep 13, 2017 at 08:21:11PM +0100, Dave Martin wrote:
> On Wed, Sep 13, 2017 at 06:32:06AM -0700, Catalin Marinas 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
> >
> > For EL1, I wonder whether we could move this later to cpufeature.c. IIRC
> > I tried to do the same with FPSIMD but hit an issue with EFI run-time
> > services (I may be wrong though).
>
> I'll take a look at this -- I believe it should be safe to disable this
> trap for EL1 relatively late. This is needed before probing for
> available vector lengths, but apart from that the kernel shouldn't touch
> SVE until/unless some user task uses SVE.
>
> This would change if we eventually enable kernel-mode SVE, but I wouldn't
> expect that to get used in early boot before the cpufeatures code runs.
>
> Ard may have a view on this.

I've had a go at this, but there's a lot of splatter.

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.

Otherwise, maybe it's simpler to keep it in __cpu_setup after all
since that's a common path that all CPUs pass through.

Cheers
---Dave