Re: [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 17 Jan 2022 18:08:13 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

Hi,

> On Mon, Jan 17, 2022 at 02:31:04PM +0000, Andre Przywara wrote:
> > On Fri, 14 Jan 2022 10:56:49 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > Hi Mark,
> >   
> > > The majority of state that we initialize at EL3 is necessary for code at
> > > lower ELs to function, but isnt' necessary for the boot-wrapper itself.
> > > Given that, it would be better to write this in C where it can be
> > > written mode clearly, and where it will be possible to add logging/debug
> > > logic.  
> > 
> > Ah, thanks, that looks much nicer and easier to read now, also is more
> > robust, as keeping register values alive for more than a few assembly
> > lines always scares me.
> >   
> > > This patch migrates the AArch64 EL3 initialization to C.
> > > 
> > > There should be no functional change as a result of this patch.  
> > 
> > I compared the removed assembly code against to added C code, and also
> > checked the register bits against the ARM ARM.
> > Two (and a half) things stood out, see below:  
> 
> Thanks for this! I've fixed those as noted below.
> 
> [...]
> 
> > > -#define HCR_EL2_RES1		(BIT(1))
> > > +#define ZCR_EL3				s3_6_c1_c2_0
> > > +#define ZCR_EL3_LEN			BITS(3, 1)  
> > 
> > The (current) actual length field should be BITS(3, 0), no?  
> 
> Yes, it should. I've corrected that to BITS(3, 0) now.
> 
> [...]
> 
> > > +void cpu_init_el3(void)
> > > +{
> > > +	unsigned long scr = SCR_EL3_RES1 | SCR_EL3_NS | SCR_EL3_HCE;
> > > +	unsigned long mdcr = 0;
> > > +	unsigned long cptr = 0;
> > > +
> > > +	if (cpu_has_pauth())
> > > +		scr |= SCR_EL3_APK | SCR_EL3_API;
> > > +
> > > +	if (mrs_field(ID_AA64ISAR0_EL1, TME))
> > > +		scr |= SCR_EL3_TME;
> > > +
> > > +	if (mrs_field(ID_AA64MMFR0_EL1, FGT))
> > > +		scr |= SCR_EL3_FGTEN;
> > > +
> > > +	if (mrs_field(ID_AA64MMFR0_EL1, ECV) >= 2)
> > > +		scr |= SCR_EL3_ECVEN;
> > > +
> > > +	if (mrs_field(ID_AA64PFR1_EL1, MTE))  
> > 
> > The assembly code checked for >=2, which seems correct to me, as
> > SCR_EL3_ATA is about MTE2?  
> 
> Yes; I botched that when converting to C. SCR_EL3.ATA is RES0 in the
> absence of MTE2. I've added `>= 2` to the condition to match that.
> 
> > > +		scr |= SCR_EL3_ATA;  
> 
> [...]
> 
> > > +	if (mrs_field(ID_AA64PFR0_EL1, SVE)) {
> > > +		cptr |= CPTR_EL3_EZ;
> > > +		msr(CPTR_EL3, cptr);
> > > +		isb();
> > > +		msr(ZCR_EL3, ZCR_EL3_LEN);  
> > 
> > So when comparing this to the other uses of XXX_EL3_YYY, they typically
> > describe a mask, but here we seems to abuse this as a value?  
> 
> True; I'll add a separate defintion for the value.
> 
> > And apart from bit 0 missing from it (as noted above), the existing
> > code writes 0x1ff into that register, presumable to cover future
> > vector length extensions beyond 2048 bits (which those RAZ/WI fields
> > in bits[8:4] seem to suggest).  
> 
> Hmm... I went and found the SVE supplement and I can't see any rationale
> for what SW *should* do, nor can I find a description of the register
> (that seems to have been factored into some XML files I can't convince
> anything to load on my machine).
> 
> > So shall we define ZCR_EL3_MAX_VEC_LEN to 0x1ff above, and then use that?
> > Or ignore the crystal ball, and just stick with 2048 bits, by writing 0xf?  
> 
> TBH, I'm not sure. In the absence of some documented guidance I'd prefer
> to go with 0xf, but given we already use 0x1ff, I want to dig into this
> a bit more.

My impression was that this "[8:4] = RAZ/WI" compared to the "[63:9] =
RES0" fields suggests this is for a potential extension, but I guess there
would be more changes needed if SVE ever goes beyond 2048. So chances are
high we need to adopt the code then anyway, and fixing the number then is
the least of our problems.

So I feel we should stick to what's explicitly documented, and put 0xf in
there.

Cheers,
Andre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel