Messages in this thread | | | Subject | Re: [PATCH V2] KVM: X86: fix tlb_flush_guest() | From | Paolo Bonzini <> | Date | Tue, 8 Jun 2021 19:36:10 +0200 |
| |
On 08/06/21 02:03, Sean Christopherson wrote: > On Tue, Jun 08, 2021, Maxim Levitsky wrote: >> So this patch *does* fix the windows boot without TDP! > > Woot! > >> Tested-by: Maxim Levitsky <mlevitsk@redhat.com> >> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > 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 <laijs@linux.alibaba.com> > 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 <mlevitsk@redhat.com> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > Fixes: f38a7b75267f ("KVM: X86: support paravirtualized help for TLB shootdowns") > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> > [sean: massaged comment and changelog] > Signed-off-by: Sean Christopherson <seanjc@google.com> > > 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
| |