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

[Pingfan Liu]

On Wed, Aug 26, 2020 at 1:56 AM James Morse <james.morse at arm.com> wrote:
>
> 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.
Yes, I will clean them.
>
>
>
> >       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'.
Yes, it is a more accurate view to consider and describe this issue.

>
>
>
> >     "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)
Yes, I will use this term.

>
>
> >     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)
Ditto.
>
>
> >     "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.'
OK, I will use this description.

>
> 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.
> }
Yes, I totally agree with you. And I revisit the manual so I can
describe this area more precisely in future.

>
>
> >     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 ?
Sure. I have sent a separate patch for the document issue. I will fold
the two patches into a series.

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

Thanks,
Pingfan