Hi Dave,

[...]

quoted

quoted

quoted

quoted

quoted

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

[...]

quoted

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

I think I missed the SCTLR_EL1 reset in the idmap code after the
enter_vhe label.

Actually, I'm not sure whether there is any architectural reason for
setting SCTLR_EL1 to INIT_SCTLR_EL1_MMU_OFF here.  "for good measure"
suggests that it felt like a good idea but there was no known reason
for it.  The commit message for the original patch doesn't offer an
explanation -- maybe Marc can remember.

This might be a defence against speculative translation table walks
using the EL1&0 regime (but the architecture says [RNRJPP]: "If an
implementation is executing at EL3 or EL2, the PE is not permitted to
use the registers associated with the EL1&0 translation regime to
speculatively access memory or translation tables.")  So it shouldn't
really matter, but in case buggy CPUs don't implement this rule
properly it may be a good idea to turn the stage1 MMU off just in case.

Thanks for great deep insight :D.

Since it's there, though, it probably does make sense to reinitialise
SCTLR2_EL1 at the same time -- but can you move this so that it is next
to the SCTLR_EL1 reinitialisation?  Otherwise, the purpose of
reinitialising SCTLR2_EL1 is unclear.  It really should come under the
same "for good measure" justification as the SCTLR_EL1 reset.

Okay.

However, I don't think this has anything to do with putting things into
a clean state for VMs.  KVM defines the reset state for all the _EL1
regs explicitly -- failing to do that would be a bug in KVM.

(See arch/arm64/kvm/sys_regs.c : sys_reg_descs[], kvm_reset_sys_regs().)

Right. I've missed the reset sysregs when kvm is launching.

Thanks!

--
Sincerely,
Yeoreum Yun