[PATCH kvm-unit-tests 0/4] x86: vmx: vm{launch,resume} improvements

[PATCH kvm-unit-tests 0/4] x86: vmx: vm{launch,resume} improvements

[Peter Feiner]

Fixed some bugs in the vmlaunch / vmresume code and improved entry
failure logic.

Peter Feiner (4):
  x86: vmx: fix vm{launch,resume} asm
  x86: vmx: fix vm{launch,resume} early exit logic
  x86: vmx: clean up vm{launch,resume} asm
  x86: vmx: better vm{launch,resume} error handling

 lib/x86/processor.h |   4 ++
 x86/vmx.c           | 169 +++++++++++++++++++++++++++++++++++-----------------
 x86/vmx.h           |  27 +++++----
 x86/vmx_tests.c     |  49 +++++++++------
 4 files changed, 166 insertions(+), 83 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH kvm-unit-tests 1/4] x86: vmx: fix vm{launch,resume} asm

[Peter Feiner]

Fixed two problems:

    1) Output operands with the '=' constraint are dead-on-arrival.
       Thus gcc was free to ignore the fail = 0 initialization. Since
       the asm only set fail when vm{launch,resume} exited early, fail
       was technically undefined when vm{launch,resume} entered the
       guest! Using the '+' constraint instead tells gcc the value is
       live.

    2) On early vm{launch,resume} failure, the 'setbe %0' instruction
       was running with the guest's GPRs since SAVE_GPR_C hadn't
       run yet. Since %0 is typically replaced with OFFSET(%%rbp),
       some arbitrary guest stack memory is modified. Solution is
       to restore the host's registers before using any asm code
       generated by gcc.

Signed-off-by: Peter Feiner <pfeiner@google.com>
---
 x86/vmx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 107a005..b2e015f 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -925,11 +925,14 @@ static int vmx_run()
 			"1: "
 			"vmresume\n\t"
 			"2: "
+			SAVE_GPR_C
 			"setbe %0\n\t"
+			"jmp 3f\n\t"
 			"vmx_return:\n\t"
 			SAVE_GPR_C
+			"3: \n\t"
 			SAVE_RFLAGS
-			: "=m"(fail)
+			: "+m"(fail)
 			: "m"(launched), "i"(HOST_RSP)
 			: "rdi", "rsi", "memory", "cc"
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH kvm-unit-tests 2/4] x86: vmx: fix vm{launch,resume} early exit logic

[Peter Feiner]

If vmlaunch or vmresume returns immediately, there's definitely been
an error -- rflags only needs to be consulted to determine what
error occurred. By setting fail=1 unconditionally, the test framework
will now detect problems with rflags *not* being set properly.

Signed-off-by: Peter Feiner <pfeiner@google.com>
---
 x86/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index b2e015f..27e85eb 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -911,6 +911,7 @@ static int vmx_run()
 	u32 ret = 0, fail = 0;
 
 	while (1) {
+
 		asm volatile (
 			"mov %%rsp, %%rsi\n\t"
 			"mov %2, %%rdi\n\t"
@@ -926,7 +927,7 @@ static int vmx_run()
 			"vmresume\n\t"
 			"2: "
 			SAVE_GPR_C
-			"setbe %0\n\t"
+			"movl $1, %0\n\t"
 			"jmp 3f\n\t"
 			"vmx_return:\n\t"
 			SAVE_GPR_C
-- 
2.8.0.rc3.226.g39d4020

[PATCH kvm-unit-tests 3/4] x86: vmx: clean up vm{launch,resume} asm

[Peter Feiner]

Did a few things to make the code easier to fllow:

	* named I/O operands
	* removed pointless LOAD_RFLAGS
	* replaced global host_rflags with local
	* only save rflags when they're meaningful (i.e., after early failure)
	* got rid of unecessary %rsi scratch register

Signed-off-by: Peter Feiner <pfeiner@google.com>
---
 x86/vmx.c | 23 +++++++++++------------
 x86/vmx.h |  8 --------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 27e85eb..67f8650 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -46,7 +46,6 @@ struct regs regs;
 struct vmx_test *current;
 u64 hypercall_field;
 bool launched;
-u64 host_rflags;
 
 union vmx_basic basic;
 union vmx_ctrl_msr ctrl_pin_rev;
@@ -909,33 +908,33 @@ static int exit_handler()
 static int vmx_run()
 {
 	u32 ret = 0, fail = 0;
+	unsigned long host_rflags;
 
 	while (1) {
 
 		asm volatile (
-			"mov %%rsp, %%rsi\n\t"
-			"mov %2, %%rdi\n\t"
-			"vmwrite %%rsi, %%rdi\n\t"
-
+			"mov %[HOST_RSP], %%rdi\n\t"
+			"vmwrite %%rsp, %%rdi\n\t"
 			LOAD_GPR_C
-			"cmpl $0, %1\n\t"
+			"cmpl $0, %[launched]\n\t"
 			"jne 1f\n\t"
-			LOAD_RFLAGS
 			"vmlaunch\n\t"
 			"jmp 2f\n\t"
 			"1: "
 			"vmresume\n\t"
 			"2: "
 			SAVE_GPR_C
-			"movl $1, %0\n\t"
+			"pushf\n\t"
+			"pop %%rdi\n\t"
+			"mov %%rdi, %[host_rflags]\n\t"
+			"movl $1, %[fail]\n\t"
 			"jmp 3f\n\t"
 			"vmx_return:\n\t"
 			SAVE_GPR_C
 			"3: \n\t"
-			SAVE_RFLAGS
-			: "+m"(fail)
-			: "m"(launched), "i"(HOST_RSP)
-			: "rdi", "rsi", "memory", "cc"
+			: [fail]"+m"(fail), [host_rflags]"=m"(host_rflags)
+			: [launched]"m"(launched), [HOST_RSP]"i"(HOST_RSP)
+			: "rdi", "memory", "cc"
 
 		);
 		if (fail)
diff --git a/x86/vmx.h b/x86/vmx.h
index 34e9be4..aba5642 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -396,14 +396,6 @@ enum Ctrl1 {
 
 #define LOAD_GPR_C	SAVE_GPR_C
 
-#define SAVE_RFLAGS		\
-	"pushf\n\t"		\
-	"pop host_rflags\n\t"
-
-#define LOAD_RFLAGS		\
-	"push host_rflags\n\t"	\
-	"popf\n\t"
-
 #define VMX_IO_SIZE_MASK	0x7
 #define _VMX_IO_BYTE		0
 #define _VMX_IO_WORD		1
-- 
2.8.0.rc3.226.g39d4020

[PATCH kvm-unit-tests 4/4] x86: vmx: better vm{launch,resume} error handling

[Peter Feiner]

This patch splits out entry error handling from other exit handling
since most tests don't expect entry errors and thus don't check the
conditions properly. Also enables tests for early entry errors (i.e.,
an entry_error_handler can return VMX_TEST_RESUME).

Consolidates and simplifies control flow. Now, vmx_run is the central
validation point for exit handler statuses rather than splitting the
responsibility between exit_handler and vmx_run.

Signed-off-by: Peter Feiner <pfeiner@google.com>
---
 lib/x86/processor.h |   4 ++
 x86/vmx.c           | 142 ++++++++++++++++++++++++++++++++++++----------------
 x86/vmx.h           |  19 ++++++-
 x86/vmx_tests.c     |  49 +++++++++++-------
 4 files changed, 151 insertions(+), 63 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index ce779d1..ee7f180 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -31,7 +31,11 @@
 #define X86_CR4_PKE    0x00400000
 
 #define X86_EFLAGS_CF  0x00000001
+#define X86_EFLAGS_PF  0x00000004
+#define X86_EFLAGS_AF  0x00000010
 #define X86_EFLAGS_ZF  0x00000040
+#define X86_EFLAGS_SF  0x00000080
+#define X86_EFLAGS_OF  0x00000800
 #define X86_EFLAGS_AC  0x00040000
 
 #define X86_IA32_EFER          0xc0000080
diff --git a/x86/vmx.c b/x86/vmx.c
index 67f8650..6618008 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -157,6 +157,52 @@ void print_vmexit_info()
 		regs.r12, regs.r13, regs.r14, regs.r15);
 }
 
+void
+print_vmentry_failure_info(struct vmentry_failure *failure) {
+	if (failure->early) {
+		printf("Early %s failure: ", failure->instr);
+		switch (failure->flags & VMX_ENTRY_FLAGS) {
+		case X86_EFLAGS_ZF:
+			printf("current-VMCS pointer is not valid.\n");
+			break;
+		case X86_EFLAGS_CF:
+			printf("error number is %ld. See Intel 30.4.\n",
+			       vmcs_read(VMX_INST_ERROR));
+			break;
+		default:
+			printf("unexpected flags %lx!\n", failure->flags);
+		}
+	} else {
+		u64 reason = vmcs_read(EXI_REASON);
+		u64 qual = vmcs_read(EXI_QUALIFICATION);
+
+		printf("Non-early %s failure (reason=0x%lx, qual=0x%lx): ",
+			failure->instr, reason, qual);
+
+		switch (reason & 0xff) {
+		case VMX_FAIL_STATE:
+			printf("invalid guest state\n");
+			break;
+		case VMX_FAIL_MSR:
+			printf("MSR loading\n");
+			break;
+		case VMX_FAIL_MCHECK:
+			printf("machine-check event\n");
+			break;
+		default:
+			printf("unexpected basic exit reason %ld\n",
+			       reason & 0xff);
+		}
+
+		if (!(reason & VMX_ENTRY_FAILURE))
+			printf("\tVMX_ENTRY_FAILURE BIT NOT SET!\n");
+
+		if (reason & 0x7fff0000)
+			printf("\tRESERVED BITS SET!\n");
+	}
+}
+
+
 static void test_vmclear(void)
 {
 	struct vmcs *tmp_root;
@@ -890,27 +936,34 @@ static int exit_handler()
 	else
 		ret = current->exit_handler();
 	vmcs_write(GUEST_RFLAGS, regs.rflags);
-	switch (ret) {
-	case VMX_TEST_VMEXIT:
-	case VMX_TEST_RESUME:
-		return ret;
-	case VMX_TEST_EXIT:
-		break;
-	default:
-		printf("ERROR : Invalid exit_handler return val %d.\n"
-			, ret);
-	}
-	print_vmexit_info();
-	abort();
-	return 0;
+
+	return ret;
+}
+
+/*
+ * Called if vmlaunch or vmresume fails.
+ *	@early    - failure due to "VMX controls and host-state area" (26.2)
+ *	@vmlaunch - was this a vmlaunch or vmresume
+ *	@rflags   - host rflags
+ */
+static int
+entry_failure_handler(struct vmentry_failure *failure)
+{
+	if (current->entry_failure_handler)
+		return current->entry_failure_handler(failure);
+	else
+		return VMX_TEST_EXIT;
 }
 
 static int vmx_run()
 {
-	u32 ret = 0, fail = 0;
 	unsigned long host_rflags;
 
 	while (1) {
+		u32 ret;
+		u32 fail = 0;
+		bool entered;
+		struct vmentry_failure failure;
 
 		asm volatile (
 			"mov %[HOST_RSP], %%rdi\n\t"
@@ -937,39 +990,44 @@ static int vmx_run()
 			: "rdi", "memory", "cc"
 
 		);
-		if (fail)
-			ret = launched ? VMX_TEST_RESUME_ERR :
-				VMX_TEST_LAUNCH_ERR;
-		else {
+
+		entered = !fail && !(vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE);
+
+		if (entered) {
+			/*
+			 * VMCS isn't in "launched" state if there's been any
+			 * entry failure (early or otherwise).
+			 */
 			launched = 1;
 			ret = exit_handler();
+		} else {
+			failure.flags = host_rflags;
+			failure.vmlaunch = !launched;
+			failure.instr = launched ? "vmresume" : "vmlaunch";
+			failure.early = fail;
+			ret = entry_failure_handler(&failure);
 		}
-		if (ret != VMX_TEST_RESUME)
+
+		switch (ret) {
+		case VMX_TEST_RESUME:
+			continue;
+		case VMX_TEST_VMEXIT:
+			return 0;
+		case VMX_TEST_EXIT:
 			break;
+		default:
+			printf("ERROR : Invalid %s_handler return val %d.\n",
+			       entered ? "exit" : "entry_failure",
+			       ret);
+			break;
+		}
+
+		if (entered)
+			print_vmexit_info();
+		else
+			print_vmentry_failure_info(&failure);
+		abort();
 	}
-	launched = 0;
-	switch (ret) {
-	case VMX_TEST_VMEXIT:
-		return 0;
-	case VMX_TEST_LAUNCH_ERR:
-		printf("%s : vmlaunch failed.\n", __func__);
-		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
-			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
-			printf("\tvmlaunch set wrong flags\n");
-		report("test vmlaunch", 0);
-		break;
-	case VMX_TEST_RESUME_ERR:
-		printf("%s : vmresume failed.\n", __func__);
-		if ((!(host_rflags & X86_EFLAGS_CF) && !(host_rflags & X86_EFLAGS_ZF))
-			|| ((host_rflags & X86_EFLAGS_CF) && (host_rflags & X86_EFLAGS_ZF)))
-			printf("\tvmresume set wrong flags\n");
-		report("test vmresume", 0);
-		break;
-	default:
-		printf("%s : unhandled ret from exit_handler, ret=%d.\n", __func__, ret);
-		break;
-	}
-	return 1;
 }
 
 static int test_run(struct vmx_test *test)
diff --git a/x86/vmx.h b/x86/vmx.h
index aba5642..0cb995d 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -32,6 +32,17 @@ struct regs {
 	u64 rflags;
 };
 
+struct vmentry_failure {
+	/* Did a vmlaunch or vmresume fail? */
+	bool vmlaunch;
+	/* Instruction mnemonic (for convenience). */
+	const char *instr;
+	/* Did the instruction return right away, or did we jump to HOST_RIP? */
+	bool early;
+	/* Contents of [re]flags after failed entry. */
+	unsigned long flags;
+};
+
 struct vmx_test {
 	const char *name;
 	int (*init)(struct vmcs *vmcs);
@@ -39,6 +50,7 @@ struct vmx_test {
 	int (*exit_handler)();
 	void (*syscall_handler)(u64 syscall_no);
 	struct regs guest_regs;
+	int (*entry_failure_handler)(struct vmentry_failure *failure);
 	struct vmcs *vmcs;
 	int exits;
 };
@@ -249,6 +261,10 @@ enum Encoding {
 	HOST_RIP		= 0x6c16ul
 };
 
+#define VMX_ENTRY_FAILURE	(1ul << 31)
+#define VMX_ENTRY_FLAGS		(X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \
+				 X86_EFLAGS_ZF | X86_EFLAGS_SF | X86_EFLAGS_OF)
+
 enum Reason {
 	VMX_EXC_NMI		= 0,
 	VMX_EXTINT		= 1,
@@ -413,8 +429,6 @@ enum Ctrl1 {
 #define VMX_TEST_VMEXIT		1
 #define VMX_TEST_EXIT		2
 #define VMX_TEST_RESUME		3
-#define VMX_TEST_LAUNCH_ERR	4
-#define VMX_TEST_RESUME_ERR	5
 
 #define HYPERCALL_BIT		(1ul << 12)
 #define HYPERCALL_MASK		0xFFF
@@ -554,6 +568,7 @@ static inline void invvpid(unsigned long type, u16 vpid, u64 gva)
 }
 
 void print_vmexit_info();
+void print_vmentry_failure_info(struct vmentry_failure *failure);
 void ept_sync(int type, u64 eptp);
 void vpid_sync(int type, u16 vpid);
 void install_ept_entry(unsigned long *pml4, int pte_level,
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 1ed0b0c..0145cad 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -1523,24 +1523,35 @@ static int msr_switch_exit_handler()
 	ulong reason;
 
 	reason = vmcs_read(EXI_REASON);
-	switch (reason) {
-	case 0x80000000 | VMX_FAIL_MSR:
-		if (vmx_get_test_stage() == 3) {
-			report("VM entry MSR load: try to load FS_BASE",
-				vmcs_read(EXI_QUALIFICATION) == 1);
-			return VMX_TEST_VMEXIT;
-		}
-		break;
-	case VMX_VMCALL:
-		if (vmx_get_test_stage() == 2) {
-			report("VM exit MSR store",
-				exit_msr_store[0].value == MSR_MAGIC + 1);
-			report("VM exit MSR load",
-				rdmsr(MSR_KERNEL_GS_BASE) == MSR_MAGIC + 2);
-			vmx_set_test_stage(3);
-			entry_msr_load[0].index = MSR_FS_BASE;
-			return VMX_TEST_RESUME;
-		}
+	if (reason == VMX_VMCALL && vmx_get_test_stage() == 2) {
+		report("VM exit MSR store",
+			exit_msr_store[0].value == MSR_MAGIC + 1);
+		report("VM exit MSR load",
+			rdmsr(MSR_KERNEL_GS_BASE) == MSR_MAGIC + 2);
+		vmx_set_test_stage(3);
+		entry_msr_load[0].index = MSR_FS_BASE;
+		return VMX_TEST_RESUME;
+	}
+	printf("ERROR %s: unexpected stage=%u or reason=%lu\n",
+		__func__, vmx_get_test_stage(), reason);
+	return VMX_TEST_EXIT;
+}
+
+static int msr_switch_entry_failure(struct vmentry_failure *failure)
+{
+	ulong reason;
+
+	if (failure->early) {
+		printf("ERROR %s: early exit\n", __func__);
+		return VMX_TEST_EXIT;
+	}
+
+	reason = vmcs_read(EXI_REASON);
+	if (reason == (VMX_ENTRY_FAILURE | VMX_FAIL_MSR) &&
+	    vmx_get_test_stage() == 3) {
+		report("VM entry MSR load: try to load FS_BASE",
+			vmcs_read(EXI_QUALIFICATION) == 1);
+		return VMX_TEST_VMEXIT;
 	}
 	printf("ERROR %s: unexpected stage=%u or reason=%lu\n",
 		__func__, vmx_get_test_stage(), reason);
@@ -1608,7 +1619,7 @@ struct vmx_test vmx_tests[] = {
 	{ "debug controls", dbgctls_init, dbgctls_main, dbgctls_exit_handler,
 		NULL, {0} },
 	{ "MSR switch", msr_switch_init, msr_switch_main,
-		msr_switch_exit_handler, NULL, {0} },
+		msr_switch_exit_handler, NULL, {0}, msr_switch_entry_failure },
 	{ "vmmcall", vmmcall_init, vmmcall_main, vmmcall_exit_handler, NULL, {0} },
 	{ NULL, NULL, NULL, NULL, NULL, {0} },
 };
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH kvm-unit-tests 2/4] x86: vmx: fix vm{launch,resume} early exit logic

[Paolo Bonzini]

On 18/03/2016 23:39, Peter Feiner wrote:
> If vmlaunch or vmresume returns immediately, there's definitely been
> an error -- rflags only needs to be consulted to determine what
> error occurred. By setting fail=1 unconditionally, the test framework
> will now detect problems with rflags *not* being set properly.

You should still do the setbe, and follow it with orl $2, %0.  Then if
fail==2 there is a problem.

Paolo

> Signed-off-by: Peter Feiner <pfeiner@google.com>
> ---
>  x86/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/x86/vmx.c b/x86/vmx.c
> index b2e015f..27e85eb 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -911,6 +911,7 @@ static int vmx_run()
>  	u32 ret = 0, fail = 0;
>  
>  	while (1) {
> +
>  		asm volatile (
>  			"mov %%rsp, %%rsi\n\t"
>  			"mov %2, %%rdi\n\t"
> @@ -926,7 +927,7 @@ static int vmx_run()
>  			"vmresume\n\t"
>  			"2: "
>  			SAVE_GPR_C
> -			"setbe %0\n\t"
> +			"movl $1, %0\n\t"
>  			"jmp 3f\n\t"
>  			"vmx_return:\n\t"
>  			SAVE_GPR_C
> 

Re: [PATCH kvm-unit-tests 2/4] x86: vmx: fix vm{launch,resume} early exit logic

[Paolo Bonzini]

On 19/03/2016 10:18, Paolo Bonzini wrote:
> 
> 
> On 18/03/2016 23:39, Peter Feiner wrote:
>> If vmlaunch or vmresume returns immediately, there's definitely been
>> an error -- rflags only needs to be consulted to determine what
>> error occurred. By setting fail=1 unconditionally, the test framework
>> will now detect problems with rflags *not* being set properly.
> 
> You should still do the setbe, and follow it with orl $2, %0.  Then if
> fail==2 there is a problem.

Nevermind, this is checked already below.

Paolo

Re: [PATCH kvm-unit-tests 0/4] x86: vmx: vm{launch,resume} improvements

[Paolo Bonzini]

On 18/03/2016 23:39, Peter Feiner wrote:
> Fixed some bugs in the vmlaunch / vmresume code and improved entry
> failure logic.
> 
> Peter Feiner (4):
>   x86: vmx: fix vm{launch,resume} asm
>   x86: vmx: fix vm{launch,resume} early exit logic
>   x86: vmx: clean up vm{launch,resume} asm
>   x86: vmx: better vm{launch,resume} error handling
> 
>  lib/x86/processor.h |   4 ++
>  x86/vmx.c           | 169 +++++++++++++++++++++++++++++++++++-----------------
>  x86/vmx.h           |  27 +++++----
>  x86/vmx_tests.c     |  49 +++++++++------
>  4 files changed, 166 insertions(+), 83 deletions(-)
> 

Thanks, applied.

Paolo