From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 2/3] KVM: VMX: Simplify pdptr and cr3 management Date: Tue, 2 Jun 2009 17:22:43 +0800 Message-ID: <200906021722.44103.sheng@linux.intel.com> References: <1243862524-22120-1-git-send-email-avi@redhat.com> <1243862524-22120-3-git-send-email-avi@redhat.com> Mime-Version: 1.0 Content-Type: Text/Plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Marcelo Tosatti , Joerg Roedel To: Avi Kivity Return-path: Received: from mga06.intel.com ([134.134.136.21]:43255 "EHLO orsmga101.jf.intel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750751AbZFBJWq convert rfc822-to-8bit (ORCPT ); Tue, 2 Jun 2009 05:22:46 -0400 In-Reply-To: <1243862524-22120-3-git-send-email-avi@redhat.com> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: On Monday 01 June 2009 21:22:02 Avi Kivity wrote: > Instead of reading the PDPTRs from memory after every exit (which is = slow > and wrong, as the PDPTRs are stored on the cpu), sync the PDPTRs from > memory to the VMCS before entry, and from the VMCS to memory after ex= it. > Do the same for cr3. > Thanks for fixing! After review my original code, I found a potential bug. For SDM 3B have= this: 23.3.4 Saving Non-Register State =2E.. If the logical processor supports the 1-setting of the =E2=80=9Cenable = EPT=E2=80=9D VM- execution control, values are saved into the four (4) PDPTE fields as f= ollows: =E2=80=94 If the =E2=80=9Cenable EPT=E2=80=9D VM-execution control is 1= and the logical processor was using PAE paging at the time of the VM exit, the PDPTE values currently= in use are saved: =E2=80=A2 The values saved into bits 11:9 of each of the fields is unde= fined. =E2=80=A2 If the value saved into one of the fields has bit 0 (present)= clear, the=20 value saved into bits 63:1 of that field is undefined. That value need = not correspond to the value that was loaded by VM entry or to any value tha= t might have been loaded in VMX non-root operation. =E2=80=A2 If the value saved into one of the fields has bit 0 (present)= set, the value saved into bits 63:12 of the field is a guest-physical address. =E2=80=94 If the =E2=80=9Cenable EPT=E2=80=9D VM-execution control is 0= or the logical processor was=20 not using PAE paging at the time of the VM exit, the values saved are=20 undefined. But drop the ept_load_pdptrs() when exit and add it in cr0 handling res= ult in=20 Windows PAE guest hang on boot. I am checking it now. Any thoughts?... --=20 regards Yang, Sheng > Signed-off-by: Avi Kivity > --- > arch/x86/kvm/vmx.c | 21 +++++++++++++++------ > 1 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 5607de8..1783606 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1538,10 +1538,6 @@ static void vmx_decache_cr4_guest_bits(struct > kvm_vcpu *vcpu) static void ept_load_pdptrs(struct kvm_vcpu *vcpu) > { > if (is_paging(vcpu) && is_pae(vcpu) && !is_long_mode(vcpu)) { > - if (!load_pdptrs(vcpu, vcpu->arch.cr3)) { > - printk(KERN_ERR "EPT: Fail to load pdptrs!\n"); > - return; > - } > vmcs_write64(GUEST_PDPTR0, vcpu->arch.pdptrs[0]); > vmcs_write64(GUEST_PDPTR1, vcpu->arch.pdptrs[1]); > vmcs_write64(GUEST_PDPTR2, vcpu->arch.pdptrs[2]); > @@ -1549,6 +1545,16 @@ static void ept_load_pdptrs(struct kvm_vcpu *v= cpu) > } > } > > +static void ept_save_pdptrs(struct kvm_vcpu *vcpu) > +{ > + if (is_paging(vcpu) && is_pae(vcpu) && !is_long_mode(vcpu)) { > + vcpu->arch.pdptrs[0] =3D vmcs_read64(GUEST_PDPTR0); > + vcpu->arch.pdptrs[1] =3D vmcs_read64(GUEST_PDPTR1); > + vcpu->arch.pdptrs[2] =3D vmcs_read64(GUEST_PDPTR2); > + vcpu->arch.pdptrs[3] =3D vmcs_read64(GUEST_PDPTR3); > + } > +} > + > static void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); > > static void ept_update_paging_mode_cr0(unsigned long *hw_cr0, > @@ -1642,7 +1648,6 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, > unsigned long cr3) if (enable_ept) { > eptp =3D construct_eptp(cr3); > vmcs_write64(EPT_POINTER, eptp); > - ept_load_pdptrs(vcpu); > guest_cr3 =3D is_paging(vcpu) ? vcpu->arch.cr3 : > VMX_EPT_IDENTITY_PAGETABLE_ADDR; > } > @@ -3199,7 +3204,7 @@ static int vmx_handle_exit(struct kvm_run *kvm_= run, > struct kvm_vcpu *vcpu) * to sync with guest real CR3. */ > if (enable_ept && is_paging(vcpu)) { > vcpu->arch.cr3 =3D vmcs_readl(GUEST_CR3); > - ept_load_pdptrs(vcpu); > + ept_save_pdptrs(vcpu); > } > > if (unlikely(vmx->fail)) { > @@ -3376,6 +3381,10 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu= , > struct kvm_run *kvm_run) { > struct vcpu_vmx *vmx =3D to_vmx(vcpu); > > + if (enable_ept && is_paging(vcpu)) { > + vmcs_writel(GUEST_CR3, vcpu->arch.cr3); > + ept_load_pdptrs(vcpu); > + } > /* Record the guest's net vcpu time for enforced NMI injections. */ > if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked)) > vmx->entry_time =3D ktime_get();