Re: [PATCH] arm64/relocate_kernel: remove redundant but misleading code

[James Morse <james.morse@arm.com>]

Hi Pingfan,

On 12/08/2020 15:21, Pingfan Liu wrote:
> I have read the arm64/kvm code, and been more clear about it now.
> 
> What do you think about the following commit log (just describe the fact)
> 
>     arm64/relocate_kernel: remove redundant code
> 
>     The kexec switch sequence looks like the following:
>       SYM_CODE_START(__cpu_soft_restart)
>               /* Clear sctlr_el1 flags. */
>               mrs     x12, sctlr_el1
>               mov_q   x13, SCTLR_ELx_FLAGS
>               bic     x12, x12, x13
>               pre_disable_mmu_workaround
>               msr     sctlr_el1, x12
>               isb
> 
>               cbz     x0, 1f                          // el2_switch?
>               mov     x0, #HVC_SOFT_RESTART
>               hvc     #0                              // no return
> 
>       1:      mov     x8, x1                          // entry
>               mov     x0, x2                          // arg0
>               mov     x1, x3                          // arg1
>               mov     x2, x4                          // arg2
>               br      x8
>       SYM_CODE_END(__cpu_soft_restart)

I think this is far to much to put in the commit message!

It should be a summary. We can always checkout the whole tree at the point the commit was
made to look at functions like this, we don't need to preserve them in the commit log.



>       SYM_CODE_START(arm64_relocate_new_kernel)
>         ...
>         pre_disable_mmu_workaround
>         msr     sctlr_el2, x0
>         ...
> 
>     As for the shutdown of MMU and clearing of I+C bits, three cases should be
>     considered:

>     -1. Guest

I don't think host/guest matter here. This one is really 'booted at EL1'.



>     "msr sctlr_el1, x12" is enough to turn off EL1&0 translation regime and
>     clear I+C bits.

>     -2. EL2&0 host

(Booted at EL2, using VHE)


>     According to "D12.2.101 SCTLR_EL2, System Control Register (EL2)" in
>     "ARM Architecture Reference Manual", actually, EL2&0 host accesses
>     to SCTLR_EL2 when using mnemonic SCTLR_EL1.
>     So "msr sctlr_el1, x12" is enough to turn off MMU and clear I+C bits.


>     -3. EL1&0 host,

(Booted at EL2, not using VHE)


>     "msr sctlr_el1, x12" turns off EL1&0 translation regime.

>      As for EL2 regime, el2_setup doesn't turn on EL2 regime
A regime isn't something you turn on, I think this should just be 'The hyp-stub doesn't
enable the MMU, see el2_setup.'

digression {
If you think about the EL1&0 regime when stage2 is enabled, is the regime only 'on' when
the stage1 MMU is enabled? Not really, when the stage1 MMU is disabled it outputs VA==IPA,
and the stage2 MMU translates this to PA. The whole thing belongs to the translation EL1&0
regime.
}


>     and set those bits , and KVM clears
>     them when it's unloaded, or has a HVC_SOFT_RESTART call.
> 
>     As a conclusion, the shutdown of MMU and clearing I+C bits in
>     SYM_CODE_START(arm64_relocate_new_kernel) is redundant.


This certainly covers all the cases.

As kexec is now depending on this behaviour of KVM, how do you feel about updating the
document at Documentation/virt/kvm/arm/hyp-abi.rst ?

I think all it needs is a "Clear the I+C bits (arm64 only)" under HVC_SOFT_RESTART.


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel