Skip to contentSkip to conversation

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

PATCH In review Jan Kiszka 4 participants 14 messages Jun 15, 2014 – Jun 16, 2014 KVM

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

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

The tests corresponding to (and going beyond) the issues fixed in
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/123282

Jan Kiszka (5):
VMX: Add tests for CR3 and CR8 interception
VMX: Only use get_stage accessor
VMX: Test both interception and execution of instructions
VMX: Validate capability MSRs
VMX: Test behavior on set and cleared save/load debug controls

 x86/vmx.c       |  73 ++++++++++++-
 x86/vmx.h       |   9 +-
 x86/vmx_tests.c | 327 +++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 322 insertions(+), 87 deletions(-)
Signature · 1 line
1.8.1.1.298.ge7eed54
author
#1 lore raw

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

From: Jan Kiszka <[email protected]>

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..d0ce365 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		0x1
+#define FIELD_INSN_INFO		0x2
 
 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
maintainer
#2 lore raw

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

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

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

Quoted 9 lines

+++ b/x86/vmx_tests.c

x86/vmx_tests.c +2 -2
@@ -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		0x1
+#define FIELD_INSN_INFO		0x2

Small nit, using (1 << 0) and (1 << 1) would have reminded better this lazy maintainer that these fields form a bitmask. :)

Paolo

Quoted 2 lines
(quoted diff) +0 -0
 asm(
#3 lore raw

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

Quoted 6 lines · Paolo Bonzini

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

Quoted 5 lines

+++ b/x86/vmx_tests.c

x86/vmx_tests.c +0 -0
@@ -820,8 +820,8 @@ static int iobmp_exit_handler()
 #define INSN_ALWAYS_TRAP    2
 #define INSN_NEVER_TRAP        3
Quoted 7 lines

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

Quoted 4 lines
x86/vmx_tests.c +2 -2
-#define FIELD_EXIT_QUAL        0
-#define FIELD_INSN_INFO        1
+#define FIELD_EXIT_QUAL        0x1
+#define FIELD_INSN_INFO        0x2

Small nit, using (1 << 0) and (1 << 1) would have reminded better this lazy maintainer that these fields form a bitmask. :)

Yes, see also the bug in KVM constants I posted recently. I'm inclined to apply such a pattern exclusively for now on, at least where this produces no inconsistencies with existing code.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
author
#4 lore raw

PATCH 2/5 VMX: Only use get_stage accessor

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Consistently make sure we are not affected by any compiler reordering when evaluating the current stage.

Signed-off-by Jan Kiszka <[email protected]>
---
 x86/vmx_tests.c | 80 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 40 deletions(-)
x86/vmx_tests.c +40 -40
index d0ce365..bf7aa2c 100644
@@ -415,13 +415,13 @@ static void cr_shadowing_main()
 	// Test read through
 	set_stage(0);
 	guest_cr0 = read_cr0();
-	if (stage == 1)
+	if (get_stage() == 1)
 		report("Read through CR0", 0);
 	else
 		vmcall();
 	set_stage(1);
 	guest_cr4 = read_cr4();
-	if (stage == 2)
+	if (get_stage() == 2)
 		report("Read through CR4", 0);
 	else
 		vmcall();

@@ -430,13 +430,13 @@ static void cr_shadowing_main()
 	guest_cr4 = guest_cr4 ^ (X86_CR4_TSD | X86_CR4_DE);
 	set_stage(2);
 	write_cr0(guest_cr0);
-	if (stage == 3)
+	if (get_stage() == 3)
 		report("Write throuth CR0", 0);
 	else
 		vmcall();
 	set_stage(3);
 	write_cr4(guest_cr4);
-	if (stage == 4)
+	if (get_stage() == 4)
 		report("Write through CR4", 0);
 	else
 		vmcall();

@@ -444,7 +444,7 @@ static void cr_shadowing_main()
 	set_stage(4);
 	vmcall();
 	cr0 = read_cr0();
-	if (stage != 5) {
+	if (get_stage() != 5) {
 		if (cr0 == guest_cr0)
 			report("Read shadowing CR0", 1);
 		else

@@ -452,7 +452,7 @@ static void cr_shadowing_main()
 	}
 	set_stage(5);
 	cr4 = read_cr4();
-	if (stage != 6) {
+	if (get_stage() != 6) {
 		if (cr4 == guest_cr4)
 			report("Read shadowing CR4", 1);
 		else

@@ -461,13 +461,13 @@ static void cr_shadowing_main()
 	// Test write shadow (same value with shadow)
 	set_stage(6);
 	write_cr0(guest_cr0);
-	if (stage == 7)
+	if (get_stage() == 7)
 		report("Write shadowing CR0 (same value with shadow)", 0);
 	else
 		vmcall();
 	set_stage(7);
 	write_cr4(guest_cr4);
-	if (stage == 8)
+	if (get_stage() == 8)
 		report("Write shadowing CR4 (same value with shadow)", 0);
 	else
 		vmcall();

@@ -478,7 +478,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr0\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 9)
+	if (get_stage() != 9)
 		report("Write shadowing different X86_CR0_TS", 0);
 	else
 		report("Write shadowing different X86_CR0_TS", 1);

@@ -488,7 +488,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr0\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 10)
+	if (get_stage() != 10)
 		report("Write shadowing different X86_CR0_MP", 0);
 	else
 		report("Write shadowing different X86_CR0_MP", 1);

@@ -498,7 +498,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr4\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 11)
+	if (get_stage() != 11)
 		report("Write shadowing different X86_CR4_TSD", 0);
 	else
 		report("Write shadowing different X86_CR4_TSD", 1);

@@ -508,7 +508,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr4\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 12)
+	if (get_stage() != 12)
 		report("Write shadowing different X86_CR4_DE", 0);
 	else
 		report("Write shadowing different X86_CR4_DE", 1);

@@ -584,31 +584,31 @@ static int cr_shadowing_exit_handler()
 		switch (get_stage()) {
 		case 4:
 			report("Read shadowing CR0", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 5:
 			report("Read shadowing CR4", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 6:
 			report("Write shadowing CR0 (same value)", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 7:
 			report("Write shadowing CR4 (same value)", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 8:
 		case 9:
 			// 0x600 encodes "mov %esi, %cr0"
 			if (exit_qual == 0x600)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 10:
 		case 11:
 			// 0x604 encodes "mov %esi, %cr4"
 			if (exit_qual == 0x604)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		default:
 			// Should not reach here

@@ -648,7 +648,7 @@ static void iobmp_main()
 	set_stage(0);
 	inb(0x5000);
 	outb(0x0, 0x5000);
-	if (stage != 0)
+	if (get_stage() != 0)
 		report("I/O bitmap - I/O pass", 0);
 	else
 		report("I/O bitmap - I/O pass", 1);

@@ -656,39 +656,39 @@ static void iobmp_main()
 	((u8 *)io_bitmap_a)[0] = 0xFF;
 	set_stage(2);
 	inb(0x0);
-	if (stage != 3)
+	if (get_stage() != 3)
 		report("I/O bitmap - trap in", 0);
 	else
 		report("I/O bitmap - trap in", 1);
 	set_stage(3);
 	outw(0x0, 0x0);
-	if (stage != 4)
+	if (get_stage() != 4)
 		report("I/O bitmap - trap out", 0);
 	else
 		report("I/O bitmap - trap out", 1);
 	set_stage(4);
 	inl(0x0);
-	if (stage != 5)
+	if (get_stage() != 5)
 		report("I/O bitmap - I/O width, long", 0);
 	// test low/high IO port
 	set_stage(5);
 	((u8 *)io_bitmap_a)[0x5000 / 8] = (1 << (0x5000 % 8));
 	inb(0x5000);
-	if (stage == 6)
+	if (get_stage() == 6)
 		report("I/O bitmap - I/O port, low part", 1);
 	else
 		report("I/O bitmap - I/O port, low part", 0);
 	set_stage(6);
 	((u8 *)io_bitmap_b)[0x1000 / 8] = (1 << (0x1000 % 8));
 	inb(0x9000);
-	if (stage == 7)
+	if (get_stage() == 7)
 		report("I/O bitmap - I/O port, high part", 1);
 	else
 		report("I/O bitmap - I/O port, high part", 0);
 	// test partial pass
 	set_stage(7);
 	inl(0x4FFF);
-	if (stage == 8)
+	if (get_stage() == 8)
 		report("I/O bitmap - partial pass", 1);
 	else
 		report("I/O bitmap - partial pass", 0);

@@ -697,18 +697,18 @@ static void iobmp_main()
 	memset(io_bitmap_a, 0x0, PAGE_SIZE);
 	memset(io_bitmap_b, 0x0, PAGE_SIZE);
 	inl(0xFFFF);
-	if (stage == 9)
+	if (get_stage() == 9)
 		report("I/O bitmap - overrun", 1);
 	else
 		report("I/O bitmap - overrun", 0);
 	set_stage(9);
 	vmcall();
 	outb(0x0, 0x0);
-	report("I/O bitmap - ignore unconditional exiting", stage == 9);
+	report("I/O bitmap - ignore unconditional exiting", get_stage() == 9);
 	set_stage(10);
 	vmcall();
 	outb(0x0, 0x0);
-	report("I/O bitmap - unconditional exiting", stage == 11);
+	report("I/O bitmap - unconditional exiting", get_stage() == 11);
 }
 
 static int iobmp_exit_handler()

@@ -726,7 +726,7 @@ static int iobmp_exit_handler()
 		switch (get_stage()) {
 		case 0:
 		case 1:
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 2:
 			if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_BYTE)

@@ -737,7 +737,7 @@ static int iobmp_exit_handler()
 				report("I/O bitmap - I/O direction, in", 0);
 			else
 				report("I/O bitmap - I/O direction, in", 1);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 3:
 			if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_WORD)

@@ -748,36 +748,36 @@ static int iobmp_exit_handler()
 				report("I/O bitmap - I/O direction, out", 1);
 			else
 				report("I/O bitmap - I/O direction, out", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 4:
 			if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_LONG)
 				report("I/O bitmap - I/O width, long", 0);
 			else
 				report("I/O bitmap - I/O width, long", 1);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 5:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x5000)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 6:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x9000)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 7:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x4FFF)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 8:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0xFFFF)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 9:
 		case 10:
 			ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
 			vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0 & ~CPU_IO);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		default:
 			// Should not reach here

@@ -948,13 +948,13 @@ static void insn_intercept_main()
 		case INSN_CPU0:
 		case INSN_CPU1:
 		case INSN_ALWAYS_TRAP:
-			if (stage != cur_insn + 1)
+			if (get_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 (stage == cur_insn + 1)
+			if (get_stage() == cur_insn + 1)
 				report(insn_table[cur_insn].name, 0);
 			else
 				report(insn_table[cur_insn].name, 1);

@@ -985,7 +985,7 @@ static int insn_intercept_exit_handler()
 	if (insn_table[cur_insn].test_field & FIELD_INSN_INFO)
 		pass = pass && insn_table[cur_insn].insn_info == insn_info;
 	if (pass)
-		set_stage(stage + 1);
+		set_stage(get_stage() + 1);
 	vmcs_write(GUEST_RIP, guest_rip + insn_len);
 	return VMX_TEST_RESUME;
 }
Signature · 1 line
1.8.1.1.298.ge7eed54
#5 lore raw

Re: [PATCH 2/5] VMX: Only use get_stage accessor

Quoted 4 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

Consistently make sure we are not affected by any compiler reordering when evaluating the current stage.

Should we prevent accidental calls to the variable directly by moving get/set to vmx.c or a separate file in lib/x86 altogether ?

Quoted 313 lines
Signed-off-by Jan Kiszka <[email protected]>
---
 x86/vmx_tests.c | 80 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 40 deletions(-)
x86/vmx_tests.c +40 -40
index d0ce365..bf7aa2c 100644
@@ -415,13 +415,13 @@ static void cr_shadowing_main()
 	// Test read through
 	set_stage(0);
 	guest_cr0 = read_cr0();
-	if (stage == 1)
+	if (get_stage() == 1)
 		report("Read through CR0", 0);
 	else
 		vmcall();
 	set_stage(1);
 	guest_cr4 = read_cr4();
-	if (stage == 2)
+	if (get_stage() == 2)
 		report("Read through CR4", 0);
 	else
 		vmcall();

@@ -430,13 +430,13 @@ static void cr_shadowing_main()
 	guest_cr4 = guest_cr4 ^ (X86_CR4_TSD | X86_CR4_DE);
 	set_stage(2);
 	write_cr0(guest_cr0);
-	if (stage == 3)
+	if (get_stage() == 3)
 		report("Write throuth CR0", 0);
 	else
 		vmcall();
 	set_stage(3);
 	write_cr4(guest_cr4);
-	if (stage == 4)
+	if (get_stage() == 4)
 		report("Write through CR4", 0);
 	else
 		vmcall();

@@ -444,7 +444,7 @@ static void cr_shadowing_main()
 	set_stage(4);
 	vmcall();
 	cr0 = read_cr0();
-	if (stage != 5) {
+	if (get_stage() != 5) {
 		if (cr0 == guest_cr0)
 			report("Read shadowing CR0", 1);
 		else

@@ -452,7 +452,7 @@ static void cr_shadowing_main()
 	}
 	set_stage(5);
 	cr4 = read_cr4();
-	if (stage != 6) {
+	if (get_stage() != 6) {
 		if (cr4 == guest_cr4)
 			report("Read shadowing CR4", 1);
 		else

@@ -461,13 +461,13 @@ static void cr_shadowing_main()
 	// Test write shadow (same value with shadow)
 	set_stage(6);
 	write_cr0(guest_cr0);
-	if (stage == 7)
+	if (get_stage() == 7)
 		report("Write shadowing CR0 (same value with shadow)", 0);
 	else
 		vmcall();
 	set_stage(7);
 	write_cr4(guest_cr4);
-	if (stage == 8)
+	if (get_stage() == 8)
 		report("Write shadowing CR4 (same value with shadow)", 0);
 	else
 		vmcall();

@@ -478,7 +478,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr0\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 9)
+	if (get_stage() != 9)
 		report("Write shadowing different X86_CR0_TS", 0);
 	else
 		report("Write shadowing different X86_CR0_TS", 1);

@@ -488,7 +488,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr0\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 10)
+	if (get_stage() != 10)
 		report("Write shadowing different X86_CR0_MP", 0);
 	else
 		report("Write shadowing different X86_CR0_MP", 1);

@@ -498,7 +498,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr4\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 11)
+	if (get_stage() != 11)
 		report("Write shadowing different X86_CR4_TSD", 0);
 	else
 		report("Write shadowing different X86_CR4_TSD", 1);

@@ -508,7 +508,7 @@ static void cr_shadowing_main()
 		"mov %%rsi, %%cr4\n\t"
 		::"m"(tmp)
 		:"rsi", "memory", "cc");
-	if (stage != 12)
+	if (get_stage() != 12)
 		report("Write shadowing different X86_CR4_DE", 0);
 	else
 		report("Write shadowing different X86_CR4_DE", 1);

@@ -584,31 +584,31 @@ static int cr_shadowing_exit_handler()
 		switch (get_stage()) {
 		case 4:
 			report("Read shadowing CR0", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 5:
 			report("Read shadowing CR4", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 6:
 			report("Write shadowing CR0 (same value)", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 7:
 			report("Write shadowing CR4 (same value)", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 8:
 		case 9:
 			// 0x600 encodes "mov %esi, %cr0"
 			if (exit_qual == 0x600)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 10:
 		case 11:
 			// 0x604 encodes "mov %esi, %cr4"
 			if (exit_qual == 0x604)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		default:
 			// Should not reach here

@@ -648,7 +648,7 @@ static void iobmp_main()
 	set_stage(0);
 	inb(0x5000);
 	outb(0x0, 0x5000);
-	if (stage != 0)
+	if (get_stage() != 0)
 		report("I/O bitmap - I/O pass", 0);
 	else
 		report("I/O bitmap - I/O pass", 1);

@@ -656,39 +656,39 @@ static void iobmp_main()
 	((u8 *)io_bitmap_a)[0] = 0xFF;
 	set_stage(2);
 	inb(0x0);
-	if (stage != 3)
+	if (get_stage() != 3)
 		report("I/O bitmap - trap in", 0);
 	else
 		report("I/O bitmap - trap in", 1);
 	set_stage(3);
 	outw(0x0, 0x0);
-	if (stage != 4)
+	if (get_stage() != 4)
 		report("I/O bitmap - trap out", 0);
 	else
 		report("I/O bitmap - trap out", 1);
 	set_stage(4);
 	inl(0x0);
-	if (stage != 5)
+	if (get_stage() != 5)
 		report("I/O bitmap - I/O width, long", 0);
 	// test low/high IO port
 	set_stage(5);
 	((u8 *)io_bitmap_a)[0x5000 / 8] = (1 << (0x5000 % 8));
 	inb(0x5000);
-	if (stage == 6)
+	if (get_stage() == 6)
 		report("I/O bitmap - I/O port, low part", 1);
 	else
 		report("I/O bitmap - I/O port, low part", 0);
 	set_stage(6);
 	((u8 *)io_bitmap_b)[0x1000 / 8] = (1 << (0x1000 % 8));
 	inb(0x9000);
-	if (stage == 7)
+	if (get_stage() == 7)
 		report("I/O bitmap - I/O port, high part", 1);
 	else
 		report("I/O bitmap - I/O port, high part", 0);
 	// test partial pass
 	set_stage(7);
 	inl(0x4FFF);
-	if (stage == 8)
+	if (get_stage() == 8)
 		report("I/O bitmap - partial pass", 1);
 	else
 		report("I/O bitmap - partial pass", 0);

@@ -697,18 +697,18 @@ static void iobmp_main()
 	memset(io_bitmap_a, 0x0, PAGE_SIZE);
 	memset(io_bitmap_b, 0x0, PAGE_SIZE);
 	inl(0xFFFF);
-	if (stage == 9)
+	if (get_stage() == 9)
 		report("I/O bitmap - overrun", 1);
 	else
 		report("I/O bitmap - overrun", 0);
 	set_stage(9);
 	vmcall();
 	outb(0x0, 0x0);
-	report("I/O bitmap - ignore unconditional exiting", stage == 9);
+	report("I/O bitmap - ignore unconditional exiting", get_stage() == 9);
 	set_stage(10);
 	vmcall();
 	outb(0x0, 0x0);
-	report("I/O bitmap - unconditional exiting", stage == 11);
+	report("I/O bitmap - unconditional exiting", get_stage() == 11);
 }
 
 static int iobmp_exit_handler()

@@ -726,7 +726,7 @@ static int iobmp_exit_handler()
 		switch (get_stage()) {
 		case 0:
 		case 1:
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 2:
 			if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_BYTE)

@@ -737,7 +737,7 @@ static int iobmp_exit_handler()
 				report("I/O bitmap - I/O direction, in", 0);
 			else
 				report("I/O bitmap - I/O direction, in", 1);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 3:
 			if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_WORD)

@@ -748,36 +748,36 @@ static int iobmp_exit_handler()
 				report("I/O bitmap - I/O direction, out", 1);
 			else
 				report("I/O bitmap - I/O direction, out", 0);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 4:
 			if ((exit_qual & VMX_IO_SIZE_MASK) != _VMX_IO_LONG)
 				report("I/O bitmap - I/O width, long", 0);
 			else
 				report("I/O bitmap - I/O width, long", 1);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		case 5:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x5000)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 6:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x9000)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 7:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0x4FFF)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 8:
 			if (((exit_qual & VMX_IO_PORT_MASK) >> VMX_IO_PORT_SHIFT) == 0xFFFF)
-				set_stage(stage + 1);
+				set_stage(get_stage() + 1);
 			break;
 		case 9:
 		case 10:
 			ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
 			vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0 & ~CPU_IO);
-			set_stage(stage + 1);
+			set_stage(get_stage() + 1);
 			break;
 		default:
 			// Should not reach here

@@ -948,13 +948,13 @@ static void insn_intercept_main()
 		case INSN_CPU0:
 		case INSN_CPU1:
 		case INSN_ALWAYS_TRAP:
-			if (stage != cur_insn + 1)
+			if (get_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 (stage == cur_insn + 1)
+			if (get_stage() == cur_insn + 1)
 				report(insn_table[cur_insn].name, 0);
 			else
 				report(insn_table[cur_insn].name, 1);

@@ -985,7 +985,7 @@ static int insn_intercept_exit_handler()
 	if (insn_table[cur_insn].test_field & FIELD_INSN_INFO)
 		pass = pass && insn_table[cur_insn].insn_info == insn_info;
 	if (pass)
-		set_stage(stage + 1);
+		set_stage(get_stage() + 1);
 	vmcs_write(GUEST_RIP, guest_rip + insn_len);
 	return VMX_TEST_RESUME;
 }
author
#6 lore raw

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

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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 bf7aa2c..d0b67de 100644
@@ -818,7 +818,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		0x1
 #define FIELD_INSN_INFO		0x2

@@ -829,7 +828,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"

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

@@ -912,55 +912,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) {
-		set_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++) {
+		set_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, get_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 (get_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 (get_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, get_stage() == cur_insn * 2 + 1);
+
+		set_stage(cur_insn * 2 + 1);
+		vmcall();
 	}
 }
 

@@ -978,14 +979,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 == get_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 (get_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 == get_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)
-		set_stage(get_stage() + 1);
+		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)
+			set_stage(get_stage() + 1);
+	}
 	vmcs_write(GUEST_RIP, guest_rip + insn_len);
 	return VMX_TEST_RESUME;
 }
Signature · 1 line
1.8.1.1.298.ge7eed54
author
#7 lore raw

PATCH 4/5 VMX: Validate capability MSRs

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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 | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 x86/vmx.h |  5 +++--
 2 files changed, 75 insertions(+), 3 deletions(-)
x86/vmx.c +72 -1
index 1182eef..84c514b 100644
@@ -635,6 +635,76 @@ 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, true_val, default1, fixed0, fixed1;
+	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++) {
+		val = rdmsr(vmx_ctl_msr[n].index);
+		default1 = vmx_ctl_msr[n].default1;
+		ok = (val & default1) == default1 &&
+			((((u32)val) ^ (val >> 32)) & ~(val >> 32)) == 0;
+		if (ok && basic.ctrl) {
+			true_val = rdmsr(vmx_ctl_msr[n].true_index);
+			ok = (val >> 32) == (true_val >> 32) &&
+				((u32)(val ^ true_val) & ~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)
 {

@@ -777,7 +847,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__);

@@ -816,6 +886,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 69a5385..38ec3c5 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
maintainer
#8 lore raw

Re: [PATCH 4/5] VMX: Validate capability MSRs

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

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

Quoted 5 lines
x86/vmx.c +5 -0
+	for (n = 0; n < ARRAY_SIZE(vmx_ctl_msr); n++) {
+		val = rdmsr(vmx_ctl_msr[n].index);
+		default1 = vmx_ctl_msr[n].default1;
+		ok = (val & default1) == default1 &&
+			((((u32)val) ^ (val >> 32)) & ~(val >> 32)) == 0;

Ouch, this can sure be make this more readable.

Please unify these:

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 {
        u64 val;
        struct {
                u32 set, clr;
        };
};

into a single "union vmx_ctl_msr", and use this union for val and true_val as well.

Paolo

Quoted 6 lines
x86/vmx.c +6 -0
+		if (ok && basic.ctrl) {
+			true_val = rdmsr(vmx_ctl_msr[n].true_index);
+			ok = (val >> 32) == (true_val >> 32) &&
+				((u32)(val ^ true_val) & ~default1) == 0;
+		}
+		report(vmx_ctl_msr[n].name, ok);
#9 lore raw

Re: [PATCH 4/5] VMX: Validate capability MSRs

Quoted 40 lines · Paolo Bonzini

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

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

Quoted 5 lines
x86/vmx.c +5 -0
+    for (n = 0; n < ARRAY_SIZE(vmx_ctl_msr); n++) {
+        val = rdmsr(vmx_ctl_msr[n].index);
+        default1 = vmx_ctl_msr[n].default1;
+        ok = (val & default1) == default1 &&
+            ((((u32)val) ^ (val >> 32)) & ~(val >> 32)) == 0;

Ouch, this can sure be make this more readable.

Please unify these:

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 {
        u64 val;
        struct {
                u32 set, clr;
        };
};
Quoted 2 lines

into a single "union vmx_ctl_msr", and use this union for val and true_val as well.

OK, will do.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
author
#10 lore raw

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

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)
x86/vmx.h +2 -0
index 38ec3c5..3c4830f 100644
@@ -326,6 +326,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,

@@ -337,6 +338,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 +110 -0

121 lines not on this page · raw message

Signature · 1 line
1.8.1.1.298.ge7eed54
maintainer
#11 lore raw

Re: [PATCH 5/5] VMX: Test behavior on set and cleared save/load debug controls

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

Quoted 63 lines

From: Jan Kiszka <[email protected]>

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 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)
x86/vmx.h +2 -0
index 38ec3c5..3c4830f 100644
@@ -326,6 +326,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,

@@ -337,6 +338,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 +20 -0
index d0b67de..0f4cfc2 100644
@@ -1406,6 +1406,114 @@ static int interrupt_exit_handler(void)
 	return VMX_TEST_VMEXIT;
 }
 
+static int dbgctls_init(struct vmcs *vmcs)
+{
+	u64 dr7 = 0x402;
+	u64 zero = 0;
+
+	msr_bmp_init();
+	asm volatile(
+		"mov %0,%%dr0\n\t"
+		"mov %0,%%dr1\n\t"
+		"mov %0,%%dr2\n\t"
+		"mov %1,%%dr7\n\t"
+		: : "r" (zero), "r" (dr7));
+	wrmsr(MSR_IA32_DEBUGCTLMSR, 0x1);
+	vmcs_write(GUEST_DR7, 0x404);
+	vmcs_write(GUEST_DEBUGCTL, 0x2);
+
+	vmcs_write(ENT_CONTROLS, vmcs_read(ENT_CONTROLS) | ENT_LOAD_DBGCTLS);
+	vmcs_write(EXI_CONTROLS, vmcs_read(EXI_CONTROLS) | EXI_SAVE_DBGCTLS);
+
+	return VMX_TEST_START;

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1438

Quoted 10 lines
x86/vmx_tests.c +10 -0
+}
+
+static void dbgctls_main(void)
+{
+	u64 dr7, debugctl;
+
+	asm volatile("mov %%dr7,%0" : "=r" (dr7));
+	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
+	debugctl = 0x2; /* KVM does not support DEBUGCTL so far */
+	report("Load debug controls", dr7 == 0x404 && debugctl == 0x2);

Please comment instead like this:

	report("Load debug controls", dr7 == 0x404 /* && debugctl == 0x2 */ );

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1459

Quoted 4 lines
x86/vmx_tests.c +4 -0
+	asm volatile("mov %%dr7,%0" : "=r" (dr7));
+	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
+	debugctl = 0x1; /* no KVM support */
+	report("Guest=host debug controls", dr7 == 0x402 && debugctl == 0x1);

Same here.

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1486

Quoted 3 lines
x86/vmx_tests.c +3 -0
+			if (dr7 == 0x400 && debugctl == 0 &&
+			    vmcs_read(GUEST_DR7) == 0x408 &&
+			    vmcs_read(GUEST_DEBUGCTL) == /* 0x3 no KVM support*/ 0x2)

and here.

Quoted 8 lines
x86/vmx_tests.c +8 -0
+				set_stage(1);
+			break;
+		case 2:
+			dr7 = 0x402;
+			asm volatile("mov %0,%%dr7" : : "r" (dr7));
+			wrmsr(MSR_IA32_DEBUGCTLMSR, 0x1);
+			vmcs_write(GUEST_DR7, 0x404);
+			vmcs_write(GUEST_DEBUGCTL, 0x2);

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1504

Quoted 10 lines
x86/vmx_tests.c +10 -0
+
+			vmcs_write(ENT_CONTROLS,
+				vmcs_read(ENT_CONTROLS) & ~ENT_LOAD_DBGCTLS);
+			vmcs_write(EXI_CONTROLS,
+				vmcs_read(EXI_CONTROLS) & ~EXI_SAVE_DBGCTLS);
+			break;
+		case 3:
+			if (dr7 == 0x400 && debugctl == 0 &&
+			    vmcs_read(GUEST_DR7) == 0x404 &&
+			    vmcs_read(GUEST_DEBUGCTL) == 0x2)

and here too, even though it happens to work.

Paolo

Quoted 24 lines
x86/vmx_tests.c +14 -0
+				set_stage(4);
+			break;
+		}
+		vmcs_write(GUEST_RIP, guest_rip + insn_len);
+		return VMX_TEST_RESUME;
+	default:
+		printf("Unknown exit reason, %d\n", reason);
+		print_vmexit_info();
+	}
+	return VMX_TEST_VMEXIT;
+}
+
 /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
 struct vmx_test vmx_tests[] = {
 	{ "null", NULL, basic_guest_main, basic_exit_handler, NULL, {0} },

@@ -1425,5 +1533,7 @@ struct vmx_test vmx_tests[] = {
 	{ "EPT framework", ept_init, ept_main, ept_exit_handler, NULL, {0} },
 	{ "interrupt", interrupt_init, interrupt_main,
 		interrupt_exit_handler, NULL, {0} },
+	{ "debug controls", dbgctls_init, dbgctls_main, dbgctls_exit_handler,
+		NULL, {0} },
 	{ NULL, NULL, NULL, NULL, NULL, {0} },
 };
#12 lore raw

Re: [PATCH 5/5] VMX: Test behavior on set and cleared save/load debug controls

Quoted 104 lines · Paolo Bonzini

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

Quoted 63 lines

From: Jan Kiszka <[email protected]>

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 | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)
x86/vmx.h +2 -0
index 38ec3c5..3c4830f 100644
@@ -326,6 +326,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,

@@ -337,6 +338,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 +20 -0
index d0b67de..0f4cfc2 100644
@@ -1406,6 +1406,114 @@ static int interrupt_exit_handler(void)
 	return VMX_TEST_VMEXIT;
 }
 
+static int dbgctls_init(struct vmcs *vmcs)
+{
+	u64 dr7 = 0x402;
+	u64 zero = 0;
+
+	msr_bmp_init();
+	asm volatile(
+		"mov %0,%%dr0\n\t"
+		"mov %0,%%dr1\n\t"
+		"mov %0,%%dr2\n\t"
+		"mov %1,%%dr7\n\t"
+		: : "r" (zero), "r" (dr7));
+	wrmsr(MSR_IA32_DEBUGCTLMSR, 0x1);
+	vmcs_write(GUEST_DR7, 0x404);
+	vmcs_write(GUEST_DEBUGCTL, 0x2);
+
+	vmcs_write(ENT_CONTROLS, vmcs_read(ENT_CONTROLS) | ENT_LOAD_DBGCTLS);
+	vmcs_write(EXI_CONTROLS, vmcs_read(EXI_CONTROLS) | EXI_SAVE_DBGCTLS);
+
+	return VMX_TEST_START;

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1438

Quoted 10 lines
x86/vmx_tests.c +10 -0
+}
+
+static void dbgctls_main(void)
+{
+	u64 dr7, debugctl;
+
+	asm volatile("mov %%dr7,%0" : "=r" (dr7));
+	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
+	debugctl = 0x2; /* KVM does not support DEBUGCTL so far */
+	report("Load debug controls", dr7 == 0x404 && debugctl == 0x2);

Please comment instead like this:

	report("Load debug controls", dr7 == 0x404 /* && debugctl == 0x2 */ );

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1459

Quoted 4 lines
x86/vmx_tests.c +4 -0
+	asm volatile("mov %%dr7,%0" : "=r" (dr7));
+	debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
+	debugctl = 0x1; /* no KVM support */
+	report("Guest=host debug controls", dr7 == 0x402 && debugctl == 0x1);

Same here.

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1486

Quoted 3 lines
x86/vmx_tests.c +3 -0
+			if (dr7 == 0x400 && debugctl == 0 &&
+			    vmcs_read(GUEST_DR7) == 0x408 &&
+			    vmcs_read(GUEST_DEBUGCTL) == /* 0x3 no KVM support*/ 0x2)

and here.

Quoted 12 lines
x86/vmx_tests.c +12 -0
+				set_stage(1);
+			break;
+		case 2:
+			dr7 = 0x402;
+			asm volatile("mov %0,%%dr7" : : "r" (dr7));
+			wrmsr(MSR_IA32_DEBUGCTLMSR, 0x1);
+			vmcs_write(GUEST_DR7, 0x404);
+			vmcs_write(GUEST_DEBUGCTL, 0x2);
+
+			vmcs_write(ENT_CONTROLS,
+				vmcs_read(ENT_CONTROLS) & ~ENT_LOAD_DBGCTLS);
+			vmcs_write(EXI_CONTROLS,
Quoted 8 lines

x86/vmx_tests.c static int interrupt_exit_handler(void) · line 1504

Quoted 6 lines
x86/vmx_tests.c +6 -0
+				vmcs_read(EXI_CONTROLS) & ~EXI_SAVE_DBGCTLS);
+			break;
+		case 3:
+			if (dr7 == 0x400 && debugctl == 0 &&
+			    vmcs_read(GUEST_DR7) == 0x404 &&
+			    vmcs_read(GUEST_DEBUGCTL) == 0x2)

and here too, even though it happens to work.

OK, no problem.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
maintainer
#13 lore raw
In reply to Jan Kiszka

Il 15/06/2014 16:24, Jan Kiszka ha scritto:

Quoted 10 lines

The tests corresponding to (and going beyond) the issues fixed in
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/123282

Jan Kiszka (5):
VMX: Add tests for CR3 and CR8 interception
VMX: Only use get_stage accessor
VMX: Test both interception and execution of instructions
VMX: Validate capability MSRs
VMX: Test behavior on set and cleared save/load debug controls

Quoted 5 lines
 x86/vmx.c       |  73 ++++++++++++-
 x86/vmx.h       |   9 +-
 x86/vmx_tests.c | 327 +++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 322 insertions(+), 87 deletions(-)

Thanks, I'll apply the first three patches as soon as Linus releases -rc1 and the corresponding KVM bits hit kernel.org.

I replied with comments to the last two patches.

Paolo