linux-kernel - Re: [PATCH v4 2/5] arm64: initialise SCTLR2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLW4A3rTcJvA0c+j@e133380.arm.com>
Date: Mon, 1 Sep 2025 16:13:07 +0100
From: Dave Martin <Dave.Martin@....com>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: catalin.marinas@....com, will@...nel.org, broonie@...nel.org,
	oliver.upton@...ux.dev, anshuman.khandual@....com, robh@...nel.org,
	james.morse@....com, mark.rutland@....com, joey.gouly@....com,
	ahmed.genidi@....com, kevin.brodsky@....com,
	scott@...amperecomputing.com, mbenes@...e.cz,
	james.clark@...aro.org, frederic@...nel.org, rafael@...nel.org,
	pavel@...nel.org, ryan.roberts@....com, suzuki.poulose@....com,
	maz@...nel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	kvmarm@...ts.linux.dev
Subject: Re: [PATCH v4 2/5] arm64: initialise SCTLR2_ELx register at boot time

Hi,

On Thu, Aug 21, 2025 at 06:24:05PM +0100, Yeoreum Yun wrote:
> The value of the SCTLR2_ELx register is UNKNOWN after reset.
> If the firmware initializes these registers properly, no additional
> initialization is required.
> However, in cases where they are not initialized correctly,
> initialize the SCTLR2_ELx registers during CPU/vCPU boot
> to prevent unexpected system behavior caused by invalid values.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
> ---

[...]

> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 23be85d93348..c25c2aed5125 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -738,6 +738,21 @@ alternative_endif
>  	set_sctlr sctlr_el2, \reg
>  .endm
>  
> +/* Set SCTLR2_ELx to the @reg value. */
> +.macro set_sctlr2_elx, el, reg, tmp
> +	mrs_s	\tmp, SYS_ID_AA64MMFR3_EL1
> +	ubfx	\tmp, \tmp, #ID_AA64MMFR3_EL1_SCTLRX_SHIFT, #4
> +	cbz	\tmp, .Lskip_sctlr2_\@
> +	.if	\el == 2
> +	msr_s	SYS_SCTLR2_EL2, \reg
> +	.elseif	\el == 12
> +	msr_s	SYS_SCTLR2_EL12, \reg
> +	.else
> +	msr_s	SYS_SCTLR2_EL1, \reg
> +	.endif
> +.Lskip_sctlr2_\@:
> +.endm
> +

You were right that just doing

	msr_s	SYS_SCTLR_\el, \reg

here doesn't work.  It looks like we rely on the C preprocessor to
expand the SYS_FOO_REG names to numeric sysreg encodings.  By the time
the assembler macros are expanded, it is too late to construct sysreg
names -- the C preprocessor only runs once, before the assembler.

So, your code here looks reasonable.

But, it will still silently do the wrong thing if \el is empty or
garbage, so it is probably worth adding an error check:

	.else
	.error "Bad EL \"\el\" in set_sctlr2_elx"
	.endif

Also, looking at all this again, the "1", "2" and "12" suffixes are not
really numbers: SYS_REG_EL02 would definitely not be the same thing as
SYS_REG_EL2 (although there is no _EL02 version of this particular
register).

So, can you use .ifc to do a string comparison instead?

If might be a good idea to migrate other macros that use an "el"
argument to do something similar -- although that probably doesn't
belong in this series.

The assembler seems to have no ".elseifc" directive, so you'll need
separate nested .ifc blocks:

	.ifc	\el,2
	msr_s	SYS_SCTLR2_EL2, \reg
	.else
	.ifc	\el,12
	msr_s	SYS_SCTLR2_EL12, \reg
	.else
	.ifc	\el,1
	msr_s	SYS_SCTLR2_EL1, \reg
	.else
	.error	"Bad EL \"\el\" in set_sctlr2_elx"
	.endif
	.endif
	.endif

(Note, I've not tested this approach.  If you can think of a better
way, please feel free to suggest.)

[...]

> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
> index 36e2d26b54f5..ac12f1b4f8e2 100644
> --- a/arch/arm64/kernel/hyp-stub.S
> +++ b/arch/arm64/kernel/hyp-stub.S
> @@ -144,7 +144,17 @@ SYM_CODE_START_LOCAL(__finalise_el2)
>  
>  .Lskip_indirection:
>  .Lskip_tcr2:
> +	mrs_s	x1, SYS_ID_AA64MMFR3_EL1
> +	ubfx	x1, x1, #ID_AA64MMFR3_EL1_SCTLRX_SHIFT, #4
> +	cbz	x1, .Lskip_sctlr2
> +	mrs_s	x1, SYS_SCTLR2_EL12
> +	msr_s	SYS_SCTLR2_EL1, x1
>  
> +	// clean SCTLR2_EL1
> +	mov_q	x1, INIT_SCTLR2_EL1
> +	msr_s	SYS_SCTLR2_EL12, x1

I'm still not sure why we need to do this.  The code doesn't seem to
clean up by the EL1 value of any other register -- or have I missed
something?

We have already switched to EL2, via the HVC call that jumped to
__finalise_el2.  We won't run at EL1 again unless KVM starts a guest --
but in that case, it's KVM's responsibility to set up the EL1 registers
before handing control to the guest.

In any case, is SCTLR2_EL1 ever set to anything except INIT_SCTLR2_EL1
before we get here?

[...]

Cheers
---Dave