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

[Dave Martin <Dave.Martin@arm.com>]

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