kvm-unit-tests: more instr. interceptions, debug control migration

PATCH In review Jan Kiszka 2 participants 12 messages Jun 17, 2014 – Jun 18, 2014 KVM

Share & tools

Jan Kiszka

author

2014-06-17 07:04 UTC #0 lore raw

v2 cover kvm-unit-tests: more instr. interceptions, debug control migration

Changes in v2 according to review remarks:
- refactored get/set_stage interface
- unified vmx_ctrl_* unions
- used vmx_ctrl_msr in capability test
- changed commented-out debugctl tests

Jan Kiszka (6):
VMX: Add tests for CR3 and CR8 interception
VMX: Rework test stage interface
VMX: Test both interception and execution of instructions
VMX: Unify vmx_ctrl_* unions to vmx_ctrl_msr
VMX: Validate capability MSRs
VMX: Test behavior on set and cleared save/load debug controls

 x86/vmx.c       | 108 ++++++++++++-
 x86/vmx.h       |  44 ++---
 x86/vmx_tests.c | 495 ++++++++++++++++++++++++++++++++++++--------------------
 3 files changed, 443 insertions(+), 204 deletions(-)

Signature · 1 line

1.8.1.1.298.ge7eed54

Jan Kiszka

author

2014-06-17 07:04 UTC #1 lore raw

PATCH v2 1/6 VMX: Add tests for CR3 and CR8 interception

Need to fix FIELD_* constants for this to make the exit qualification check work.

Signed-off-by Jan Kiszka <[email protected]>

---
 x86/vmx.h       |  2 ++
 x86/vmx_tests.c | 32 +++++++++++++++++++++++++++++---
 2 files changed, 31 insertions(+), 3 deletions(-)

x86/vmx.h +2 -0

index 26dd161..69a5385 100644
@@ -357,6 +357,8 @@ enum Ctrl0 {
 	CPU_RDTSC		= 1ul << 12,
 	CPU_CR3_LOAD		= 1ul << 15,
 	CPU_CR3_STORE		= 1ul << 16,
+	CPU_CR8_LOAD		= 1ul << 19,
+	CPU_CR8_STORE		= 1ul << 20,
 	CPU_TPR_SHADOW		= 1ul << 21,
 	CPU_NMI_WINDOW		= 1ul << 22,
 	CPU_IO			= 1ul << 24,

x86/vmx_tests.c +29 -3

index a40cb18..149a591 100644
@@ -820,8 +820,8 @@ static int iobmp_exit_handler()
 #define INSN_ALWAYS_TRAP	2
 #define INSN_NEVER_TRAP		3
 
-#define FIELD_EXIT_QUAL		0
-#define FIELD_INSN_INFO		1
+#define FIELD_EXIT_QUAL		(1 << 1)
+#define FIELD_INSN_INFO		(1 << 2)
 
 asm(
 	"insn_hlt: hlt;ret\n\t"

@@ -829,6 +829,12 @@ asm(
 	"insn_mwait: mwait;ret\n\t"
 	"insn_rdpmc: rdpmc;ret\n\t"
 	"insn_rdtsc: rdtsc;ret\n\t"
+	"insn_cr3_load: mov %rax,%cr3;ret\n\t"
+	"insn_cr3_store: mov %cr3,%rax;ret\n\t"
+#ifdef __x86_64__
+	"insn_cr8_load: mov %rax,%cr8;ret\n\t"
+	"insn_cr8_store: mov %cr8,%rax;ret\n\t"
+#endif
 	"insn_monitor: monitor;ret\n\t"
 	"insn_pause: pause;ret\n\t"
 	"insn_wbinvd: wbinvd;ret\n\t"

@@ -840,6 +846,12 @@ extern void insn_invlpg();
 extern void insn_mwait();
 extern void insn_rdpmc();
 extern void insn_rdtsc();
+extern void insn_cr3_load();
+extern void insn_cr3_store();
+#ifdef __x86_64__
+extern void insn_cr8_load();
+extern void insn_cr8_store();
+#endif
 extern void insn_monitor();
 extern void insn_pause();
 extern void insn_wbinvd();

@@ -856,7 +868,7 @@ struct insn_table {
 	u32 reason;
 	ulong exit_qual;
 	u32 insn_info;
-	// Use FIELD_EXIT_QUAL and FIELD_INSN_INFO to efines
+	// Use FIELD_EXIT_QUAL and FIELD_INSN_INFO to define
 	// which field need to be tested, reason is always tested
 	u32 test_field;
 };

@@ -877,6 +889,16 @@ static struct insn_table insn_table[] = {
 	{"MWAIT", CPU_MWAIT, insn_mwait, INSN_CPU0, 36, 0, 0, 0},
 	{"RDPMC", CPU_RDPMC, insn_rdpmc, INSN_CPU0, 15, 0, 0, 0},
 	{"RDTSC", CPU_RDTSC, insn_rdtsc, INSN_CPU0, 16, 0, 0, 0},
+	{"CR3 load", CPU_CR3_LOAD, insn_cr3_load, INSN_CPU0, 28, 0x3, 0,
+		FIELD_EXIT_QUAL},
+	{"CR3 store", CPU_CR3_STORE, insn_cr3_store, INSN_CPU0, 28, 0x13, 0,
+		FIELD_EXIT_QUAL},
+#ifdef __x86_64__
+	{"CR8 load", CPU_CR8_LOAD, insn_cr8_load, INSN_CPU0, 28, 0x8, 0,
+		FIELD_EXIT_QUAL},
+	{"CR8 store", CPU_CR8_STORE, insn_cr8_store, INSN_CPU0, 28, 0x18, 0,
+		FIELD_EXIT_QUAL},
+#endif
 	{"MONITOR", CPU_MONITOR, insn_monitor, INSN_CPU0, 39, 0, 0, 0},
 	{"PAUSE", CPU_PAUSE, insn_pause, INSN_CPU0, 40, 0, 0, 0},
 	// Flags for Secondary Processor-Based VM-Execution Controls

@@ -894,6 +916,10 @@ static int insn_intercept_init()
 
 	ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
 	ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
+		CPU_CR3_LOAD | CPU_CR3_STORE |
+#ifdef __x86_64__
+		CPU_CR8_LOAD | CPU_CR8_STORE |
+#endif
 		CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
 	ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
 	vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);

Signature · 1 line

1.8.1.1.298.ge7eed54

Paolo Bonzini

maintainer

2014-06-17 07:41 UTC #2 lore raw

Re: [PATCH v2 1/6] VMX: Add tests for CR3 and CR8 interception

Il 17/06/2014 09:04, Jan Kiszka ha scritto:

x86/vmx_tests.c static int iobmp_exit_handler() · line 824

Quoted 5 lines

x86/vmx_tests.c +2 -2

-#define FIELD_EXIT_QUAL		0
-#define FIELD_INSN_INFO		1
+#define FIELD_EXIT_QUAL		(1 << 1)
+#define FIELD_INSN_INFO		(1 << 2)

Heh, you probably wanted 1<<0 and 1<<1. I'll fix it up. :)

Paolo

Jan Kiszka

author

2014-06-17 07:42 UTC #3 lore raw

Re: [PATCH v2 1/6] VMX: Add tests for CR3 and CR8 interception

Quoted 8 lines · Paolo Bonzini

Il 17/06/2014 09:04, Jan Kiszka ha scritto:
x86/vmx_tests.c static int iobmp_exit_handler() · line 824
Quoted 5 lines
x86/vmx_tests.c +2 -2
-#define FIELD_EXIT_QUAL        0
-#define FIELD_INSN_INFO        1
+#define FIELD_EXIT_QUAL        (1 << 1)
+#define FIELD_INSN_INFO        (1 << 2)
Heh, you probably wanted 1<<0 and 1<<1. I'll fix it up. :)

Yeah... thanks in advance.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2014-06-17 07:04 UTC #4 lore raw

PATCH v2 2/6 VMX: Rework test stage interface

In reply to Jan Kiszka

Consistently access the stage only via the helper functions. To enforce this, move them from vmx_tests.c to vmx.c. At this chance, introduce a stage incrementation helper.

Signed-off-by Jan Kiszka <[email protected]>

---
 x86/vmx.c       |  26 ++++++
 x86/vmx.h       |   4 +
 x86/vmx_tests.c | 250 +++++++++++++++++++++++++++-----------------------------
 3 files changed, 151 insertions(+), 129 deletions(-)

x86/vmx.c +26 -0

index 1182eef..ba6a02b 100644
@@ -62,6 +62,32 @@ extern void *vmx_return;
 extern void *entry_sysenter;
 extern void *guest_entry;
 
+static volatile u32 stage;
+
+void vmx_set_test_stage(u32 s)
+{
+	barrier();
+	stage = s;
+	barrier();
+}
+
+u32 vmx_get_test_stage(void)
+{
+	u32 s;
+
+	barrier();
+	s = stage;
+	barrier();
+	return s;
+}
+
+void vmx_inc_test_stage(void)
+{
+	barrier();
+	stage++;
+	barrier();
+}
+
 static int make_vmcs_current(struct vmcs *vmcs)
 {
 	bool ret;

x86/vmx.h +4 -0

index 69a5385..1a8ae4c 100644
@@ -514,6 +514,10 @@ extern union vmx_ctrl_exit ctrl_exit_rev;
 extern union vmx_ctrl_ent ctrl_enter_rev;
 extern union vmx_ept_vpid  ept_vpid;
 
+void vmx_set_test_stage(u32 s);
+u32 vmx_get_test_stage(void);
+void vmx_inc_test_stage(void);
+
 static inline int vmcs_clear(struct vmcs *vmcs)
 {
 	bool ret;

x86/vmx_tests.c +121 ~~-129~~

696 lines not on this page · raw message

Signature · 1 line

1.8.1.1.298.ge7eed54

Jan Kiszka

author

2014-06-17 07:04 UTC #5 lore raw

PATCH v2 3/6 VMX: Test both interception and execution of instructions

In reply to Jan Kiszka

Extend the instruction interception test to also check for interception-free execution.

Signed-off-by Jan Kiszka <[email protected]>

---
 x86/vmx_tests.c | 121 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 72 insertions(+), 49 deletions(-)

x86/vmx_tests.c +72 ~~-49~~

index 5485e4c..f02de3d 100644
@@ -807,7 +807,6 @@ static int iobmp_exit_handler()
 #define INSN_CPU0		0
 #define INSN_CPU1		1
 #define INSN_ALWAYS_TRAP	2
-#define INSN_NEVER_TRAP		3
 
 #define FIELD_EXIT_QUAL		(1 << 1)
 #define FIELD_INSN_INFO		(1 << 2)

@@ -818,7 +817,7 @@ asm(
 	"insn_mwait: mwait;ret\n\t"
 	"insn_rdpmc: rdpmc;ret\n\t"
 	"insn_rdtsc: rdtsc;ret\n\t"
-	"insn_cr3_load: mov %rax,%cr3;ret\n\t"
+	"insn_cr3_load: mov cr3,%rax; mov %rax,%cr3;ret\n\t"
 	"insn_cr3_store: mov %cr3,%rax;ret\n\t"
 #ifdef __x86_64__
 	"insn_cr8_load: mov %rax,%cr8;ret\n\t"

@@ -848,6 +847,7 @@ extern void insn_cpuid();
 extern void insn_invd();
 
 u32 cur_insn;
+u64 cr3;
 
 struct insn_table {
 	const char *name;

@@ -901,55 +901,56 @@ static struct insn_table insn_table[] = {
 
 static int insn_intercept_init()
 {
-	u32 ctrl_cpu[2];
+	u32 ctrl_cpu;
 
-	ctrl_cpu[0] = vmcs_read(CPU_EXEC_CTRL0);
-	ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
-		CPU_CR3_LOAD | CPU_CR3_STORE |
-#ifdef __x86_64__
-		CPU_CR8_LOAD | CPU_CR8_STORE |
-#endif
-		CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
-	ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
-	vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
-	ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
-	ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
-	ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
-	vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
+	ctrl_cpu = ctrl_cpu_rev[0].set | CPU_SECONDARY;
+	ctrl_cpu &= ctrl_cpu_rev[0].clr;
+	vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu);
+	vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu_rev[1].set);
+	cr3 = read_cr3();
 	return VMX_TEST_START;
 }
 
 static void insn_intercept_main()
 {
-	cur_insn = 0;
-	while(insn_table[cur_insn].name != NULL) {
-		vmx_set_test_stage(cur_insn);
-		if ((insn_table[cur_insn].type == INSN_CPU0
-			&& !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag))
-			|| (insn_table[cur_insn].type == INSN_CPU1
-			&& !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
-			printf("\tCPU_CTRL1.CPU_%s is not supported.\n",
-				insn_table[cur_insn].name);
+	char msg[80];
+
+	for (cur_insn = 0; insn_table[cur_insn].name != NULL; cur_insn++) {
+		vmx_set_test_stage(cur_insn * 2);
+		if ((insn_table[cur_insn].type == INSN_CPU0 &&
+		     !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
+		    (insn_table[cur_insn].type == INSN_CPU1 &&
+		     !(ctrl_cpu_rev[1].clr & insn_table[cur_insn].flag))) {
+			printf("\tCPU_CTRL%d.CPU_%s is not supported.\n",
+			       insn_table[cur_insn].type - INSN_CPU0,
+			       insn_table[cur_insn].name);
 			continue;
 		}
+
+		if ((insn_table[cur_insn].type == INSN_CPU0 &&
+		     !(ctrl_cpu_rev[0].set & insn_table[cur_insn].flag)) ||
+		    (insn_table[cur_insn].type == INSN_CPU1 &&
+		     !(ctrl_cpu_rev[1].set & insn_table[cur_insn].flag))) {
+			/* skip hlt, it stalls the guest and is tested below */
+			if (insn_table[cur_insn].insn_func != insn_hlt)
+				insn_table[cur_insn].insn_func();
+			snprintf(msg, sizeof(msg), "execute %s",
+				 insn_table[cur_insn].name);
+			report(msg, vmx_get_test_stage() == cur_insn * 2);
+		} else if (insn_table[cur_insn].type != INSN_ALWAYS_TRAP)
+			printf("\tCPU_CTRL%d.CPU_%s always traps.\n",
+			       insn_table[cur_insn].type - INSN_CPU0,
+			       insn_table[cur_insn].name);
+
+		vmcall();
+
 		insn_table[cur_insn].insn_func();
-		switch (insn_table[cur_insn].type) {
-		case INSN_CPU0:
-		case INSN_CPU1:
-		case INSN_ALWAYS_TRAP:
-			if (vmx_get_test_stage() != cur_insn + 1)
-				report(insn_table[cur_insn].name, 0);
-			else
-				report(insn_table[cur_insn].name, 1);
-			break;
-		case INSN_NEVER_TRAP:
-			if (vmx_get_test_stage() == cur_insn + 1)
-				report(insn_table[cur_insn].name, 0);
-			else
-				report(insn_table[cur_insn].name, 1);
-			break;
-		}
-		cur_insn ++;
+		snprintf(msg, sizeof(msg), "intercept %s",
+			 insn_table[cur_insn].name);
+		report(msg, vmx_get_test_stage() == cur_insn * 2 + 1);
+
+		vmx_set_test_stage(cur_insn * 2 + 1);
+		vmcall();
 	}
 }

@@ -967,14 +968,36 @@ static int insn_intercept_exit_handler()
 	exit_qual = vmcs_read(EXI_QUALIFICATION);
 	insn_len = vmcs_read(EXI_INST_LEN);
 	insn_info = vmcs_read(EXI_INST_INFO);
-	pass = (cur_insn == vmx_get_test_stage()) &&
+
+	if (reason == VMX_VMCALL) {
+		u32 val = 0;
+
+		if (insn_table[cur_insn].type == INSN_CPU0)
+			val = vmcs_read(CPU_EXEC_CTRL0);
+		else if (insn_table[cur_insn].type == INSN_CPU1)
+			val = vmcs_read(CPU_EXEC_CTRL1);
+
+		if (vmx_get_test_stage() & 1)
+			val &= ~insn_table[cur_insn].flag;
+		else
+			val |= insn_table[cur_insn].flag;
+
+		if (insn_table[cur_insn].type == INSN_CPU0)
+			vmcs_write(CPU_EXEC_CTRL0, val | ctrl_cpu_rev[0].set);
+		else if (insn_table[cur_insn].type == INSN_CPU1)
+			vmcs_write(CPU_EXEC_CTRL1, val | ctrl_cpu_rev[1].set);
+	} else {
+		pass = (cur_insn * 2 == vmx_get_test_stage()) &&
 			insn_table[cur_insn].reason == reason;
-	if (insn_table[cur_insn].test_field & FIELD_EXIT_QUAL)
-		pass = pass && insn_table[cur_insn].exit_qual == exit_qual;
-	if (insn_table[cur_insn].test_field & FIELD_INSN_INFO)
-		pass = pass && insn_table[cur_insn].insn_info == insn_info;
-	if (pass)
-		vmx_inc_test_stage();
+		if (insn_table[cur_insn].test_field & FIELD_EXIT_QUAL &&
+		    insn_table[cur_insn].exit_qual != exit_qual)
+			pass = false;
+		if (insn_table[cur_insn].test_field & FIELD_INSN_INFO &&
+		    insn_table[cur_insn].insn_info != insn_info)
+			pass = false;
+		if (pass)
+			vmx_inc_test_stage();
+	}
 	vmcs_write(GUEST_RIP, guest_rip + insn_len);
 	return VMX_TEST_RESUME;
 }

Signature · 1 line

1.8.1.1.298.ge7eed54

Jan Kiszka

author

2014-06-17 07:04 UTC #6 lore raw

PATCH v2 4/6 VMX: Unify vmx_ctrl_* unions to vmx_ctrl_msr

In reply to Jan Kiszka

Signed-off-by Jan Kiszka <[email protected]>

---
 x86/vmx.c |  8 ++++----
 x86/vmx.h | 31 +++++--------------------------
 2 files changed, 9 insertions(+), 30 deletions(-)

x86/vmx.c +4 -4

index ba6a02b..f01e443 100644
@@ -49,10 +49,10 @@ bool launched;
 u64 host_rflags;
 
 union vmx_basic basic;
-union vmx_ctrl_pin ctrl_pin_rev;
-union vmx_ctrl_cpu ctrl_cpu_rev[2];
-union vmx_ctrl_exit ctrl_exit_rev;
-union vmx_ctrl_ent ctrl_enter_rev;
+union vmx_ctrl_msr ctrl_pin_rev;
+union vmx_ctrl_msr ctrl_cpu_rev[2];
+union vmx_ctrl_msr ctrl_exit_rev;
+union vmx_ctrl_msr ctrl_enter_rev;
 union vmx_ept_vpid  ept_vpid;
 
 extern struct descriptor_table_ptr gdt64_desc;

x86/vmx.h +5 ~~-26~~

index 1a8ae4c..00f2842 100644
@@ -55,28 +55,7 @@ union vmx_basic {
 	};
 };
 
-union vmx_ctrl_pin {
-	u64 val;
-	struct {
-		u32 set, clr;
-	};
-};
-
-union vmx_ctrl_cpu {
-	u64 val;
-	struct {
-		u32 set, clr;
-	};
-};
-
-union vmx_ctrl_exit {
-	u64 val;
-	struct {
-		u32 set, clr;
-	};
-};
-
-union vmx_ctrl_ent {
+union vmx_ctrl_msr {
 	u64 val;
 	struct {
 		u32 set, clr;

@@ -508,10 +487,10 @@ enum Ctrl1 {
 extern struct regs regs;
 
 extern union vmx_basic basic;
-extern union vmx_ctrl_pin ctrl_pin_rev;
-extern union vmx_ctrl_cpu ctrl_cpu_rev[2];
-extern union vmx_ctrl_exit ctrl_exit_rev;
-extern union vmx_ctrl_ent ctrl_enter_rev;
+extern union vmx_ctrl_msr ctrl_pin_rev;
+extern union vmx_ctrl_msr ctrl_cpu_rev[2];
+extern union vmx_ctrl_msr ctrl_exit_rev;
+extern union vmx_ctrl_msr ctrl_enter_rev;
 extern union vmx_ept_vpid  ept_vpid;
 
 void vmx_set_test_stage(u32 s);

Signature · 1 line

1.8.1.1.298.ge7eed54

Jan Kiszka

author

2014-06-17 07:04 UTC #7 lore raw

PATCH v2 5/6 VMX: Validate capability MSRs

In reply to Jan Kiszka

Check for required-0 or required-1 bits as well as known field value restrictions. Also check the consistency between VMX_*_CTLS and VMX_TRUE_*_CTLS and between CR0/4_FIXED0 and CR0/4_FIXED1.

Signed-off-by Jan Kiszka <[email protected]>

---
 x86/vmx.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 x86/vmx.h |  5 +++--
 2 files changed, 76 insertions(+), 3 deletions(-)

x86/vmx.c +73 -1

index f01e443..5c3498a 100644
@@ -661,6 +661,77 @@ static void test_vmptrst(void)
 	report("test vmptrst", (!ret) && (vmcs1 == vmcs2));
 }
 
+struct vmx_ctl_msr {
+	const char *name;
+	u32 index, true_index;
+	u32 default1;
+} vmx_ctl_msr[] = {
+	{ "MSR_IA32_VMX_PINBASED_CTLS", MSR_IA32_VMX_PINBASED_CTLS,
+	  MSR_IA32_VMX_TRUE_PIN, 0x16 },
+	{ "MSR_IA32_VMX_PROCBASED_CTLS", MSR_IA32_VMX_PROCBASED_CTLS,
+	  MSR_IA32_VMX_TRUE_PROC, 0x401e172 },
+	{ "MSR_IA32_VMX_PROCBASED_CTLS2", MSR_IA32_VMX_PROCBASED_CTLS2,
+	  MSR_IA32_VMX_PROCBASED_CTLS2, 0 },
+	{ "MSR_IA32_VMX_EXIT_CTLS", MSR_IA32_VMX_EXIT_CTLS,
+	  MSR_IA32_VMX_TRUE_EXIT, 0x36dff },
+	{ "MSR_IA32_VMX_ENTRY_CTLS", MSR_IA32_VMX_ENTRY_CTLS,
+	  MSR_IA32_VMX_TRUE_ENTRY, 0x11ff },
+};
+
+static void test_vmx_caps(void)
+{
+	u64 val, default1, fixed0, fixed1;
+	union vmx_ctrl_msr ctrl, true_ctrl;
+	unsigned int n;
+	bool ok;
+
+	printf("\nTest suite: VMX capability reporting\n");
+
+	report("MSR_IA32_VMX_BASIC",
+	       (basic.revision & (1ul << 31)) == 0 &&
+	       basic.size > 0 && basic.size <= 4096 &&
+	       (basic.type == 0 || basic.type == 6) &&
+	       basic.reserved1 == 0 && basic.reserved2 == 0);
+
+	val = rdmsr(MSR_IA32_VMX_MISC);
+	report("MSR_IA32_VMX_MISC",
+	       (!(ctrl_cpu_rev[1].clr & CPU_URG) || val & (1ul << 5)) &&
+	       ((val >> 16) & 0x1ff) <= 256 &&
+	       (val & 0xc0007e00) == 0);
+
+	for (n = 0; n < ARRAY_SIZE(vmx_ctl_msr); n++) {
+		ctrl.val = rdmsr(vmx_ctl_msr[n].index);
+		default1 = vmx_ctl_msr[n].default1;
+		ok = (ctrl.set & default1) == default1 &&
+			((ctrl.set ^ ctrl.clr) & ~ctrl.clr) == 0;
+		if (ok && basic.ctrl) {
+			true_ctrl.val = rdmsr(vmx_ctl_msr[n].true_index);
+			ok = ctrl.clr == true_ctrl.clr &&
+				((ctrl.set ^ true_ctrl.set) & ~default1) == 0;
+		}
+		report(vmx_ctl_msr[n].name, ok);
+	}
+
+	fixed0 = rdmsr(MSR_IA32_VMX_CR0_FIXED0);
+	fixed1 = rdmsr(MSR_IA32_VMX_CR0_FIXED1);
+	report("MSR_IA32_VMX_IA32_VMX_CR0_FIXED0/1",
+	       ((fixed0 ^ fixed1) & ~fixed1) == 0);
+
+	fixed0 = rdmsr(MSR_IA32_VMX_CR4_FIXED0);
+	fixed1 = rdmsr(MSR_IA32_VMX_CR4_FIXED1);
+	report("MSR_IA32_VMX_IA32_VMX_CR4_FIXED0/1",
+	       ((fixed0 ^ fixed1) & ~fixed1) == 0);
+
+	val = rdmsr(MSR_IA32_VMX_VMCS_ENUM);
+	report("MSR_IA32_VMX_VMCS_ENUM",
+	       (val & 0x3e) >= 0x2a &&
+	       (val & 0xfffffffffffffc01Ull) == 0);
+
+	val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
+	report("MSR_IA32_VMX_EPT_VPID_CAP",
+	       (val & 0xfffff07ef9eebebeUll) == 0);
+}
+
 /* This function can only be called in guest */
 static void __attribute__((__used__)) hypercall(u32 hypercall_no)
 {

@@ -803,7 +874,7 @@ static int test_run(struct vmx_test *test)
 	regs = test->guest_regs;
 	vmcs_write(GUEST_RFLAGS, regs.rflags | 0x2);
 	launched = 0;
-	printf("\nTest suite : %s\n", test->name);
+	printf("\nTest suite: %s\n", test->name);
 	vmx_run();
 	if (vmx_off()) {
 		printf("%s : vmxoff failed.\n", __func__);

@@ -842,6 +913,7 @@ int main(void)
 		goto exit;
 	}
 	test_vmxoff();
+	test_vmx_caps();
 
 	while (vmx_tests[++i].name != NULL)
 		if (test_run(&vmx_tests[i]))

x86/vmx.h +3 -2

index 00f2842..87457b1 100644
@@ -46,12 +46,13 @@ union vmx_basic {
 	struct {
 		u32 revision;
 		u32	size:13,
-			: 3,
+			reserved1: 3,
 			width:1,
 			dual:1,
 			type:4,
 			insouts:1,
-			ctrl:1;
+			ctrl:1,
+			reserved2:8;
 	};
 };

Signature · 1 line

1.8.1.1.298.ge7eed54

Paolo Bonzini

maintainer

2014-06-17 08:00 UTC #8 lore raw

Re: [PATCH v2 5/6] VMX: Validate capability MSRs

Il 17/06/2014 09:04, Jan Kiszka ha scritto:

x86/vmx.c static void test_vmptrst(void) · line 706

Quoted 3 lines

x86/vmx.c +3 -0

+		default1 = vmx_ctl_msr[n].default1;
+		ok = (ctrl.set & default1) == default1 &&
+			((ctrl.set ^ ctrl.clr) & ~ctrl.clr) == 0;

Thanks, now I can understand what's going on. :) It can still be simplified though.

This is just (ctrl.set & ~ctrl.clr) == 0:

	(a ^ b) & ~b == 0

-> (a & ~b) ^ (b & ~b) == 0
-> a & ~b == 0

This expresses just as well the concept that set=1, clr=0 is an impossible combination.

Also, it's a bit easier to read if the second line is written as a separate statement:

	ok = ok && (ctrl.set & ~ctrl.clr) == 0;

x86/vmx.c static void test_vmptrst(void) · line 710

Quoted 4 lines

x86/vmx.c +4 -0

+		if (ok && basic.ctrl) {
+			true_ctrl.val = rdmsr(vmx_ctl_msr[n].true_index);
+			ok = ctrl.clr == true_ctrl.clr &&
+				((ctrl.set ^ true_ctrl.set) & ~default1) == 0;

This is

	((ctrl.set ^ true_ctrl.set) & ~default1 == 0

-> ((ctrl.set & ~default1) ^ (true_ctrl.set & ~default1) == 0
-> ((ctrl.set & ~default1) == (true_ctrl.set & ~default1)

A bit longer, but clearer: the difference between ctrl.set and true_ctrl.set is only that true_ctrl.set can clear some default-1 bits.

Or you can simplify it further:

-> (ctrl.set | default1) == (true_ctrl.set | default1)
-> ctrl.set == (true_ctrl.set | default1)

Also clearer: the difference between ctrl.set and true_ctrl.set is only that default-1 bits must be 1 in ctrl.set. Pick the one you prefer.

Again, using a separate statement makes it easier to read in my opinion; in fact I would also write both statements as

	ok = ok && ...

even though it's redundant for the clr test.

Can you submit v3 of this patch only?

Paolo

Jan Kiszka

author

2014-06-18 05:32 UTC #9 lore raw

PATCH v3 5/6 VMX: Validate capability MSRs

Check for required-0 or required-1 bits as well as known field value restrictions. Also check the consistency between VMX_*_CTLS and VMX_TRUE_*_CTLS and between CR0/4_FIXED0 and CR0/4_FIXED1.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v3:
- integrated suggestions of Paolo

 x86/vmx.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 x86/vmx.h |  5 +++--
 2 files changed, 76 insertions(+), 3 deletions(-)

x86/vmx.c +73 -1

index f01e443..5bb5969 100644
@@ -661,6 +661,77 @@ static void test_vmptrst(void)
 	report("test vmptrst", (!ret) && (vmcs1 == vmcs2));
 }
 
+struct vmx_ctl_msr {
+	const char *name;
+	u32 index, true_index;
+	u32 default1;
+} vmx_ctl_msr[] = {
+	{ "MSR_IA32_VMX_PINBASED_CTLS", MSR_IA32_VMX_PINBASED_CTLS,
+	  MSR_IA32_VMX_TRUE_PIN, 0x16 },
+	{ "MSR_IA32_VMX_PROCBASED_CTLS", MSR_IA32_VMX_PROCBASED_CTLS,
+	  MSR_IA32_VMX_TRUE_PROC, 0x401e172 },
+	{ "MSR_IA32_VMX_PROCBASED_CTLS2", MSR_IA32_VMX_PROCBASED_CTLS2,
+	  MSR_IA32_VMX_PROCBASED_CTLS2, 0 },
+	{ "MSR_IA32_VMX_EXIT_CTLS", MSR_IA32_VMX_EXIT_CTLS,
+	  MSR_IA32_VMX_TRUE_EXIT, 0x36dff },
+	{ "MSR_IA32_VMX_ENTRY_CTLS", MSR_IA32_VMX_ENTRY_CTLS,
+	  MSR_IA32_VMX_TRUE_ENTRY, 0x11ff },
+};
+
+static void test_vmx_caps(void)
+{
+	u64 val, default1, fixed0, fixed1;
+	union vmx_ctrl_msr ctrl, true_ctrl;
+	unsigned int n;
+	bool ok;
+
+	printf("\nTest suite: VMX capability reporting\n");
+
+	report("MSR_IA32_VMX_BASIC",
+	       (basic.revision & (1ul << 31)) == 0 &&
+	       basic.size > 0 && basic.size <= 4096 &&
+	       (basic.type == 0 || basic.type == 6) &&
+	       basic.reserved1 == 0 && basic.reserved2 == 0);
+
+	val = rdmsr(MSR_IA32_VMX_MISC);
+	report("MSR_IA32_VMX_MISC",
+	       (!(ctrl_cpu_rev[1].clr & CPU_URG) || val & (1ul << 5)) &&
+	       ((val >> 16) & 0x1ff) <= 256 &&
+	       (val & 0xc0007e00) == 0);
+
+	for (n = 0; n < ARRAY_SIZE(vmx_ctl_msr); n++) {
+		ctrl.val = rdmsr(vmx_ctl_msr[n].index);
+		default1 = vmx_ctl_msr[n].default1;
+		ok = (ctrl.set & default1) == default1;
+		ok = ok && (ctrl.set & ~ctrl.clr) == 0;
+		if (ok && basic.ctrl) {
+			true_ctrl.val = rdmsr(vmx_ctl_msr[n].true_index);
+			ok = ctrl.clr == true_ctrl.clr;
+			ok = ok && ctrl.set == (true_ctrl.set | default1);
+		}
+		report(vmx_ctl_msr[n].name, ok);
+	}
+
+	fixed0 = rdmsr(MSR_IA32_VMX_CR0_FIXED0);
+	fixed1 = rdmsr(MSR_IA32_VMX_CR0_FIXED1);
+	report("MSR_IA32_VMX_IA32_VMX_CR0_FIXED0/1",
+	       ((fixed0 ^ fixed1) & ~fixed1) == 0);
+
+	fixed0 = rdmsr(MSR_IA32_VMX_CR4_FIXED0);
+	fixed1 = rdmsr(MSR_IA32_VMX_CR4_FIXED1);
+	report("MSR_IA32_VMX_IA32_VMX_CR4_FIXED0/1",
+	       ((fixed0 ^ fixed1) & ~fixed1) == 0);
+
+	val = rdmsr(MSR_IA32_VMX_VMCS_ENUM);
+	report("MSR_IA32_VMX_VMCS_ENUM",
+	       (val & 0x3e) >= 0x2a &&
+	       (val & 0xfffffffffffffc01Ull) == 0);
+
+	val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
+	report("MSR_IA32_VMX_EPT_VPID_CAP",
+	       (val & 0xfffff07ef9eebebeUll) == 0);
+}
+
 /* This function can only be called in guest */
 static void __attribute__((__used__)) hypercall(u32 hypercall_no)
 {

@@ -803,7 +874,7 @@ static int test_run(struct vmx_test *test)
 	regs = test->guest_regs;
 	vmcs_write(GUEST_RFLAGS, regs.rflags | 0x2);
 	launched = 0;
-	printf("\nTest suite : %s\n", test->name);
+	printf("\nTest suite: %s\n", test->name);
 	vmx_run();
 	if (vmx_off()) {
 		printf("%s : vmxoff failed.\n", __func__);

@@ -842,6 +913,7 @@ int main(void)
 		goto exit;
 	}
 	test_vmxoff();
+	test_vmx_caps();
 
 	while (vmx_tests[++i].name != NULL)
 		if (test_run(&vmx_tests[i]))

x86/vmx.h +3 -2

index 00f2842..87457b1 100644
@@ -46,12 +46,13 @@ union vmx_basic {
 	struct {
 		u32 revision;
 		u32	size:13,
-			: 3,
+			reserved1: 3,
 			width:1,
 			dual:1,
 			type:4,
 			insouts:1,
-			ctrl:1;
+			ctrl:1,
+			reserved2:8;
 	};
 };

Signature · 1 line

1.8.1.1.298.ge7eed54

Paolo Bonzini

maintainer Reviewed-by on v3 5/6

2014-06-18 09:38 UTC #10 lore raw

Re: [PATCH v3 5/6] VMX: Validate capability MSRs

Il 18/06/2014 07:32, Jan Kiszka ha scritto:

Quoted 124 lines

Check for required-0 or required-1 bits as well as known field value restrictions. Also check the consistency between VMX_*_CTLS and VMX_TRUE_*_CTLS and between CR0/4_FIXED0 and CR0/4_FIXED1.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v3:
- integrated suggestions of Paolo

 x86/vmx.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 x86/vmx.h |  5 +++--
 2 files changed, 76 insertions(+), 3 deletions(-)

x86/vmx.c +73 -1

index f01e443..5bb5969 100644
@@ -661,6 +661,77 @@ static void test_vmptrst(void)
 	report("test vmptrst", (!ret) && (vmcs1 == vmcs2));
 }
 
+struct vmx_ctl_msr {
+	const char *name;
+	u32 index, true_index;
+	u32 default1;
+} vmx_ctl_msr[] = {
+	{ "MSR_IA32_VMX_PINBASED_CTLS", MSR_IA32_VMX_PINBASED_CTLS,
+	  MSR_IA32_VMX_TRUE_PIN, 0x16 },
+	{ "MSR_IA32_VMX_PROCBASED_CTLS", MSR_IA32_VMX_PROCBASED_CTLS,
+	  MSR_IA32_VMX_TRUE_PROC, 0x401e172 },
+	{ "MSR_IA32_VMX_PROCBASED_CTLS2", MSR_IA32_VMX_PROCBASED_CTLS2,
+	  MSR_IA32_VMX_PROCBASED_CTLS2, 0 },
+	{ "MSR_IA32_VMX_EXIT_CTLS", MSR_IA32_VMX_EXIT_CTLS,
+	  MSR_IA32_VMX_TRUE_EXIT, 0x36dff },
+	{ "MSR_IA32_VMX_ENTRY_CTLS", MSR_IA32_VMX_ENTRY_CTLS,
+	  MSR_IA32_VMX_TRUE_ENTRY, 0x11ff },
+};
+
+static void test_vmx_caps(void)
+{
+	u64 val, default1, fixed0, fixed1;
+	union vmx_ctrl_msr ctrl, true_ctrl;
+	unsigned int n;
+	bool ok;
+
+	printf("\nTest suite: VMX capability reporting\n");
+
+	report("MSR_IA32_VMX_BASIC",
+	       (basic.revision & (1ul << 31)) == 0 &&
+	       basic.size > 0 && basic.size <= 4096 &&
+	       (basic.type == 0 || basic.type == 6) &&
+	       basic.reserved1 == 0 && basic.reserved2 == 0);
+
+	val = rdmsr(MSR_IA32_VMX_MISC);
+	report("MSR_IA32_VMX_MISC",
+	       (!(ctrl_cpu_rev[1].clr & CPU_URG) || val & (1ul << 5)) &&
+	       ((val >> 16) & 0x1ff) <= 256 &&
+	       (val & 0xc0007e00) == 0);
+
+	for (n = 0; n < ARRAY_SIZE(vmx_ctl_msr); n++) {
+		ctrl.val = rdmsr(vmx_ctl_msr[n].index);
+		default1 = vmx_ctl_msr[n].default1;
+		ok = (ctrl.set & default1) == default1;
+		ok = ok && (ctrl.set & ~ctrl.clr) == 0;
+		if (ok && basic.ctrl) {
+			true_ctrl.val = rdmsr(vmx_ctl_msr[n].true_index);
+			ok = ctrl.clr == true_ctrl.clr;
+			ok = ok && ctrl.set == (true_ctrl.set | default1);
+		}
+		report(vmx_ctl_msr[n].name, ok);
+	}
+
+	fixed0 = rdmsr(MSR_IA32_VMX_CR0_FIXED0);
+	fixed1 = rdmsr(MSR_IA32_VMX_CR0_FIXED1);
+	report("MSR_IA32_VMX_IA32_VMX_CR0_FIXED0/1",
+	       ((fixed0 ^ fixed1) & ~fixed1) == 0);
+
+	fixed0 = rdmsr(MSR_IA32_VMX_CR4_FIXED0);
+	fixed1 = rdmsr(MSR_IA32_VMX_CR4_FIXED1);
+	report("MSR_IA32_VMX_IA32_VMX_CR4_FIXED0/1",
+	       ((fixed0 ^ fixed1) & ~fixed1) == 0);
+
+	val = rdmsr(MSR_IA32_VMX_VMCS_ENUM);
+	report("MSR_IA32_VMX_VMCS_ENUM",
+	       (val & 0x3e) >= 0x2a &&
+	       (val & 0xfffffffffffffc01Ull) == 0);
+
+	val = rdmsr(MSR_IA32_VMX_EPT_VPID_CAP);
+	report("MSR_IA32_VMX_EPT_VPID_CAP",
+	       (val & 0xfffff07ef9eebebeUll) == 0);
+}
+
 /* This function can only be called in guest */
 static void __attribute__((__used__)) hypercall(u32 hypercall_no)
 {

@@ -803,7 +874,7 @@ static int test_run(struct vmx_test *test)
 	regs = test->guest_regs;
 	vmcs_write(GUEST_RFLAGS, regs.rflags | 0x2);
 	launched = 0;
-	printf("\nTest suite : %s\n", test->name);
+	printf("\nTest suite: %s\n", test->name);
 	vmx_run();
 	if (vmx_off()) {
 		printf("%s : vmxoff failed.\n", __func__);

@@ -842,6 +913,7 @@ int main(void)
 		goto exit;
 	}
 	test_vmxoff();
+	test_vmx_caps();
 
 	while (vmx_tests[++i].name != NULL)
 		if (test_run(&vmx_tests[i]))

x86/vmx.h +1 -1

index 00f2842..87457b1 100644
@@ -46,12 +46,13 @@ union vmx_basic {
 	struct {
 		u32 revision;
 		u32	size:13,
-			: 3,
+			reserved1: 3,

x86/vmx.h union vmx_basic { · line 58

Quoted 10 lines

x86/vmx.h +2 -1

 			width:1,
 			dual:1,
 			type:4,
 			insouts:1,
-			ctrl:1;
+			ctrl:1,
+			reserved2:8;
 	};
 };

Thanks,

Reviewed-by Paolo Bonzini <[email protected]>

Jan Kiszka

author

2014-06-17 07:04 UTC #11 lore raw

PATCH v2 6/6 VMX: Test behavior on set and cleared save/load debug controls

In reply to Jan Kiszka

This particularly checks the case when debug controls are not to be loaded/saved on host-guest transitions.

We have to fake results related to IA32_DEBUGCTL as support for this MSR is missing KVM. The test already contains all bits required once KVM adds support.

Signed-off-by Jan Kiszka <[email protected]>

---
 x86/vmx.h       |   2 +
 x86/vmx_tests.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+)

x86/vmx.h +2 -0

index 87457b1..022f1df 100644
@@ -305,6 +305,7 @@ enum Reason {
 #define X86_EFLAGS_ZF	0x00000040 /* Zero Flag */
 
 enum Ctrl_exi {
+	EXI_SAVE_DBGCTLS	= 1UL << 2,
 	EXI_HOST_64             = 1UL << 9,
 	EXI_LOAD_PERF		= 1UL << 12,
 	EXI_INTA                = 1UL << 15,

@@ -316,6 +317,7 @@ enum Ctrl_exi {
 };
 
 enum Ctrl_ent {
+	ENT_LOAD_DBGCTLS	= 1UL << 2,
 	ENT_GUEST_64            = 1UL << 9,
 	ENT_LOAD_PAT		= 1UL << 14,
 	ENT_LOAD_EFER           = 1UL << 15,

x86/vmx_tests.c +112 -0

123 lines not on this page · raw message

Signature · 1 line

1.8.1.1.298.ge7eed54