Re: [PATCH v4 2/5] arm64: initialise SCTLR2_ELx register at boot time

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

Hi Dave,

[...]

> > > > 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

When I look at init_el2(), it returns to EL1 via:

  mov x0, #INIT_PSTATE_EL1
  msr spsr_el2, x0
  ...
  eret

In other words, from init_kernel_el() through finalise_el2(),
all system-register accesses are made at EL1 (i.e., SYS_REG_EL1).
During this period, it appears that only SCTLR_EL1 is modified,
so the code only needs to care about the accessed register — SCTLR_EL1.

That’s why SCTLR_EL1 is reinitialised at the end of finalise_el2().
Otherwise, the MMU bit might remain enabled, which could cause errors later
when launching a VM under VHE.

However, the idea behind this patch is to initialise SCTLR2_ELx
the same way as SCTLR_ELx.
I’m not sure whether SCTLR2_ELx is modified during this period.
If it is (now or in the future),
it should be cleared/reinitialised just like SCTLR_EL1.

This patch is based on the assumption that there may be modifications to
SCTLR2_ELx during this period. So it isn’t about other system registers;
it’s about the register actually used during this period.

Am I missing anything?

Thanks!

--
Sincerely,
Yeoreum Yun