From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 16/24] Implement VMLAUNCH and VMRESUME Date: Sun, 26 Sep 2010 14:56:54 +0200 Message-ID: <4C9F4316.5090906@redhat.com> References: <1276431753-nyh@il.ibm.com> <201006131230.o5DCUk2i013070@rice.haifa.ibm.com> <4C161569.3000602@redhat.com> <20100926111412.GC29641@fermat.math.technion.ac.il> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44013 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757620Ab0IZM5E (ORCPT ); Sun, 26 Sep 2010 08:57:04 -0400 In-Reply-To: <20100926111412.GC29641@fermat.math.technion.ac.il> Sender: kvm-owner@vger.kernel.org List-ID: On 09/26/2010 01:14 PM, Nadav Har'El wrote: > On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 16/24] Implement VMLAUNCH and VMRESUME": > > >+ vmx_set_cr0(vcpu, > > >+ (get_shadow_vmcs(vcpu)->guest_cr0& > > >+ ~get_shadow_vmcs(vcpu)->cr0_guest_host_mask) | > > >+ (get_shadow_vmcs(vcpu)->cr0_read_shadow& > > >+ get_shadow_vmcs(vcpu)->cr0_guest_host_mask)); > > >+ > > >+ /* However, vmx_set_cr0 incorrectly enforces KVM's relationship > > >between > > >+ * GUEST_CR0 and CR0_READ_SHADOW, e.g., that the former is the same > > >as > > >+ * the latter with with TS added if !fpu_active. We need to take the > > >+ * actual GUEST_CR0 that L1 wanted, just with added TS if !fpu_active > > >+ * like KVM wants (for the "lazy fpu" feature, to avoid the costly > > >+ * restoration of fpu registers until the FPU is really used). > > >+ */ > > >+ vmcs_writel(GUEST_CR0, get_shadow_vmcs(vcpu)->guest_cr0 | > > >+ (vcpu->fpu_active ? 0 : X86_CR0_TS)); > > > > > > > Please update vmx_set_cr0() instead. > > How would you like that I do that? > I could split vmx_set_cr0(vcpu, cr0) into a __vmx_set_cr0(vcpu, cr0, hw_cr0) > and vmx_set_cr0 that calls it. Is this what you had in mind? Won't it be > a little ugly? I agree, though, that it will avoid the vmwriting GUEST_CR0 > twice in the nested case. Just move the extra calculations into vmx_set_cr0(). Check if you're in nested mode, and if so apply cr0_guest_host_mask. The vmlaunch/vmresume code becomes kvm_set_cr0(vcpu, get_shadow_vmcs(vcpu)->guest_cr0). > > >+ /* we have to set the X86_CR0_PG bit of the cached cr0, because > > >+ * kvm_mmu_reset_context enables paging only if X86_CR0_PG is set in > > >+ * CR0 (we need the paging so that KVM treat this guest as a paging > > >+ * guest so we can easly forward page faults to L1.) > > >+ */ > > >+ vcpu->arch.cr0 |= X86_CR0_PG; > > > > > > > Since this version doesn't support unrestricted nested guests, cr0.pg > > will be already set or we will have failed vmentry. > > I believe without this "hack", things didn't work properly during boot of > L2, when cr0_read_shadow.pg was not yet set. I think PG is set in guest_cr0, > but not in cr0_read_shadow, which is what vcpu->arch.cr0 caches. I don't see how vcpu->arch.cr0 can cache cr0_read_shadow. All the mmu calculations depend on vcpu->arch.cr0, which must be what the processor uses for translations. cr0_read_shadow is only use to emulate read access to cr0 (note we need to both update the real CR0_READ_SHADOW, and to consider the virtual CR0_READ_SHADOW when emulating). -- error compiling committee.c: too many arguments to function