Thread (21 messages) 21 messages, 5 authors, 2021-06-08

Re: [PATCH V2] KVM: X86: fix tlb_flush_guest()

From: Paolo Bonzini <pbonzini@redhat.com>
Date: 2021-06-08 17:36:17
Also in: lkml

On 08/06/21 02:03, Sean Christopherson wrote:
quoted hunk ↗ jump to hunk
On Tue, Jun 08, 2021, Maxim Levitsky wrote:
quoted
So this patch *does* fix the windows boot without TDP!
Woot!
quoted
Tested-by: Maxim Levitsky <redacted>
Reviewed-by: Maxim Levitsky <redacted>
Lai,

I have a reworded version of your patch sitting in a branch that leverages this
path to fix similar bugs and do additional cleanup.  Any objection to me gathering
Maxim's tags and posting the version below?  I'm more than happy to hold off if
you'd prefer to send your own version, but I don't want to send my own series
without this fix as doing so would introduce bugs.

Thanks!

Author: Lai Jiangshan [off-list ref]
Date:   Tue Jun 1 01:22:56 2021 +0800

     KVM: x86: Unload MMU on guest TLB flush if TDP disabled to force MMU sync
     
     When using shadow paging, unload the guest MMU when emulating a guest TLB
     flush to all roots are synchronized.  From the guest's perspective,
     flushing the TLB ensures any and all modifications to its PTEs will be
     recognized by the CPU.
     
     Note, unloading the MMU is overkill, but is done to mirror KVM's existing
     handling of INVPCID(all) and ensure the bug is squashed.  Future cleanup
     can be done to more precisely synchronize roots when servicing a guest
     TLB flush.
     
     If TDP is enabled, synchronizing the MMU is unnecessary even if nested
     TDP is in play, as a "legacy" TLB flush from L1 does not invalidate L1's
     TDP mappgins.  For EPT, an explicit INVEPT is required to invalidate
     guest-physical mappings.  For NPT, guest mappings are always tagged with
     an ASID and thus can only be invalidated via the VMCB's ASID control.
     
     This bug has existed since the introduction of KVM_VCPU_FLUSH_TLB, but
     was only recently exposed after Linux guests stopped flushing the local
     CPU's TLB prior to flushing remote TLBs (see commit 4ce94eabac16,
     "x86/mm/tlb: Flush remote and local TLBs concurrently").
     
     Tested-by: Maxim Levitsky [off-list ref]
     Reviewed-by: Maxim Levitsky [off-list ref]
     Fixes: f38a7b75267f ("KVM: X86: support paravirtualized help for TLB shootdowns")
     Signed-off-by: Lai Jiangshan [off-list ref]
     [sean: massaged comment and changelog]
     Signed-off-by: Sean Christopherson [off-list ref]
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1cd6d4685932..3b02528d5ee8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3072,6 +3072,18 @@ 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) {
+               /*
+                * Unload the entire MMU to force a sync of the shadow page
+                * tables.  A TLB flush on behalf of the guest is equivalent
+                * to INVPCID(all), toggling CR4.PGE, etc...  Note, loading the
+                * MMU will also do an actual TLB flush.
+                */
I slightly prefer it in the opposite order (first why, then how):

                /*
                 * A TLB flush on behalf of the guest is equivalent to
                 * INVPCID(all), toggling CR4.PGE, etc., which requires
                 * a forced sync of the shadow page tables.  Unload the
                 * entire MMU here and the subsequent load will sync the
                 * shadow page tables, and also flush the TLB.
                 */

Queued, thanks all!  It's great that this fixes an actual bug.

Paolo
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help