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

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

On Fri, 14 Jan 2022 10:56:46 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

Hi Mark,

> The SCTLR_ELx registers contain fields which are UNKNOWN or
> IMPLEMENTATION DEFINED out of reset. This includes SCTLR_ELx.EE, which
> defines the endianness of memory accesses (e.g. reads from literal
> pools). Due to this, portions of boot-wrapper code are not guaranteed
> to work correctly.
> 
> Rework the startup code to explicitly initialize SCTLR_ELx for the
> exception level the boot-wrapper was entered at. When entered at EL2
> it's necessary to first initialise HCR_EL2.E2H as this affects the RESx
> behaviour of bits in SCTLR_EL2, and also aliases SCTLR_EL1 to SCTLR_EL2,
> which would break the initialization performed in jump_kernel.
> 
> As we plan to eventually support the highest implemented EL being any of
> EL3/EL2/EL1, code is added to handle all of these exception levels, even
> though we do not currently support starting at EL1.
> 
> We'll initialize other registers in subsequent patches.

So the idea of initialising each EL and the respective code below looks
good to me, however I have some questions about the SCTLR reset values
below:

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/aarch64/boot.S            | 74 +++++++++++++++++++++++++++-------
>  arch/aarch64/include/asm/cpu.h | 27 ++++++++++++-
>  2 files changed, 85 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index 900b9f8..45a0367 100644
> --- a/arch/aarch64/boot.S
> +++ b/arch/aarch64/boot.S
> @@ -26,26 +26,26 @@
>  	 *   PSCI is not supported when entered in this exception level.
>  	 */
>  ASM_FUNC(_start)
> -	cpuid	x0, x1
> -	bl	find_logical_id
> -	cmp	x0, #MPIDR_INVALID
> -	beq	err_invalid_id
> -	bl	setup_stack
> -
> -	/*
> -	 * EL3 initialisation
> -	 */
>  	mrs	x0, CurrentEL
>  	cmp	x0, #CURRENTEL_EL3
> -	b.eq	1f
> +	b.eq	reset_at_el3
> +	cmp	x0, #CURRENTEL_EL2
> +	b.eq	reset_at_el2
> +	cmp	x0, #CURRENTEL_EL1
> +	b.eq	reset_at_el1
>  
> -	mov	w0, #1
> -	ldr	x1, =flag_no_el3
> -	str	w0, [x1]
> +	/* Booting at EL0 is not supported */
> +	b	.
>  
> -	b	start_no_el3
> +	/*
> +	 * EL3 initialisation
> +	 */
> +reset_at_el3:
> +	mov_64	x0, SCTLR_EL3_RESET
> +	msr	sctlr_el3, x0
> +	isb
>  
> -1:	mov	x0, #0x30			// RES1
> +	mov	x0, #0x30			// RES1
>  	orr	x0, x0, #(1 << 0)		// Non-secure EL1
>  	orr	x0, x0, #(1 << 8)		// HVC enable
>  
> @@ -135,10 +135,54 @@ ASM_FUNC(_start)
>  	ldr	x0, =COUNTER_FREQ
>  	msr	cntfrq_el0, x0
>  
> +	cpuid	x0, x1
> +	bl	find_logical_id
> +	cmp	x0, #MPIDR_INVALID
> +	b.eq	err_invalid_id
> +	bl	setup_stack
> +
>  	bl	gic_secure_init
>  
>  	b	start_el3
>  
> +	/*
> +	 * EL2 initialization
> +	 */
> +reset_at_el2:
> +	// Ensure E2H is not in use
> +	mov_64	x0, HCR_EL2_RESET
> +	msr	hcr_el2, x0
> +	isb
> +
> +	mov_64	x0, SCTLR_EL2_RESET
> +	msr	sctlr_el2, x0
> +	isb
> +
> +	b	reset_no_el3
> +
> +	/*
> +	 * EL1 initialization
> +	 */
> +reset_at_el1:
> +	mov_64	x0, SCTLR_EL1_RESET
> +	msr	sctlr_el1, x0
> +	isb
> +
> +	b	reset_no_el3
> +
> +reset_no_el3:
> +	cpuid	x0, x1
> +	bl	find_logical_id
> +	cmp	x0, #MPIDR_INVALID
> +	b.eq	err_invalid_id
> +	bl	setup_stack
> +
> +	mov	w0, #1
> +	ldr	x1, =flag_no_el3
> +	str	w0, [x1]
> +
> +	b	start_no_el3
> +
>  err_invalid_id:
>  	b	.
>  
> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> index 1053414..1e9141a 100644
> --- a/arch/aarch64/include/asm/cpu.h
> +++ b/arch/aarch64/include/asm/cpu.h
> @@ -14,6 +14,32 @@
>  #define MPIDR_ID_BITS		0xff00ffffff
>  
>  #define CURRENTEL_EL3		(3 << 2)
> +#define CURRENTEL_EL2		(2 << 2)
> +#define CURRENTEL_EL1		(1 << 2)
> +
> +/*
> + * RES1 bit definitions definitions as of ARM DDI 0487G.b
> + *
> + * These includes bits which are RES1 in some configurations.
> + */
> +#define SCTLR_EL3_RES1		(BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> +				 BIT(18) | BIT(16) | BIT(11) | BIT(5) | BIT(4))
> +
> +#define SCTLR_EL2_RES1		(BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> +				 BIT(18) | BIT(16) | BIT(11) | BIT(5) | BIT(4))

I compared all bits against the ARM ARM and the kernel version for EL2,
that looks correct to me.

> +
> +#define SCTLR_EL1_RES1		(BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> +				 BIT(11) | BIT(8) | BIT(7) | BIT(4))

- The kernel sets TSCXT(bit[20]), and the ARM ARM says that the value
should be RES1 if FEAT_CSV2_* is not implemented. Should we copy this?
- The kernel clears ITD(bit[7]), and the ARM ARM says it's *Otherwise* RES1
(no AArch32 in EL0). I feel like we should not disable IT instructions in
EL0 needlessly?
- I also feel like we should set CP15BEN(bit[5]), for similar reasons.

Granted those bits affect only EL0 execution, which we don't care about in
the boot-wrapper, but I was wondering if we should change those anyway? At
least bit 20?

> +
> +#define HCR_EL2_RES1		(BIT(1))

Should we set RW(bit[31]), just to be safe? Not sure this is explicitly
mentioned somewhere, but is the boot flow when we are entered in EL2 to
stay in EL2 and launch the kernel in there as well?

> +
> +/*
> + * Initial register values required for the boot-wrapper to run out-of-reset.
> + */
> +#define SCTLR_EL3_RESET		SCTLR_EL3_RES1
> +#define SCTLR_EL2_RESET		SCTLR_EL2_RES1
> +#define SCTLR_EL1_RESET		SCTLR_EL1_RES1
> +#define HCR_EL2_RESET		HCR_EL2_RES1
>  
>  #define ID_AA64PFR0_EL1_GIC	BITS(27, 24)
>  
> @@ -43,7 +69,6 @@
>  #define ZCR_EL3_LEN_MASK	0x1ff
>  
>  #define SCTLR_EL1_CP15BEN	(1 << 5)
> -#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.
Shall we have a separate reset value for 32bit?

Cheers,
Andre


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