More nvmx unit tests changes

PATCH Applied Bandan Das 2 participants 6 messages Jun 9, 2014 – Jun 19, 2014 KVM

lore

Share & tools

lore

Bandan Das

author

2014-06-09 21:05 UTC #0 lore raw

v2 cover More nvmx unit tests changes

A couple more updates to test_vmxon, test_vmptrld
and test_vmclear based on kvm commits -

3573e22cfecaac83f82ef4f6847d90e466fc8e10
KVM: nVMX: additional checks on vmxon region

96ec146330d18a938b4773be8d6dd1f93399507c
KVM: nVMX: fail on invalid vmclear/vmptrld pointer

v2:
- 1/3(New): Move the writing of flags register inside asm blocks

Bandan Das (3):
VMX: move write_rflags inside asm blocks for vmx instructions
VMX: Add more checks to test_vmxon
VMX: Updated test_vmclear and test_vmptrld

 lib/x86/processor.h |   7 ---
 x86/vmx.c           | 121 +++++++++++++++++++++++++++++++++++++++++-----------
 x86/vmx.h           |  10 ++++-
 3 files changed, 103 insertions(+), 35 deletions(-)

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-06-09 21:05 UTC #1 lore raw

PATCH v2 1/3 VMX: move write_rflags inside asm blocks for vmx instructions

This makes sure that the flags register doesn't get accidentally overwritten due to compiler optimizations or other means

Signed-off-by Bandan Das <[email protected]>

---
 lib/x86/processor.h |  7 -------
 x86/vmx.c           | 31 ++++++++++---------------------
 x86/vmx.h           | 10 ++++++++--
 3 files changed, 18 insertions(+), 30 deletions(-)

lib/x86/processor.h +0 -7

index 7fc1026..f5f1c82 100644
@@ -351,11 +351,4 @@ static inline void safe_halt(void)
 	asm volatile("sti; hlt");
 }
 
-#ifdef __x86_64__
-static inline void write_rflags(u64 r)
-{
-	asm volatile("push %0; popf\n\t" : : "q"(r) : "cc");
-}
-#endif
-
 #endif

x86/vmx.c +10 ~~-21~~

index 1182eef..768d07f 100644
@@ -65,8 +65,10 @@ extern void *guest_entry;
 static int make_vmcs_current(struct vmcs *vmcs)
 {
 	bool ret;
+	u64 rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
 
-	asm volatile ("vmptrld %1; setbe %0" : "=q" (ret) : "m" (vmcs) : "cc");
+	asm volatile ("push %1; popf; vmptrld %2; setbe %0"
+		      : "=q" (ret) : "q" (rflags), "m" (vmcs) : "cc");
 	return ret;
 }

@@ -92,16 +94,19 @@ static void __attribute__((__used__)) syscall_handler(u64 syscall_no)
 static inline int vmx_on()
 {
 	bool ret;
-	asm volatile ("vmxon %1; setbe %0\n\t"
-		: "=q"(ret) : "m"(vmxon_region) : "cc");
+	u64 rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
+	asm volatile ("push %1; popf; vmxon %2; setbe %0\n\t"
+		      : "=q" (ret) : "q" (rflags), "m" (vmxon_region) : "cc");
 	return ret;
 }
 
 static inline int vmx_off()
 {
 	bool ret;
-	asm volatile("vmxoff; setbe %0\n\t"
-		: "=q"(ret) : : "cc");
+	u64 rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
+
+	asm volatile("push %1; popf; vmxoff; setbe %0\n\t"
+		     : "=q"(ret) : "q" (rflags) : "cc");
 	return ret;
 }

@@ -129,20 +134,13 @@ void print_vmexit_info()
 
 static void test_vmclear(void)
 {
-	u64 rflags;
-
-	rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
-	write_rflags(rflags);
 	report("test vmclear", vmcs_clear(vmcs_root) == 0);
 }
 
 static void test_vmxoff(void)
 {
 	int ret;
-	u64 rflags;
 
-	rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
-	write_rflags(rflags);
 	ret = vmx_off();
 	report("test vmxoff", !ret);
 }

@@ -599,10 +597,7 @@ static int test_vmx_feature_control(void)
 static int test_vmxon(void)
 {
 	int ret;
-	u64 rflags;
 
-	rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
-	write_rflags(rflags);
 	ret = vmx_on();
 	report("test vmxon", !ret);
 	return ret;

@@ -610,27 +605,21 @@ static int test_vmxon(void)
 
 static void test_vmptrld(void)
 {
-	u64 rflags;
 	struct vmcs *vmcs;
 
 	vmcs = alloc_page();
 	vmcs->revision_id = basic.revision;
-	rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
-	write_rflags(rflags);
 	report("test vmptrld", make_vmcs_current(vmcs) == 0);
 }
 
 static void test_vmptrst(void)
 {
-	u64 rflags;
 	int ret;
 	struct vmcs *vmcs1, *vmcs2;
 
 	vmcs1 = alloc_page();
 	memset(vmcs1, 0, PAGE_SIZE);
 	init_vmcs(&vmcs1);
-	rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
-	write_rflags(rflags);
 	ret = vmcs_save(&vmcs2);
 	report("test vmptrst", (!ret) && (vmcs1 == vmcs2));
 }

x86/vmx.h +8 -2

index 26dd161..86bdf67 100644
@@ -2,6 +2,7 @@
 #define __VMX_H
 
 #include "libcflat.h"
+#include "processor.h"
 
 struct vmcs {
 	u32 revision_id; /* vmcs revision identifier */

@@ -515,7 +516,10 @@ extern union vmx_ept_vpid  ept_vpid;
 static inline int vmcs_clear(struct vmcs *vmcs)
 {
 	bool ret;
-	asm volatile ("vmclear %1; setbe %0" : "=q" (ret) : "m" (vmcs) : "cc");
+	u64 rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
+
+	asm volatile ("push %1; popf; vmclear %2; setbe %0"
+		      : "=q" (ret) : "q" (rflags), "m" (vmcs) : "cc");
 	return ret;
 }

@@ -537,8 +541,10 @@ static inline int vmcs_write(enum Encoding enc, u64 val)
 static inline int vmcs_save(struct vmcs **vmcs)
 {
 	bool ret;
+	u64 rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
 
-	asm volatile ("vmptrst %1; setbe %0" : "=q" (ret) : "m" (*vmcs) : "cc");
+	asm volatile ("push %1; popf; vmptrst %2; setbe %0"
+		      : "=q" (ret) : "q" (rflags), "m" (*vmcs) : "cc");
 	return ret;
 }

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-06-09 21:05 UTC #2 lore raw

PATCH v2 2/3 VMX: Add more checks to test_vmxon

In reply to Bandan Das

Verify that vmon fails with unaligned vmxon region or any bits set beyong the physical address width. Also verify failure with an invalid revision identifier.

Signed-off-by Bandan Das <[email protected]>

---
 x86/vmx.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

x86/vmx.c +37 -3

index 768d07f..13e2be8 100644
@@ -37,7 +37,7 @@
 #include "smp.h"
 #include "io.h"
 
-u32 *vmxon_region;
+u64 *vmxon_region;
 struct vmcs *vmcs_root;
 u32 vpid_cnt;
 void *guest_stack, *guest_syscall_stack;

@@ -596,10 +596,44 @@ static int test_vmx_feature_control(void)
 
 static int test_vmxon(void)
 {
-	int ret;
+	int ret, ret1;
+	u64 *tmp_region = vmxon_region;
+	int width = cpuid(0x80000008).a & 0xff;
+
+	/* Unaligned page access */
+	vmxon_region = (u64 *)((intptr_t)vmxon_region + 1);
+	ret1 = vmx_on();
+	report("test vmxon with unaligned vmxon region", ret1);
+	if (!ret1) {
+		ret = 1;
+		goto out;
+	}
+
+	/* gpa bits beyond physical address width are set*/
+	vmxon_region = (u64 *)((intptr_t)tmp_region | ((u64)1 << (width+1)));
+	ret1 = vmx_on();
+	report("test vmxon with bits set beyond physical address width", ret1);
+	if (!ret1) {
+		ret = 1;
+		goto out;
+	}
 
+	/* invalid revision indentifier */
+	vmxon_region = tmp_region;
+	*vmxon_region = 0xba9da9;
+	ret1 = vmx_on();
+	report("test vmxon with invalid revision identifier", ret1);
+	if (!ret1) {
+		ret = 1;
+		goto out;
+	}
+
+	/* and finally a valid region */
+	*vmxon_region = basic.revision;
 	ret = vmx_on();
-	report("test vmxon", !ret);
+	report("test vmxon with valid vmxon region", !ret);
+
+out:
 	return ret;
 }

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-06-09 21:05 UTC #3 lore raw

PATCH v2 3/3 VMX: Updated test_vmclear and test_vmptrld

In reply to Bandan Das

Check if the vmcs pointer is not aligned to page size, and if bits beyond physical address width are set. Also, vmclear and vmptrld should fail if the vmxon region is supplied instead of the vmcs

Signed-off-by Bandan Das <[email protected]>

---
 x86/vmx.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

x86/vmx.c +49 -3

index 13e2be8..b29fc05 100644
@@ -134,7 +134,35 @@ void print_vmexit_info()
 
 static void test_vmclear(void)
 {
-	report("test vmclear", vmcs_clear(vmcs_root) == 0);
+	struct vmcs *tmp_root;
+	int width = cpuid(0x80000008).a & 0xff;
+
+	/*
+	 * Note- The tests below do not necessarily have a
+	 * valid VMCS, but that's ok since the invalid vmcs
+	 * is only used for a specific test and is discarded
+	 * without touching its contents
+	 */
+
+	/* Unaligned page access */
+	tmp_root = (struct vmcs *)((intptr_t)vmcs_root + 1);
+	report("test vmclear with unaligned vmcs",
+	       vmcs_clear(tmp_root) == 1);
+
+	/* gpa bits beyond physical address width are set*/
+	tmp_root = (struct vmcs *)((intptr_t)vmcs_root |
+				   ((u64)1 << (width+1)));
+	report("test vmclear with vmcs address bits set beyond physical address width",
+	       vmcs_clear(tmp_root) == 1);
+
+	/* Pass VMXON region */
+	tmp_root = (struct vmcs *)vmxon_region;
+	report("test vmclear with vmxon region",
+	       vmcs_clear(tmp_root) == 1);
+
+	/* Valid VMCS */
+	report("test vmclear with valid vmcs region", vmcs_clear(vmcs_root) == 0);
+
 }
 
 static void test_vmxoff(void)

@@ -639,11 +667,29 @@ out:
 
 static void test_vmptrld(void)
 {
-	struct vmcs *vmcs;
+	struct vmcs *vmcs, *tmp_root;
+	int width = cpuid(0x80000008).a & 0xff;
 
 	vmcs = alloc_page();
 	vmcs->revision_id = basic.revision;
-	report("test vmptrld", make_vmcs_current(vmcs) == 0);
+
+	/* Unaligned page access */
+	tmp_root = (struct vmcs *)((intptr_t)vmcs + 1);
+	report("test vmptrld with unaligned vmcs",
+	       vmcs_clear(tmp_root) == 1);
+
+	/* gpa bits beyond physical address width are set*/
+	tmp_root = (struct vmcs *)((intptr_t)vmcs |
+				   ((u64)1 << (width+1)));
+	report("test vmptrld with vmcs address bits set beyond physical address width",
+	       vmcs_clear(tmp_root) == 1);
+
+	/* Pass VMXON region */
+	tmp_root = (struct vmcs *)vmxon_region;
+	report("test vmptrld with vmxon region",
+	       vmcs_clear(tmp_root) == 1);
+
+	report("test vmptrld with valid vmcs region", make_vmcs_current(vmcs) == 0);
 }
 
 static void test_vmptrst(void)

Signature · 1 line

1.8.3.1

Paolo Bonzini

maintainer

2014-06-10 06:52 UTC #4 lore raw

In reply to Bandan Das

Il 09/06/2014 23:04, Bandan Das ha scritto:

Quoted 17 lines

A couple more updates to test_vmxon, test_vmptrld
and test_vmclear based on kvm commits -
3573e22cfecaac83f82ef4f6847d90e466fc8e10
KVM: nVMX: additional checks on vmxon region
96ec146330d18a938b4773be8d6dd1f93399507c
KVM: nVMX: fail on invalid vmclear/vmptrld pointer
v2:
- 1/3(New): Move the writing of flags register inside asm blocks
Bandan Das (3):
VMX: move write_rflags inside asm blocks for vmx instructions
VMX: Add more checks to test_vmxon
VMX: Updated test_vmclear and test_vmptrld

Quoted 5 lines

 lib/x86/processor.h |   7 ---
 x86/vmx.c           | 121 +++++++++++++++++++++++++++++++++++++++++-----------
 x86/vmx.h           |  10 ++++-
 3 files changed, 103 insertions(+), 35 deletions(-)

Sooner or later, we probably want to return the flags so that you can test for CF=1 or ZF=1. For now, I've applied the patch. Thanks!

Paolo

Paolo Bonzini

maintainer Applied

2014-06-19 11:41 UTC #5 lore raw

In reply to Bandan Das

Il 09/06/2014 23:04, Bandan Das ha scritto:

Quoted 17 lines

A couple more updates to test_vmxon, test_vmptrld
and test_vmclear based on kvm commits -
3573e22cfecaac83f82ef4f6847d90e466fc8e10
KVM: nVMX: additional checks on vmxon region
96ec146330d18a938b4773be8d6dd1f93399507c
KVM: nVMX: fail on invalid vmclear/vmptrld pointer
v2:
- 1/3(New): Move the writing of flags register inside asm blocks
Bandan Das (3):
VMX: move write_rflags inside asm blocks for vmx instructions
VMX: Add more checks to test_vmxon
VMX: Updated test_vmclear and test_vmptrld

Quoted 5 lines

 lib/x86/processor.h |   7 ---
 x86/vmx.c           | 121 +++++++++++++++++++++++++++++++++++++++++-----------
 x86/vmx.h           |  10 ++++-
 3 files changed, 103 insertions(+), 35 deletions(-)

Applied, thanks.

Paolo