lkml.org 
[lkml]   [2021]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V2] KVM: X86: fix tlb_flush_guest()
From
Date
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

\
 
 \ /
  Last update: 2021-06-08 19:36    [W:1.647 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site