From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 25/28] nVMX: Further fixes for lazy FPU loading Date: Thu, 09 Dec 2010 15:05:30 +0200 Message-ID: <4D00D41A.2000900@redhat.com> References: <1291827596-nyh@il.ibm.com> <201012081712.oB8HCkpY008822@rice.haifa.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, gleb@redhat.com To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:20033 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755142Ab0LINFf (ORCPT ); Thu, 9 Dec 2010 08:05:35 -0500 In-Reply-To: <201012081712.oB8HCkpY008822@rice.haifa.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/08/2010 07:12 PM, Nadav Har'El wrote: > KVM's "Lazy FPU loading" means that sometimes L0 needs to set CR0.TS, even > if a guest didn't set it. Moreover, L0 must also trap CR0.TS changes and > NM exceptions, even if we have a guest hypervisor (L1) who didn't want these > traps. And of course, conversely: If L1 wanted to trap these events, we > must let it, even if L0 is not interested in them. > > This patch fixes some existing KVM code (in update_exception_bitmap(), > vmx_fpu_activate(), vmx_fpu_deactivate()) to do the correct merging of L0's > and L1's needs. Note that handle_cr() was already fixed in the above patch, > and that new code in introduced in previous patches already handles CR0 > correctly (see prepare_vmcs02(), prepare_vmcs12(), and nested_vmx_vmexit()). > > > @@ -1415,8 +1424,19 @@ static void vmx_fpu_activate(struct kvm_ > cr0&= ~(X86_CR0_TS | X86_CR0_MP); > cr0 |= kvm_read_cr0_bits(vcpu, X86_CR0_TS | X86_CR0_MP); > vmcs_writel(GUEST_CR0, cr0); > - update_exception_bitmap(vcpu); > vcpu->arch.cr0_guest_owned_bits = X86_CR0_TS; > + if (is_guest_mode(vcpu)) { > + /* While we (L0) no longer care about NM exceptions or cr0.TS > + * changes, our guest hypervisor (L1) might care in which case > + * we must trap them for it. > + */ > + u32 eb = vmcs_read32(EXCEPTION_BITMAP)& ~(1u<< NM_VECTOR); > + struct vmcs_fields *vmcs12 = get_vmcs12_fields(vcpu); > + eb |= vmcs12->exception_bitmap; > + vcpu->arch.cr0_guest_owned_bits&= ~vmcs12->cr0_guest_host_mask; > + vmcs_write32(EXCEPTION_BITMAP, eb); > + } else > + update_exception_bitmap(vcpu); Isn't update_exception_bitmap() sufficient for both cases? > vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits); > } > > @@ -1424,12 +1444,24 @@ static void vmx_decache_cr0_guest_bits(s > > static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu) > { > + /* Note that there is no vcpu->fpu_active = 0 here. The caller must > + * set this *before* calling this function. > + */ > vmx_decache_cr0_guest_bits(vcpu); > vmcs_set_bits(GUEST_CR0, X86_CR0_TS | X86_CR0_MP); > - update_exception_bitmap(vcpu); > + vmcs_write32(EXCEPTION_BITMAP, > + vmcs_read32(EXCEPTION_BITMAP) | (1u<< NM_VECTOR)); Why not fold the logic into update_exception_bitmap()? > vcpu->arch.cr0_guest_owned_bits = 0; > vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits); > - vmcs_writel(CR0_READ_SHADOW, vcpu->arch.cr0); > + if (is_guest_mode(vcpu)) > + /* Unfortunately in nested mode we play with arch.cr0's PG > + * bit, so we musn't copy it all, just the relevant TS bit > + */ > + vmcs_writel(CR0_READ_SHADOW, > + (vmcs_readl(CR0_READ_SHADOW)& ~X86_CR0_TS) | > + (vcpu->arch.cr0& X86_CR0_TS)); > + else > + vmcs_writel(CR0_READ_SHADOW, vcpu->arch.cr0); Didn't you have a nice guest_readable_cr0() function that did this? > } > > static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) -- error compiling committee.c: too many arguments to function