raspberrypi / tools

1.88k stars 866 forks source link

armstub8.S: missing ACTLR_EL3 initialization #114

Closed fincs closed 4 years ago

fincs commented 4 years ago

Currently the arm bootstub for AArch64 does not set the system register ACTLR_EL3. This register (which on reset is zero) enables access from lower exception levels to other system registers which are needed to apply fixes for errata affecting the processor. I hit this problem while working on a certain OS port to the Pi 3, however it also affects other codebases such as u-boot: https://github.com/u-boot/u-boot/blob/master/arch/arm/cpu/armv8/start.S#L208-L240

I've tested locally the following change to the armstub, and verified it working:

@@ -63,10 +63,13 @@
 #define SCR_RES1_4     BIT(4)
 #define SCR_NS         BIT(0)
 #define SCR_VAL \
     (SCR_RW | SCR_HCE | SCR_SMD | SCR_RES1_5 | SCR_RES1_4 | SCR_NS)

+#define ACTLR_VAL \
+   (BIT(0) | BIT(1) | BIT(4) | BIT(5) | BIT(6))
+
 #define CPUECTLR_EL1       S3_1_C15_C2_1
 #define CPUECTLR_EL1_SMPEN BIT(6)

 #define SPSR_EL3_D     BIT(9)
 #define SPSR_EL3_A     BIT(8)
@@ -118,10 +121,14 @@ _start:

    /* Set up SCR */
    mov x0, #SCR_VAL
    msr SCR_EL3, x0

+   /* Set up ACTLR */
+   mov x0, #ACTLR_VAL
+   msr ACTLR_EL3, x0
+
    /* Set SMPEN */
    mov x0, #CPUECTLR_EL1_SMPEN
    msr CPUECTLR_EL1, x0

 #ifdef GIC

It would be very much appreciated if this change could make it upstream, or if an alternative solution was suggested.

pelwell commented 4 years ago

The registers you've enabled access to aren't used by Linux, so I'd prefer to leave it as it is.

Distributions are free to bundle custom versions of stub files - this one would be loaded automatically if it's called called armstub8-gic.bin.

fincs commented 4 years ago

Thankfully, since Linux does not use these registers, my proposed change would not introduce any regressions.

It would be ideal if there was no need for non-Linux operating systems to distribute custom versions of the arm bootstrapping code, as they are an additional maintenance burden & can lead to hard to debug problems when they inevitably get out of sync with upstream.

pelwell commented 4 years ago

Space is an issue in the stubs. 0x100 is the historical address to position the flattened Device Tree or ATAGs for the kernel, and I think some bare metal code might depend on that. As of today the 32-bit Pi 4 stub is exactly 0x100 bytes long with no padding. Your patch is only to the 64-bit stub, which is already longer, making it less of an issue, but then the two stubs have different functionality - which is less than ideal.