Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751612AbdG0RVS (ORCPT ); Thu, 27 Jul 2017 13:21:18 -0400 Received: from mail-qk0-f176.google.com ([209.85.220.176]:38721 "EHLO mail-qk0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751441AbdG0RVQ (ORCPT ); Thu, 27 Jul 2017 13:21:16 -0400 MIME-Version: 1.0 In-Reply-To: <1501163686-13648-1-git-send-email-pbonzini@redhat.com> References: <1501163686-13648-1-git-send-email-pbonzini@redhat.com> From: David Matlack Date: Thu, 27 Jul 2017 10:20:45 -0700 Message-ID: Subject: Re: [PATCH] KVM: nVMX: do not pin the VMCS12 To: Paolo Bonzini Cc: "linux-kernel@vger.kernel.org" , kvm list , Jim Mattson Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3949 Lines: 100 On Thu, Jul 27, 2017 at 6:54 AM, Paolo Bonzini wrote: > Since the current implementation of VMCS12 does a memcpy in and out > of guest memory, we do not need current_vmcs12 and current_vmcs12_page > anymore. current_vmptr is enough to read and write the VMCS12. This patch also fixes dirty tracking (memslot->dirty_bitmap) of the VMCS12 page by using kvm_write_guest. nested_release_page() only marks the struct page dirty. > > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 23 ++++++----------------- > 1 file changed, 6 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index b37161808352..142f16ebdca2 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -416,9 +416,6 @@ struct nested_vmx { > > /* The guest-physical address of the current VMCS L1 keeps for L2 */ > gpa_t current_vmptr; > - /* The host-usable pointer to the above */ > - struct page *current_vmcs12_page; > - struct vmcs12 *current_vmcs12; > /* > * Cache of the guest's VMCS, existing outside of guest memory. > * Loaded from guest memory during VMPTRLD. Flushed to guest > @@ -7183,10 +7180,6 @@ static inline void nested_release_vmcs12(struct vcpu_vmx *vmx) > if (vmx->nested.current_vmptr == -1ull) > return; > > - /* current_vmptr and current_vmcs12 are always set/reset together */ > - if (WARN_ON(vmx->nested.current_vmcs12 == NULL)) > - return; > - > if (enable_shadow_vmcs) { > /* copy to memory all shadowed fields in case > they were modified */ > @@ -7199,13 +7192,11 @@ static inline void nested_release_vmcs12(struct vcpu_vmx *vmx) > vmx->nested.posted_intr_nv = -1; > > /* Flush VMCS12 to guest memory */ > - memcpy(vmx->nested.current_vmcs12, vmx->nested.cached_vmcs12, > - VMCS12_SIZE); > + kvm_vcpu_write_guest_page(&vmx->vcpu, > + vmx->nested.current_vmptr >> PAGE_SHIFT, > + vmx->nested.cached_vmcs12, 0, VMCS12_SIZE); Have you hit any "suspicious RCU usage" error messages during VM teardown with this patch? We did when we replaced memcpy with kvm_write_guest a while back. IIRC it was due to kvm->srcu not being held in one of the teardown paths. kvm_write_guest() expects it to be held in order to access memslots. We fixed this by skipping the VMCS12 flush during VMXOFF. I'll send that patch along with a few other nVMX dirty tracking related patches I've been meaning to get upstreamed. > > - kunmap(vmx->nested.current_vmcs12_page); > - nested_release_page(vmx->nested.current_vmcs12_page); > vmx->nested.current_vmptr = -1ull; > - vmx->nested.current_vmcs12 = NULL; > } > > /* > @@ -7623,14 +7614,13 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu) > } > > nested_release_vmcs12(vmx); > - vmx->nested.current_vmcs12 = new_vmcs12; > - vmx->nested.current_vmcs12_page = page; > /* > * Load VMCS12 from guest memory since it is not already > * cached. > */ > - memcpy(vmx->nested.cached_vmcs12, > - vmx->nested.current_vmcs12, VMCS12_SIZE); > + memcpy(vmx->nested.cached_vmcs12, new_vmcs12, VMCS12_SIZE); > + kunmap(page); + nested_release_page_clean(page); > + > set_current_vmptr(vmx, vmptr); > } > > @@ -9354,7 +9344,6 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id) > > vmx->nested.posted_intr_nv = -1; > vmx->nested.current_vmptr = -1ull; > - vmx->nested.current_vmcs12 = NULL; > > vmx->msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED; > > -- > 1.8.3.1 >