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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aLbJeQf9LKXFTxzS@e133380.arm.com>
Date: Tue, 2 Sep 2025 11:39:53 +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 Mon, Sep 01, 2025 at 07:29:28PM +0100, Yeoreum Yun wrote:
> Hi Dave,
> 
> > 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 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

[...]

> > 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.)
> 
> Thanks for your suggestion. Let me test and check about this idea could
> be applied in other macro too :D
> (But as you mention, I'll apply this for SCTLR2 in other patchset).

Ack, let me know how it goes.

It is probably not worth doing this if the changes become complicated,
though.

[...]

> > > 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?
> 
> Regardless of VHE and nVHE, between init_kernel_el() and finalise_el2()
> the kernel modifies SCTLR_EL1/SCTLR2_EL1 (since el level in this period
> is EL1).
> and at the end of finalise_el2(), kernel switches to el2 and
> if VHE, it writes the SCTLR_EL1/SCTLR2_EL1 to SCTLR_EL2/SCTLR2_EL2 and
> initialize the SCTLR_EL1/SCTLR2_EL1.
> 
> Although there is no code to modify SCTLR2_EL1 between this period,
> as SCTLR1_ELx, I initialize the SCTLR2_EL1 as init value for the future
> usage.

I think that we don't need to ensure that all sysregs are cleanly
initialised; only those that can affect execution in some way need to
be initialised.

Once we are running at EL2 with VHE, we don't switch back to EL1, so
the _EL1 control registers don't affect execution any more.


If we did have to clean up the _EL1 registers, then this code would
need to clean up all the other regs too, but I can't see clean-up for
anything other than SCTLR2_EL1 here.  Is there some cleanup code
elsewhere that I have missed?

Cheers
---Dave