From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 5/7] Nested VMX patch 5 Simplify fpu handling Date: Thu, 17 Dec 2009 11:10:38 +0200 Message-ID: <4B29F58E.4090407@redhat.com> References: <1260470309-7166-1-git-send-email-oritw@il.ibm.com> <1260470309-7166-2-git-send-email-oritw@il.ibm.com> <1260470309-7166-3-git-send-email-oritw@il.ibm.com> <1260470309-7166-4-git-send-email-oritw@il.ibm.com> <1260470309-7166-5-git-send-email-oritw@il.ibm.com> <1260470309-7166-6-git-send-email-oritw@il.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, benami@il.ibm.com, abelg@il.ibm.com, muli@il.ibm.com, aliguori@us.ibm.com, mdday@us.ibm.com To: oritw@il.ibm.com Return-path: Received: from mx1.redhat.com ([209.132.183.28]:30473 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758079AbZLQJKx (ORCPT ); Thu, 17 Dec 2009 04:10:53 -0500 In-Reply-To: <1260470309-7166-6-git-send-email-oritw@il.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/10/2009 08:38 PM, oritw@il.ibm.com wrote: > From: Orit Wasserman > > What exactly is the simplification? Is it intended to have a functional change? > --- > arch/x86/kvm/vmx.c | 27 +++++++++++++++++---------- > 1 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 8745d44..de1f596 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1244,8 +1244,6 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu) > u32 eb; > > eb = (1u<< PF_VECTOR) | (1u<< UD_VECTOR) | (1u<< MC_VECTOR); > - if (!vcpu->fpu_active) > - eb |= 1u<< NM_VECTOR; > /* > * Unconditionally intercept #DB so we can maintain dr6 without > * reading it every exit. > @@ -1463,10 +1461,6 @@ static void vmx_fpu_activate(struct kvm_vcpu *vcpu) > if (vcpu->fpu_active) > return; > vcpu->fpu_active = 1; > - vmcs_clear_bits(GUEST_CR0, X86_CR0_TS); > - if (vcpu->arch.cr0& X86_CR0_TS) > - vmcs_set_bits(GUEST_CR0, X86_CR0_TS); > - update_exception_bitmap(vcpu); > } > > static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu) > @@ -1474,8 +1468,6 @@ static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu) > if (!vcpu->fpu_active) > return; > vcpu->fpu_active = 0; > - vmcs_set_bits(GUEST_CR0, X86_CR0_TS); > - update_exception_bitmap(vcpu); > } > > static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) > @@ -2715,8 +2707,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > vmx_flush_tlb(vcpu); > vmcs_writel(GUEST_CR3, guest_cr3); > - if (vcpu->arch.cr0& X86_CR0_PE) > - vmx_fpu_deactivate(vcpu); > + if (vcpu->arch.cr0& X86_CR0_PE) { > + if (guest_cr3 != vmcs_readl(GUEST_CR3)) > + vmx_fpu_deactivate(vcpu); > + } > Why the added cr3 check? It may make sense, but it isn't a simplification. > static void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > @@ -5208,6 +5202,19 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu) > if (vcpu->arch.switch_db_regs) > get_debugreg(vcpu->arch.dr6, 6); > > + if (vcpu->fpu_active) { > + if (vmcs_readl(CR0_READ_SHADOW)& X86_CR0_TS) > + vmcs_set_bits(GUEST_CR0, X86_CR0_TS); > + else > + vmcs_clear_bits(GUEST_CR0, X86_CR0_TS); > + vmcs_write32(EXCEPTION_BITMAP, > + vmcs_read32(EXCEPTION_BITMAP)& ~(1u<< NM_VECTOR)); > + } else { > + vmcs_set_bits(GUEST_CR0, X86_CR0_TS); > + vmcs_write32(EXCEPTION_BITMAP, > + vmcs_read32(EXCEPTION_BITMAP) | (1u<< NM_VECTOR)); > + } > This is executed unconditionally, so the vmreads/vmwrites take place every time. Need to cache the previous fpu_active state and only take action if it changed. Since this is a large piece of code, move it to a function. Please post this as the first patch (or better, separately), so I can apply it independently of the rest. -- error compiling committee.c: too many arguments to function