On Fri, May 28, 2021, Lai Jiangshan wrote:

On 2021/5/28 00:13, Sean Christopherson wrote:

quoted

And making a request won't work without revamping the order of request handling
in vcpu_enter_guest(), e.g. KVM_REQ_MMU_RELOAD and KVM_REQ_MMU_SYNC are both
serviced before KVM_REQ_STEAL_UPDATE.

Yes, it just fixes the said problem in the simplest way.
I copied KVM_REQ_MMU_RELOAD from kvm_handle_invpcid(INVPCID_TYPE_ALL_INCL_GLOBAL).
(If the guest is not preempted, it will call invpcid_flush_all() and will be handled
by this way)

The problem is that record_steal_time() is called after KVM_REQ_MMU_RELOAD
in vcpu_enter_guest() and so the reload request won't be recognized until the
next VM-Exit.  It works for kvm_handle_invpcid() because vcpu_enter_guest() is
guaranteed to run between the invcpid code and VM-Enter.

The improvement code will go later, and will not be backported.

I would argue that introducing a potential performance regression is in itself a
bug.  IMO, going straight to kvm_mmu_sync_roots() is not high risk.

The proper way to flush guest is to use code in

https://lore.kernel.org/lkml/20210525213920.3340-1-jiangshanlai@gmail.com/ (local)
as:
+		kvm_mmu_sync_roots(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu); //or just call flush_current directly
+		for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++)
+			vcpu->arch.mmu->prev_roots[i].need_sync = true;

If need_sync patch is not accepted, we can just use kvm_mmu_sync_roots(vcpu)
to keep the current pagetable and use kvm_mmu_free_roots() to free all the other
roots in prev_roots.

I like the idea, I just haven't gotten around to reviewing that patch yet.

quoted

Cleaning up and documenting the MMU related requests is on my todo list, but the
immediate fix should be tiny and I can do my cleanups on top.

I believe the minimal fix is:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 81ab3b8f22e5..b0072063f9bf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c

@@ -3072,6 +3072,9 @@ static void kvm_vcpu_flush_tlb_all(struct kvm_vcpu *vcpu)
  static void kvm_vcpu_flush_tlb_guest(struct kvm_vcpu *vcpu)
  {
         ++vcpu->stat.tlb_flush;
+
+       if (!tdp_enabled)
+               kvm_mmu_sync_roots(vcpu);

it doesn't handle prev_roots which are also needed as
shown in kvm_handle_invpcid(INVPCID_TYPE_ALL_INCL_GLOBAL).

Ya, I belated realized this :-)

quoted

         static_call(kvm_x86_tlb_flush_guest)(vcpu);

For tdp_enabled, I think it is better to use kvm_x86_tlb_flush_current()
to make it consistent with other shadowpage code.

quoted

  }