Re: [PATCH 16/24] Implement VMLAUNCH and VMRESUME

[Avi Kivity <avi@redhat.com>]

  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