On Wed, Aug 26, 2020 at 1:56 AM James Morse [off-list ref] wrote:

Hi Pingfan,

On 12/08/2020 15:21, Pingfan Liu wrote:

quoted

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.

quoted

      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:

quoted

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

quoted

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

quoted

    -2. EL2&0 host

(Booted at EL2, using VHE)

Yes, I will use this term.

quoted

    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.

quoted

    -3. EL1&0 host,

(Booted at EL2, not using VHE)

Ditto.

quoted

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

quoted

     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.

quoted

    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

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