Message ID | 20190613161608.120838-1-jmattson@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | kvm: nVMX: Remove unnecessary sync_roots from handle_invept | expand |
On Thu, Jun 13, 2019 at 9:16 AM Jim Mattson <jmattson@google.com> wrote: > > When L0 is executing handle_invept(), the TDP MMU is active. Emulating > an L1 INVEPT does require synchronizing the appropriate shadow EPT > root(s), but a call to kvm_mmu_sync_roots in this context won't do > that. Similarly, the hardware TLB and paging-structure-cache entries > associated with the appropriate shadow EPT root(s) must be flushed, > but requesting a TLB_FLUSH from this context won't do that either. > > How did this ever work? KVM always does a sync_roots and TLB flush (in > the correct context) when transitioning from L1 to L2. That isn't the > best choice for nested VM performance, but it effectively papers over > the mistakes here. > > Remove the unnecessary operations and leave a comment to try to do > better in the future. > > Reported-by: Junaid Shahid <junaids@google.com> > Fixes: bfd0a56b90005f ("nEPT: Nested INVEPT") > Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> > Cc: Nadav Har'El <nyh@il.ibm.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Xinhao Xu <xinhao.xu@intel.com> > Cc: Yang Zhang <yang.z.zhang@Intel.com> > Cc: Gleb Natapov <gleb@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by Peter Shier <pshier@google.com> > Reviewed-by: Junaid Shahid <junaids@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/vmx/nested.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 1032f068f0b9..35621e73e726 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4670,13 +4670,11 @@ static int handle_invept(struct kvm_vcpu *vcpu) > > switch (type) { > case VMX_EPT_EXTENT_GLOBAL: > + case VMX_EPT_EXTENT_CONTEXT: > /* > - * TODO: track mappings and invalidate > - * single context requests appropriately > + * TODO: Sync the necessary shadow EPT roots here, rather than > + * at the next emulated VM-entry. > */ > - case VMX_EPT_EXTENT_CONTEXT: > - kvm_mmu_sync_roots(vcpu); > - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); > break; > default: > BUG_ON(1); > -- > 2.22.0.rc2.383.gf4fbbf30c2-goog > Ping.
On 13/06/19 18:16, Jim Mattson wrote: > When L0 is executing handle_invept(), the TDP MMU is active. Emulating > an L1 INVEPT does require synchronizing the appropriate shadow EPT > root(s), but a call to kvm_mmu_sync_roots in this context won't do > that. Similarly, the hardware TLB and paging-structure-cache entries > associated with the appropriate shadow EPT root(s) must be flushed, > but requesting a TLB_FLUSH from this context won't do that either. > > How did this ever work? KVM always does a sync_roots and TLB flush (in > the correct context) when transitioning from L1 to L2. That isn't the > best choice for nested VM performance, but it effectively papers over > the mistakes here. > > Remove the unnecessary operations and leave a comment to try to do > better in the future. > > Reported-by: Junaid Shahid <junaids@google.com> > Fixes: bfd0a56b90005f ("nEPT: Nested INVEPT") > Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> > Cc: Nadav Har'El <nyh@il.ibm.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Xinhao Xu <xinhao.xu@intel.com> > Cc: Yang Zhang <yang.z.zhang@Intel.com> > Cc: Gleb Natapov <gleb@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by Peter Shier <pshier@google.com> > Reviewed-by: Junaid Shahid <junaids@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/vmx/nested.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 1032f068f0b9..35621e73e726 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4670,13 +4670,11 @@ static int handle_invept(struct kvm_vcpu *vcpu) switch (type) { case VMX_EPT_EXTENT_GLOBAL: + case VMX_EPT_EXTENT_CONTEXT: /* - * TODO: track mappings and invalidate - * single context requests appropriately + * TODO: Sync the necessary shadow EPT roots here, rather than + * at the next emulated VM-entry. */ - case VMX_EPT_EXTENT_CONTEXT: - kvm_mmu_sync_roots(vcpu); - kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); break; default: BUG_ON(1);