Re: [PATCH 4/24] Allow setting the VMXE bit in CR4

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

On Tue, Jun 15, 2010, Gleb Natapov wrote about "Re: [PATCH 4/24] Allow setting the VMXE bit in CR4":
> On Sun, Jun 13, 2010 at 03:24:37PM +0300, Nadav Har'El wrote:
> > This patch allows the guest to enable the VMXE bit in CR4, which is a
> > prerequisite to running VMXON.
>..
> > --- .before/arch/x86/kvm/x86.c	2010-06-13 15:01:28.000000000 +0300
>..
> > +	if (cr4 & x86_cr4_vmxe && !nested)
> >  		return 1;

After I moved back the "nested" option from x86.c to vmx.c, this created a
problem in this patch, because kvm_set_cr4 (in x86.c) can no longer test the
"nested" option as above to decide if the VMXE bit is to be allowed or not.

But this setback was actually an opportunity to do this testing more
correctly. I've changed kvm_x86_ops->set_cr4() to return 1 when a #GP should
be thrown (like in __kvm_set_cr4()). SVM's set_cr4() now always refuses to set
the VMXE bit, while VMX's set_cr4() refuses to set or unset it as appropriate
(it cannot be set if "nested" is not on, and cannot be unset after VMXON).

> We shouldn't be able to clear X86_CR4_VMXE after VMXON.

You're absolutely right. I fixed that too in the fixed patch below.


----
Subject: [PATCH 4/24] Allow setting the VMXE bit in CR4

This patch allows the guest to enable the VMXE bit in CR4, which is a
prerequisite to running VMXON.

Whether to allow setting the VMXE bit now depends on the architecture (svm
or vmx), so its checking has moved to kvm_x86_ops->set_cr4(). This function
now returns an int: If kvm_x86_ops->set_cr4() returns 1, __kvm_set_cr4()
will also return 1, and this will cause kvm_set_cr4() will throw a #GP.

Turning on the VMXE bit is allowed only when the "nested" module option is on,
and turning it off is forbidden after a vmxon.

Signed-off-by: Nadav Har'El <nyh@il.ibm.com>
---
--- .before/arch/x86/include/asm/kvm_host.h	2010-06-15 17:20:01.000000000 +0300
+++ .after/arch/x86/include/asm/kvm_host.h	2010-06-15 17:20:01.000000000 +0300
@@ -490,7 +490,7 @@ struct kvm_x86_ops {
 	void (*decache_cr4_guest_bits)(struct kvm_vcpu *vcpu);
 	void (*set_cr0)(struct kvm_vcpu *vcpu, unsigned long cr0);
 	void (*set_cr3)(struct kvm_vcpu *vcpu, unsigned long cr3);
-	void (*set_cr4)(struct kvm_vcpu *vcpu, unsigned long cr4);
+	int (*set_cr4)(struct kvm_vcpu *vcpu, unsigned long cr4);
 	void (*set_efer)(struct kvm_vcpu *vcpu, u64 efer);
 	void (*get_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
--- .before/arch/x86/kvm/svm.c	2010-06-15 17:20:01.000000000 +0300
+++ .after/arch/x86/kvm/svm.c	2010-06-15 17:20:01.000000000 +0300
@@ -1242,11 +1242,14 @@ static void svm_set_cr0(struct kvm_vcpu 
 	update_cr0_intercept(svm);
 }
 
-static void svm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
+static int svm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	unsigned long host_cr4_mce = read_cr4() & X86_CR4_MCE;
 	unsigned long old_cr4 = to_svm(vcpu)->vmcb->save.cr4;
 
+	if (cr4 & X86_CR4_VMXE)
+		return 1;
+
 	if (npt_enabled && ((old_cr4 ^ cr4) & X86_CR4_PGE))
 		force_new_asid(vcpu);
 
@@ -1255,6 +1258,7 @@ static void svm_set_cr4(struct kvm_vcpu 
 		cr4 |= X86_CR4_PAE;
 	cr4 |= host_cr4_mce;
 	to_svm(vcpu)->vmcb->save.cr4 = cr4;
+	return 0;
 }
 
 static void svm_set_segment(struct kvm_vcpu *vcpu,
--- .before/arch/x86/kvm/x86.c	2010-06-15 17:20:01.000000000 +0300
+++ .after/arch/x86/kvm/x86.c	2010-06-15 17:20:01.000000000 +0300
@@ -490,11 +490,9 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu,
 		   && !load_pdptrs(vcpu, vcpu->arch.cr3))
 		return 1;
 
-	if (cr4 & X86_CR4_VMXE)
+	if (kvm_x86_ops->set_cr4(vcpu, cr4))
 		return 1;
 
-	kvm_x86_ops->set_cr4(vcpu, cr4);
-
 	if ((cr4 ^ old_cr4) & pdptr_bits)
 		kvm_mmu_reset_context(vcpu);
 
--- .before/arch/x86/kvm/vmx.c	2010-06-15 17:20:01.000000000 +0300
+++ .after/arch/x86/kvm/vmx.c	2010-06-15 17:20:01.000000000 +0300
@@ -1874,7 +1874,7 @@ static void ept_save_pdptrs(struct kvm_v
 		  (unsigned long *)&vcpu->arch.regs_dirty);
 }
 
-static void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
+static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
 
 static void ept_update_paging_mode_cr0(unsigned long *hw_cr0,
 					unsigned long cr0,
@@ -1969,11 +1969,19 @@ static void vmx_set_cr3(struct kvm_vcpu 
 	vmcs_writel(GUEST_CR3, guest_cr3);
 }
 
-static void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
+static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	unsigned long hw_cr4 = cr4 | (to_vmx(vcpu)->rmode.vm86_active ?
 		    KVM_RMODE_VM_CR4_ALWAYS_ON : KVM_PMODE_VM_CR4_ALWAYS_ON);
 
+	if (cr4 & X86_CR4_VMXE){
+		if (!nested)
+			return 1;
+	} else {
+		if (nested && to_vmx(vcpu)->nested.vmxon)
+			return 1;
+	}
+
 	vcpu->arch.cr4 = cr4;
 	if (enable_ept) {
 		if (!is_paging(vcpu)) {
@@ -1986,6 +1994,7 @@ static void vmx_set_cr4(struct kvm_vcpu 
 
 	vmcs_writel(CR4_READ_SHADOW, cr4);
 	vmcs_writel(GUEST_CR4, hw_cr4);
+	return 0;
 }
 
 static u64 vmx_get_segment_base(struct kvm_vcpu *vcpu, int seg)

-- 
Nadav Har'El                        |      Tuesday, Jun 15 2010, 3 Tammuz 5770
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |Tea or coffee? Coffee, without cream. It
http://nadav.harel.org.il           |will be without milk, we have no cream.