Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Jan 18, 2022 at 12:37:41PM +0000, Andre Przywara wrote:
> On Mon, 17 Jan 2022 13:05:54 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> Hi Mark,
> 
> > On Mon, Jan 17, 2022 at 12:15:57PM +0000, Mark Rutland wrote:
> > > On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:  
> > > > On Fri, 14 Jan 2022 10:56:46 +0000
> > > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > > 
> > > > Hi Mark,  
> > > 
> > > Hi Andre,  
> >  
> > > > > +
> > > > > +#define SCTLR_EL1_RES1		(BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > > > > +				 BIT(11) | BIT(8) | BIT(7) | BIT(4))  
> > 
> > > > > -#define SCTLR_EL1_RES1		(3 << 28 | 3 << 22 | 1 << 11)
> > > > >  
> > > > >  #ifdef KERNEL_32
> > > > >  /* 32-bit kernel decompressor uses CP15 barriers */
> > > > > #define SCTLR_EL1_KERNEL        (SCTLR_EL1_RES1 | SCTLR_EL1_CP15BEN)  
> > > > 
> > > > So I wonder if this actually works? The ARMv7 version of SCTLR
> > > > differs in some bits from both the ARMv8 AArch32 version and more
> > > > importantly the AArch64 version.  
> > 
> > > > I had troubles the other day running the
> > > > arm32 Linux kernel decompressor with some ARMv8 SCTLR_EL1 reset value. The
> > > > decompressor code does only read-modify-write of SCTLR (probably to
> > > > cover multiple architecture revisions), so some bits might stay wrong. In
> > > > particular I think having bits 28 and 29 set caused problems.
> > > > By looking at the ARMv7 ARM and with experimentation I came up
> > > > with 0x00c00878 as a safe and working value.  
> > 
> > Having re-read all of this, I believe you're right; I'll rework the
> > AArch32-kernel SCTLR_EL1_KERNEL to not use the AArch64 bit definitions,
> > and I'll add some commentary explaining that we're writing to the AArch32
> > format.
> > 
> > > > Shall we have a separate reset value for 32bit?  
> > 
> > Assuming you meant to alter the SCTLR_ELx_KERNEL definition, yes.
> > 
> > The idea of splitting the SCTLR_ELx_RESET and SCTLR_ELx_KERNEL
> > definitions was that the former would be whatever the boot-wrapper
> > needed to run at ELx, and the latter was whatever we must initialize for
> > the kernel to run at ELx, so I don't want to put the AArch32 kernel bits
> > into SCTLR_EL1_RESET.
> > 
> > My only remaining concern is exactly what we must initialize. If there's
> > any documentation we can refer to, that'd be great, otherwise I'll dig
> > through your prior suggestion.
> 
> I don't know of any recommendation what to initialise, though the ARMv7
> ARM speaks a bit about reset values.
> So the SCTLR_KERNEL value we use in arch/aarch32/include/asm/cpu.h seems
> to work (although it's not perfect), but we should use the same in the
> KERNEL_32 case in arch/aarch64/include/asm/cpu.h.

Ok; for now I'm going to copy the SCTLR_KERNEL definition as-is from the
aarch32 header, and add a comment that we should restructure things to share a
single definition in future.

The rest of the reply is mostly for future reference.

> Going through the SCTLR description in the ARMv8 *and* ARMv7 ARM again,
> I think a safe and sane init value would be to set bits
> [23,22,18,16,11,5,4,3] to one. This differs slightly from the value I told
> you above, which I think was what I observed on an Cortex-A7 when dumping
> SCTLR in the decompressor code.

Having delved through the ARM ARM, those bits look right to me. Below is
expanded detail for each bit to save having to delve into the ARM ARM again.

Looking at the latest ARM ARM (ARM DDI 0487G.b), section G8.2.126 "SCTLR,
System Control Register" starting on page G8-6810:

* Bit 25 / EE
  RES0/RES1 in depending on implemented endianness support. Currently the
  boot-wrapper doesn't handle missin support for either BE or LE, so I'm going
  to ignore this for now and just assume LE is present (so 0 is a legitimate
  value).

* Bit 23 / SPAN
  RES1 where FEAT_PAN is not implemented.

* Bit 22 
  RES1 always.

* Bit 18 / nTWE
  Not RESx, but warm-resets to 1.

* Bit 16 / nTWI
  Not RESx, but warm-resets to 1.

* Bit 11 
  RES1 always.

* Bit 5 / CP15BEN
  Not RESx, but warm-resets to 1.

* Bit 4 / LSMAOE
  RES1 where FEAT_LSMAOC is not implemented.
  Warm-resets to 1 where FEAT_LSMAOC is implemented.

* Bit 3 / nTLSMD
  RES1 where FEAT_LSMAOC is not implemented.
  Warm-resets to 1 where FEAT_LSMAOC is implemented.

... with all other bits being UNKNOWN, RES0, or warm-rest to 0.

> I became aware of the issue when I tried to start an ARM kernel on a
> Cortex-A53 board, with U-Boot dropping from AArch64-EL2 to AArch32-EL1.
> SCTLR_EL1 there got initialised according to the ARMv8 ARM (bits
> [29,28,23,22,20,11] set), but this made the kernel decompressor hang as
> soon as the MMU got enabled. I *think* I bisected it down to bits 28 and
> 29, which are RES1 in AArch64, but enable TEX remap and the Access Flag in
> v7, and both reset to 0 there. I haven't tried that on the model with the
> boot-wrapper, but I guess we see the same problem there.
> 
> In any case it seems to be already broken, so a fix or discussion doesn't
> need to block this series.

Sure; I think I'll save the AArch32 cleanup for a follow-up and go for a simple
change to jsut avoid making it worse in this patch.

Thanks,
Mark.

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