Re: [PATCH v2 2/6] arm64: initialise SCTLR2_ELx register at boot time

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Marc,

> [dropping ry111@xry111.site, which bounces]
>
> On Mon, 11 Aug 2025 17:33:36 +0100,
> Yeoreum Yun <yeoreum.yun@arm.com> wrote:
> >
> > add initialisation for SCTRL2_ELx register at boot time.
>
> Again, please expand.

Okay.


[...]
> >
> > +.macro __init_el2_sctlr2
>
> Writing this as __init_sctlr2_el2 would read vastly better (yes, I
> know most macros in this file are similarly braindead).
>
> > +	.macro init_sctlr2_elx, el, tmp
> > +	mrs_s	\tmp, SYS_ID_AA64MMFR3_EL1
> > +	ubfx	\tmp, \tmp, #ID_AA64MMFR3_EL1_SCTLRX_SHIFT, #4
> > +	cbz	\tmp, .Lskip_sctlr2_\@
> > +	.if	\el == 2
> > +	mov_q	\tmp, INIT_SCTLR2_EL2
> > +	msr_s	SYS_SCTLR_EL2, \tmp
> > +	.else
> > +	mov_q	\tmp, INIT_SCTLR2_EL1
> > +	.if	\el == 12
> > +	msr_s	SYS_SCTLR_EL12, \tmp
> > +	.else
> > +	msr_s	SYS_SCTLR_EL1, \tmp
> > +	.endif
>
> I don't think this is the correct place for this macro.
> asm/assembler.h seems more suitable, and already has that sort of
> things.

Thanks for letting me know the proper place.
I'll rewrite it on there.

>
> > +	.endif
> > +.Lskip_sctlr2_\@:
> > +	.endm
> >  #else
> >
> >  #include <linux/bitfield.h>
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index ca04b338cb0d..0dff7593e50b 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -276,6 +276,7 @@ SYM_INNER_LABEL(init_el1, SYM_L_LOCAL)
> >  	mov_q	x0, INIT_SCTLR_EL1_MMU_OFF
> >  	pre_disable_mmu_workaround
> >  	msr	sctlr_el1, x0
> > +	init_sctlr2_elx	1, x0
> >  	isb
> >  	mov_q	x0, INIT_PSTATE_EL1
> >  	msr	spsr_el1, x0
> > @@ -298,7 +299,6 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
> >  	msr	sctlr_el2, x0
> >  	isb
> >  0:
> > -
> >  	init_el2_hcr	HCR_HOST_NVHE_FLAGS
> >  	init_el2_state
> >
> > @@ -315,12 +315,15 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
> >
> >  	/* Set a sane SCTLR_EL1, the VHE way */
> >  	msr_s	SYS_SCTLR_EL12, x1
> > +	init_sctlr2_elx	12, x2
> >  	mov	x2, #BOOT_CPU_FLAG_E2H
> >  	b	3f
> >
> >  2:
> >  	msr	sctlr_el1, x1
> > +	init_sctlr2_elx	1, x2
> >  	mov	x2, xzr
> > +
> >  3:
> >  	mov	x0, #INIT_PSTATE_EL1
> >  	msr	spsr_el2, x0
>
> This is missing something: you should resynchronise SCTLR2_EL2 from
> SCTLR2_EL1 in __finalise_el2, rather than relying on whatever you've
> set in __init_el2_sctlr2.

Thanks. I'll add missing on enter_vhe().

--
Sincerely,
Yeoreum Yun