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

["Nadav Har'El" <nyh@math.technion.ac.il>]

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.

> >+	/* 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.

> >+	if (enable_ept&&  !nested_cpu_has_vmx_ept(vcpu)) {
> >   
> 
> We don't support nested ept yet, yes?

Right. It seems like this (and a couple of other places) were left from
our internal codebase (which did have nested ept). I'll clean it up.


-- 
Nadav Har'El                        |      Monday, Sep 20 2010, 12 Tishri 5771
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |The two rules for success are: 1. Never
http://nadav.harel.org.il           |tell them everything you know.