[kvm-unit-tests PATCH v2 0/4] Add additional testing for routing L2 exceptions

[kvm-unit-tests PATCH v2 0/4] Add additional testing for routing L2 exceptions

[Aaron Lewis]

In a previous series testing was added to verify that when a #PF occured
in L2 the exception was routed to the correct place.  In this series
other exceptions are tested (ie: #GP, #UD, #DE, #DB, #BP, #AC).

The first two commits in this series are bug fixes that were discovered
while making these changes.  The last two implement the tests.

v1 -> v2:
 - Add guest_stack_top and guest_syscall_stack_top for aligning L2's
   stacks.
 - Refactor test to make it more extensible (ie: Added
   vmx_exception_tests array and framework around it).
 - Split test into 2 commits:
   1. Test infrustructure.
   2. Test cases.

Aaron Lewis (4):
  x86: Fix a #GP from occurring in usermode library's exception handlers
  x86: Align L2's stacks
  x86: Add a test framework for nested_vmx_reflect_vmexit() testing
  x86: Add test coverage for nested_vmx_reflect_vmexit() testing

 lib/x86/desc.c     |   2 +-
 lib/x86/desc.h     |   5 ++
 lib/x86/usermode.c |   3 +
 x86/unittests.cfg  |   7 ++
 x86/vmx.c          |  28 ++++++--
 x86/vmx.h          |   2 +
 x86/vmx_tests.c    | 161 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 201 insertions(+), 7 deletions(-)

-- 
2.34.1.173.g76aa8bc2d0-goog

[kvm-unit-tests PATCH v2 1/4] x86: Fix a #GP from occurring in usermode library's exception handlers

[Aaron Lewis]

When handling an exception in usermode.c the exception handler #GPs when
executing IRET to return from the exception handler.  This happens
because the stack segment selector does not have the same privilege
level as the return code segment selector.  Set the stack segment
selector to match the code segment selector's privilege level to fix the
issue.

This problem has been disguised in kvm-unit-tests because a #GP
exception handler has been registered with run_in_user() for the tests
that are currently using this feature.  With a #GP exception handler
registered, the first exception will be processed then #GP on the
IRET. The IRET from the second #GP will then succeed, and the subsequent
lngjmp() will restore RSP to a sane value.  But if no #GP handler is
installed, e.g. if a test wants to handle only #ACs, the #GP on the
initial IRET will be fatal.

This is only a problem in 64-bit mode because 64-bit mode
unconditionally pops SS:RSP  (SDM vol 3, 6.14.3 "IRET in IA-32e Mode").
In 32-bit mode SS:RSP is not popped because there is no privilege level
change when executing IRET at the end of the #GP handler.

Signed-off-by:  Aaron Lewis <aaronlewis@google.com>
Reviewed-by: Sean Christopherson <seanjc@google.com>
---
 lib/x86/desc.h     | 4 ++++
 lib/x86/usermode.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index b65539e..9b81da0 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -18,6 +18,10 @@ struct ex_regs {
     unsigned long rip;
     unsigned long cs;
     unsigned long rflags;
+#ifdef __x86_64__
+    unsigned long rsp;
+    unsigned long ss;
+#endif
 };
 
 typedef void (*handler)(struct ex_regs *regs);
diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index 2e77831..57a017d 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -26,6 +26,9 @@ static void restore_exec_to_jmpbuf_exception_handler(struct ex_regs *regs)
 	/* longjmp must happen after iret, so do not do it now.  */
 	regs->rip = (unsigned long)&restore_exec_to_jmpbuf;
 	regs->cs = KERNEL_CS;
+#ifdef __x86_64__
+	regs->ss = KERNEL_DS;
+#endif
 }
 
 uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
-- 
2.34.1.173.g76aa8bc2d0-goog

[kvm-unit-tests PATCH v2 2/4] x86: Align L2's stacks

[Aaron Lewis]

Setting the stack to PAGE_SIZE - 1 sets the stack to being 1-byte
aligned, which fails in usermode with alignment checks enabled (ie: with
flags cr0.am set and eflags.ac set).  This was causing an #AC in
usermode.c when preparing to call the callback in run_in_user().
Aligning the stack fixes the issue.

For the purposes of fixing the #AC in usermode.c the stack has to be
aligned to at least an 8-byte boundary.  Setting it to a page aligned
boundary ensures any stack alignment requirements are met as x86_64
stacks generally want to be 16-byte aligned.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 x86/vmx.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 6dc9a55..f4fbb94 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -42,7 +42,7 @@
 u64 *bsp_vmxon_region;
 struct vmcs *vmcs_root;
 u32 vpid_cnt;
-void *guest_stack, *guest_syscall_stack;
+u64 guest_stack_top, guest_syscall_stack_top;
 u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2];
 struct regs regs;
 
@@ -1241,8 +1241,7 @@ static void init_vmcs_guest(void)
 	vmcs_write(GUEST_CR3, guest_cr3);
 	vmcs_write(GUEST_CR4, guest_cr4);
 	vmcs_write(GUEST_SYSENTER_CS,  KERNEL_CS);
-	vmcs_write(GUEST_SYSENTER_ESP,
-		(u64)(guest_syscall_stack + PAGE_SIZE - 1));
+	vmcs_write(GUEST_SYSENTER_ESP, guest_syscall_stack_top);
 	vmcs_write(GUEST_SYSENTER_EIP, (u64)(&entry_sysenter));
 	vmcs_write(GUEST_DR7, 0);
 	vmcs_write(GUEST_EFER, rdmsr(MSR_EFER));
@@ -1292,7 +1291,7 @@ static void init_vmcs_guest(void)
 
 	/* 26.3.1.4 */
 	vmcs_write(GUEST_RIP, (u64)(&guest_entry));
-	vmcs_write(GUEST_RSP, (u64)(guest_stack + PAGE_SIZE - 1));
+	vmcs_write(GUEST_RSP, guest_stack_top);
 	vmcs_write(GUEST_RFLAGS, X86_EFLAGS_FIXED);
 
 	/* 26.3.1.5 */
@@ -1388,8 +1387,8 @@ void init_vmx(u64 *vmxon_region)
 static void alloc_bsp_vmx_pages(void)
 {
 	bsp_vmxon_region = alloc_page();
-	guest_stack = alloc_page();
-	guest_syscall_stack = alloc_page();
+	guest_stack_top = (uintptr_t)alloc_page() + PAGE_SIZE;
+	guest_syscall_stack_top = (uintptr_t)alloc_page() + PAGE_SIZE;
 	vmcs_root = alloc_page();
 }
 
-- 
2.34.1.173.g76aa8bc2d0-goog

[kvm-unit-tests PATCH v2 3/4] x86: Add a test framework for nested_vmx_reflect_vmexit() testing

[Aaron Lewis]

Set up a test framework that verifies an exception occurring in L2 is
forwarded to the right place (L0?, L1?, L2?).  To add a test to this
framework just add the exception and callbacks to the
vmx_exception_tests array.

This framework tests two things:
 1) It tests that an exception is handled by L2.
 2) It tests that an exception is handled by L1.
To test that this happens, each exception is triggered twice; once with
just an L2 exception handler registered, and again with both an L2
exception handler registered and L1's exception bitmap set.  The
expectation is that the first exception will be handled by L2 and the
second by L1.

To implement this support was added to vmx.c to allow more than one
L2 test be run in a single test.  Previously there was a hard limit of
only being allowed to set the L2 guest code once in a given test.  That
is no longer a limitation with the addition of
test_set_guest_restartable().

Support was also added to allow the test to complete without running
through the entirety of the L2 guest code. Calling the function
test_set_guest_finished() marks the guest code as completed, allowing
it to end without running to the end.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 lib/x86/desc.c    |  2 +-
 lib/x86/desc.h    |  1 +
 x86/unittests.cfg |  7 ++++
 x86/vmx.c         | 17 +++++++++
 x86/vmx.h         |  2 ++
 x86/vmx_tests.c   | 88 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 16b7256..c2eb16e 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -91,7 +91,7 @@ struct ex_record {
 
 extern struct ex_record exception_table_start, exception_table_end;
 
-static const char* exception_mnemonic(int vector)
+const char* exception_mnemonic(int vector)
 {
 	switch(vector) {
 	case 0: return "#DE";
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 9b81da0..ad6277b 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -224,6 +224,7 @@ void set_intr_alt_stack(int e, void *fn);
 void print_current_tss_info(void);
 handler handle_exception(u8 v, handler fn);
 void unhandled_exception(struct ex_regs *regs, bool cpu);
+const char* exception_mnemonic(int vector);
 
 bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
 			void *data);
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 9fcdcae..0353b69 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -368,6 +368,13 @@ arch = x86_64
 groups = vmx nested_exception
 check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y
 
+[vmx_exception_test]
+file = vmx.flat
+extra_params = -cpu max,+vmx -append vmx_exception_test
+arch = x86_64
+groups = vmx nested_exception
+timeout = 10
+
 [debug]
 file = debug.flat
 arch = x86_64
diff --git a/x86/vmx.c b/x86/vmx.c
index f4fbb94..9908746 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -1895,6 +1895,23 @@ void test_set_guest(test_guest_func func)
 	v2_guest_main = func;
 }
 
+/*
+ * Set the target of the first enter_guest call and reset the RIP so 'func'
+ * will start from the beginning.  This can be called multiple times per test.
+ */
+void test_set_guest_restartable(test_guest_func func)
+{
+	assert(current->v2);
+	v2_guest_main = func;
+	init_vmcs_guest();
+	guest_finished = 0;
+}
+
+void test_set_guest_finished(void)
+{
+	guest_finished = 1;
+}
+
 static void check_for_guest_termination(union exit_reason exit_reason)
 {
 	if (is_hypercall(exit_reason)) {
diff --git a/x86/vmx.h b/x86/vmx.h
index 4423986..5321a7e 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -1055,7 +1055,9 @@ void hypercall(u32 hypercall_no);
 typedef void (*test_guest_func)(void);
 typedef void (*test_teardown_func)(void *data);
 void test_set_guest(test_guest_func func);
+void test_set_guest_restartable(test_guest_func func);
 void test_add_teardown(test_teardown_func func, void *data);
 void test_skip(const char *msg);
+void test_set_guest_finished(void);
 
 #endif
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 3d57ed6..018db2f 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -10701,6 +10701,93 @@ static void vmx_pf_vpid_test(void)
 	__vmx_pf_vpid_test(invalidate_tlb_new_vpid, 1);
 }
 
+struct vmx_exception_test {
+	u8 vector;
+	void (*guest_code)(void);
+	void (*init_test)(void);
+	void (*uninit_test)(void);
+};
+
+struct vmx_exception_test vmx_exception_tests[] = {
+};
+
+static u8 vmx_exception_test_vector;
+
+static void vmx_exception_handler(struct ex_regs *regs)
+{
+	report(regs->vector == vmx_exception_test_vector,
+	       "Handling %s in L2's exception handler",
+	       exception_mnemonic(vmx_exception_test_vector));
+	vmcall();
+}
+
+static void handle_exception_in_l2(u8 vector)
+{
+	handler old_handler = handle_exception(vector, vmx_exception_handler);
+
+	vmx_exception_test_vector = vector;
+
+	enter_guest();
+	report(vmcs_read(EXI_REASON) == VMX_VMCALL,
+	       "%s handled by L2", exception_mnemonic(vector));
+
+	test_set_guest_finished();
+
+	handle_exception(vector, old_handler);
+}
+
+static void handle_exception_in_l1(u32 vector)
+{
+	handler old_handler = handle_exception(vector, vmx_exception_handler);
+	u32 old_eb = vmcs_read(EXC_BITMAP);
+
+	vmx_exception_test_vector = 0xff;
+
+	vmcs_write(EXC_BITMAP, old_eb | (1u << vector));
+
+	enter_guest();
+
+	report((vmcs_read(EXI_REASON) == VMX_EXC_NMI) &&
+	       ((vmcs_read(EXI_INTR_INFO) & 0xff) == vector),
+	       "%s handled by L1", exception_mnemonic(vector));
+
+	test_set_guest_finished();
+
+	vmcs_write(EXC_BITMAP, old_eb);
+	handle_exception(vector, old_handler);
+}
+
+static void vmx_exception_test(void)
+{
+	struct vmx_exception_test *t;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vmx_exception_tests); i++) {
+		t = &vmx_exception_tests[i];
+
+		TEST_ASSERT(t->guest_code);
+		test_set_guest_restartable(t->guest_code);
+
+		if (t->init_test)
+			t->init_test();
+
+		handle_exception_in_l2(t->vector);
+
+		if (t->uninit_test)
+			t->uninit_test();
+
+		test_set_guest_restartable(t->guest_code);
+
+		if (t->init_test)
+			t->init_test();
+
+		handle_exception_in_l1(t->vector);
+
+		if (t->uninit_test)
+			t->uninit_test();
+	}
+}
+
 #define TEST(name) { #name, .v2 = name }
 
 /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
@@ -10810,5 +10897,6 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_pf_no_vpid_test),
 	TEST(vmx_pf_invvpid_test),
 	TEST(vmx_pf_vpid_test),
+	TEST(vmx_exception_test),
 	{ NULL, NULL, NULL, NULL, NULL, {0} },
 };
-- 
2.34.1.173.g76aa8bc2d0-goog

[kvm-unit-tests PATCH v2 4/4] x86: Add test coverage for nested_vmx_reflect_vmexit() testing

[Aaron Lewis]

Add test cases to ensure exceptions that occur in L2 are forwarded to
the correct place.  Add testing for exceptions: #GP, #UD, #DE, #DB, #BP,
and #AC.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 x86/vmx_tests.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 018db2f..f795330 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -21,6 +21,7 @@
 #include "smp.h"
 #include "delay.h"
 #include "access.h"
+#include "x86/usermode.h"
 
 #define VPID_CAP_INVVPID_TYPES_SHIFT 40
 
@@ -10701,6 +10702,72 @@ static void vmx_pf_vpid_test(void)
 	__vmx_pf_vpid_test(invalidate_tlb_new_vpid, 1);
 }
 
+static void vmx_l2_gp_test(void)
+{
+	*(volatile u64 *)NONCANONICAL = 0;
+}
+
+static void vmx_l2_ud_test(void)
+{
+	asm volatile ("ud2");
+}
+
+static void vmx_l2_de_test(void)
+{
+	asm volatile (
+		"xor %%eax, %%eax\n\t"
+		"xor %%ebx, %%ebx\n\t"
+		"xor %%edx, %%edx\n\t"
+		"idiv %%ebx\n\t"
+		::: "eax", "ebx", "edx");
+}
+
+static void vmx_l2_bp_test(void)
+{
+	asm volatile ("int3");
+}
+
+static void vmx_db_init(void)
+{
+	enable_tf();
+}
+
+static void vmx_db_uninit(void)
+{
+	disable_tf();
+}
+
+static void vmx_l2_db_test(void)
+{
+}
+
+static uint64_t usermode_callback(void)
+{
+	/* Trigger an #AC by writing 8 bytes to a 4-byte aligned address. */
+	asm volatile(
+		"sub $0x10, %rsp\n\t"
+		"movq $0, 0x4(%rsp)\n\t"
+		"add $0x10, %rsp\n\t");
+
+	return 0;
+}
+
+static void vmx_l2_ac_test(void)
+{
+	u64 old_cr0  = read_cr0();
+	u64 old_rflags = read_rflags();
+	bool raised_vector = false;
+
+	write_cr0(old_cr0 | X86_CR0_AM);
+	write_rflags(old_rflags | X86_EFLAGS_AC);
+
+	run_in_user(usermode_callback, AC_VECTOR, 0, 0, 0, 0, &raised_vector);
+	report(raised_vector, "#AC vector raised from usermode in L2");
+
+	write_cr0(old_cr0);
+	write_rflags(old_rflags);
+}
+
 struct vmx_exception_test {
 	u8 vector;
 	void (*guest_code)(void);
@@ -10709,6 +10776,12 @@ struct vmx_exception_test {
 };
 
 struct vmx_exception_test vmx_exception_tests[] = {
+	{ GP_VECTOR, vmx_l2_gp_test },
+	{ UD_VECTOR, vmx_l2_ud_test },
+	{ DE_VECTOR, vmx_l2_de_test },
+	{ DB_VECTOR, vmx_l2_db_test, vmx_db_init, vmx_db_uninit },
+	{ BP_VECTOR, vmx_l2_bp_test },
+	{ AC_VECTOR, vmx_l2_ac_test },
 };
 
 static u8 vmx_exception_test_vector;
-- 
2.34.1.173.g76aa8bc2d0-goog

Re: [kvm-unit-tests PATCH v2 0/4] Add additional testing for routing L2 exceptions

[Paolo Bonzini]

On 12/14/21 02:18, Aaron Lewis wrote:
> In a previous series testing was added to verify that when a #PF occured
> in L2 the exception was routed to the correct place.  In this series
> other exceptions are tested (ie: #GP, #UD, #DE, #DB, #BP, #AC).
> 
> The first two commits in this series are bug fixes that were discovered
> while making these changes.  The last two implement the tests.
> 
> v1 -> v2:
>   - Add guest_stack_top and guest_syscall_stack_top for aligning L2's
>     stacks.
>   - Refactor test to make it more extensible (ie: Added
>     vmx_exception_tests array and framework around it).
>   - Split test into 2 commits:
>     1. Test infrustructure.
>     2. Test cases.
> 
> Aaron Lewis (4):
>    x86: Fix a #GP from occurring in usermode library's exception handlers
>    x86: Align L2's stacks
>    x86: Add a test framework for nested_vmx_reflect_vmexit() testing
>    x86: Add test coverage for nested_vmx_reflect_vmexit() testing
> 
>   lib/x86/desc.c     |   2 +-
>   lib/x86/desc.h     |   5 ++
>   lib/x86/usermode.c |   3 +
>   x86/unittests.cfg  |   7 ++
>   x86/vmx.c          |  28 ++++++--
>   x86/vmx.h          |   2 +
>   x86/vmx_tests.c    | 161 +++++++++++++++++++++++++++++++++++++++++++++
>   7 files changed, 201 insertions(+), 7 deletions(-)
> 

Queued patches 1-2 for now, thanks.

Paolo

Re: [kvm-unit-tests PATCH v2 2/4] x86: Align L2's stacks

[Sean Christopherson]

On Tue, Dec 14, 2021, Aaron Lewis wrote:
> Setting the stack to PAGE_SIZE - 1 sets the stack to being 1-byte
> aligned, which fails in usermode with alignment checks enabled (ie: with
> flags cr0.am set and eflags.ac set).  This was causing an #AC in
> usermode.c when preparing to call the callback in run_in_user().
> Aligning the stack fixes the issue.
> 
> For the purposes of fixing the #AC in usermode.c the stack has to be
> aligned to at least an 8-byte boundary.  Setting it to a page aligned
> boundary ensures any stack alignment requirements are met as x86_64
> stacks generally want to be 16-byte aligned.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [kvm-unit-tests PATCH v2 3/4] x86: Add a test framework for nested_vmx_reflect_vmexit() testing

[Sean Christopherson]

On Tue, Dec 14, 2021, Aaron Lewis wrote:
> Set up a test framework that verifies an exception occurring in L2 is
> forwarded to the right place (L0?, L1?, L2?).  To add a test to this
> framework just add the exception and callbacks to the
> vmx_exception_tests array.

The bulk of this patch belongs in patch 04.  It's nearly impossible to properly
review the guts of vmx_exception_test() without seeing how the hooks are actually
used, and it also adds a test without any testcases, which is odd.

The introduction of test_set_guest_restartable() and test_set_guest_finished()
can and should go in a separate patch.  It would be nice if a few existing tests
were converted to use test_set_guest_finished(), but certainly not necessary,
and that might cause too much scope screep.

Exposing exception_mnemonic() should also go in a separate patch.  E.g. if someone
else is working on a test that wants to use exception_mnemonic(), then the two
in-flight series can share a single patch.  That's not very likely to happen in 
KUT, but it's good practice in general.

> This framework tests two things:
>  1) It tests that an exception is handled by L2.
>  2) It tests that an exception is handled by L1.
> To test that this happens, each exception is triggered twice; once with
> just an L2 exception handler registered, and again with both an L2
> exception handler registered and L1's exception bitmap set.  The
> expectation is that the first exception will be handled by L2 and the
> second by L1.
> 
> To implement this support was added to vmx.c to allow more than one
> L2 test be run in a single test.  Previously there was a hard limit of
> only being allowed to set the L2 guest code once in a given test.  That
> is no longer a limitation with the addition of
> test_set_guest_restartable().
> 
> Support was also added to allow the test to complete without running
> through the entirety of the L2 guest code. Calling the function
> test_set_guest_finished() marks the guest code as completed, allowing
> it to end without running to the end.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---

...

> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 9fcdcae..0353b69 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -368,6 +368,13 @@ arch = x86_64
>  groups = vmx nested_exception
>  check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y
>  
> +[vmx_exception_test]
> +file = vmx.flat
> +extra_params = -cpu max,+vmx -append vmx_exception_test
> +arch = x86_64
> +groups = vmx nested_exception
> +timeout = 10

Why add a new test case instead of folding this into "vmx"?  It's quite speedy.
The "vmx" bucket definitely needs some cleanup, but I don't thinking adding a bunch
of one-off tests is the way forward.

> +
>  [debug]
>  file = debug.flat
>  arch = x86_64
> diff --git a/x86/vmx.c b/x86/vmx.c
> index f4fbb94..9908746 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -1895,6 +1895,23 @@ void test_set_guest(test_guest_func func)
>  	v2_guest_main = func;
>  }
>  
> +/*
> + * Set the target of the first enter_guest call and reset the RIP so 'func'
> + * will start from the beginning.  This can be called multiple times per test.
> + */
> +void test_set_guest_restartable(test_guest_func func)

Hmm, "restartable" is somewhat confusing as it implies that other guests aren't
restartable, and sometimes people refer to resuming a guest after a VM-Exit as
"restarting" the guest.  Maybe test_override_guest()?  


> +{
> +	assert(current->v2);
> +	v2_guest_main = func;

These two lines can be shared with the existing test_set_guest().  It's kinda silly
since it's just two lines, but it is helpful to show the relationship between the
two helpers.  E.g.

static void __test_set_guest(test_guest_func func)
{
	assert(current->v2);
	v2_guest_main = func;
}

/*
 * Set the target of the first enter_guest call. Can only be called once per
 * test. Must be called before first enter_guest call.
 */
void test_set_guest(test_guest_func func)
{
	TEST_ASSERT_MSG(!v2_guest_main, "Already set guest func.");
	__test_set_guest(func);
}

void test_override_guest(test_guest_func func)
{
	__test_set_guest(func);
	init_vmcs_guest();
}


> +	init_vmcs_guest();
> +	guest_finished = 0;

This seems unnecessary, can't the test simply not set this flag?  Ah, after
running and debugging, the issue is that vmx_l2_ac_test() doesn't do a vmcall()
and so that test runs to completion.  As annoying as I find guest_entry to be, I
do think it's better to leave this alone and add the vmcall() to vmx_l2_ac_test().

> +}
> +
> +void test_set_guest_finished(void)
> +{
> +	guest_finished = 1;
> +}
> +
>  static void check_for_guest_termination(union exit_reason exit_reason)
>  {
>  	if (is_hypercall(exit_reason)) {
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 4423986..5321a7e 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -1055,7 +1055,9 @@ void hypercall(u32 hypercall_no);
>  typedef void (*test_guest_func)(void);
>  typedef void (*test_teardown_func)(void *data);
>  void test_set_guest(test_guest_func func);
> +void test_set_guest_restartable(test_guest_func func);
>  void test_add_teardown(test_teardown_func func, void *data);
>  void test_skip(const char *msg);
> +void test_set_guest_finished(void);
>  
>  #endif
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 3d57ed6..018db2f 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -10701,6 +10701,93 @@ static void vmx_pf_vpid_test(void)
>  	__vmx_pf_vpid_test(invalidate_tlb_new_vpid, 1);
>  }
>  
> +struct vmx_exception_test {
> +	u8 vector;
> +	void (*guest_code)(void);
> +	void (*init_test)(void);
> +	void (*uninit_test)(void);

The init/uninit helpers are unnecessary, the #DB test can instead set EFLAGS.TF
from L2 and then rely on test_override_guest() to restore vmcs.GUEST_RFLAGS.  The
#AC test can do the same thing (stuff state without restoring).  It's a little
gross, but it yields much cleaner code in the test loop and we're already relying
on test_override_guest() to restore guest state (RIP).

> +};
> +
> +struct vmx_exception_test vmx_exception_tests[] = {
> +};
> +
> +static u8 vmx_exception_test_vector;
> +
> +static void vmx_exception_handler(struct ex_regs *regs)
> +{
> +	report(regs->vector == vmx_exception_test_vector,
> +	       "Handling %s in L2's exception handler",
> +	       exception_mnemonic(vmx_exception_test_vector));
> +	vmcall();
> +}
> +
> +static void handle_exception_in_l2(u8 vector)
> +{
> +	handler old_handler = handle_exception(vector, vmx_exception_handler);
> +
> +	vmx_exception_test_vector = vector;
> +
> +	enter_guest();
> +	report(vmcs_read(EXI_REASON) == VMX_VMCALL,
> +	       "%s handled by L2", exception_mnemonic(vector));
> +
> +	test_set_guest_finished();

Just call test_set_guest_finished() after all tests run.

> +	handle_exception(vector, old_handler);
> +}
> +
> +static void handle_exception_in_l1(u32 vector)
> +{
> +	handler old_handler = handle_exception(vector, vmx_exception_handler);
> +	u32 old_eb = vmcs_read(EXC_BITMAP);
> +
> +	vmx_exception_test_vector = 0xff;

No need to install the handler or set the vector, just let L2 expode on the
unexpected exception.

> +
> +	vmcs_write(EXC_BITMAP, old_eb | (1u << vector));
> +
> +	enter_guest();
> +
> +	report((vmcs_read(EXI_REASON) == VMX_EXC_NMI) &&
> +	       ((vmcs_read(EXI_INTR_INFO) & 0xff) == vector),
> +	       "%s handled by L1", exception_mnemonic(vector));
> +
> +	test_set_guest_finished();
> +
> +	vmcs_write(EXC_BITMAP, old_eb);
> +	handle_exception(vector, old_handler);
> +}
> +
> +static void vmx_exception_test(void)
> +{
> +	struct vmx_exception_test *t;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vmx_exception_tests); i++) {
> +		t = &vmx_exception_tests[i];
> +
> +		TEST_ASSERT(t->guest_code);

Eh, I wouldn't bother, especially once (un)init_test go bye bye.

This is what I ended up with (pulling in code from the next patch):

static void vmx_l2_gp_test(void)
{
	*(volatile u64 *)NONCANONICAL = 0;
}

static void vmx_l2_ud_test(void)
{
	asm volatile ("ud2");
}

static void vmx_l2_de_test(void)
{
	asm volatile (
		"xor %%eax, %%eax\n\t"
		"xor %%ebx, %%ebx\n\t"
		"xor %%edx, %%edx\n\t"
		"idiv %%ebx\n\t"
		::: "eax", "ebx", "edx");
}

static void vmx_l2_bp_test(void)
{
	asm volatile ("int3");
}

static void vmx_l2_db_test(void)
{
	write_rflags(read_rflags() | X86_EFLAGS_TF);
}

static uint64_t usermode_callback(void)
{
	/* Trigger an #AC by writing 8 bytes to a 4-byte aligned address. */
	asm volatile(
		"sub $0x10, %rsp\n\t"
		"movq $0, 0x4(%rsp)\n\t"
		"add $0x10, %rsp\n\t");

	return 0;
}

static void vmx_l2_ac_test(void)
{
	bool raised_vector = false;

	write_cr0(read_cr0() | X86_CR0_AM);
	write_rflags(read_rflags() | X86_EFLAGS_AC);

	run_in_user(usermode_callback, AC_VECTOR, 0, 0, 0, 0, &raised_vector);
	report(raised_vector, "#AC vector raised from usermode in L2");
	vmcall();
}

struct vmx_exception_test {
	u8 vector;
	void (*guest_code)(void);
};

struct vmx_exception_test vmx_exception_tests[] = {
	{ GP_VECTOR, vmx_l2_gp_test },
	{ UD_VECTOR, vmx_l2_ud_test },
	{ DE_VECTOR, vmx_l2_de_test },
	{ DB_VECTOR, vmx_l2_db_test },
	{ BP_VECTOR, vmx_l2_bp_test },
	{ AC_VECTOR, vmx_l2_ac_test },
};

static u8 vmx_exception_test_vector;

static void vmx_exception_handler(struct ex_regs *regs)
{
	report(regs->vector == vmx_exception_test_vector,
	       "Handling %s in L2's exception handler",
	       exception_mnemonic(vmx_exception_test_vector));
	vmcall();
}

static void handle_exception_in_l2(u8 vector)
{
	handler old_handler = handle_exception(vector, vmx_exception_handler);

	vmx_exception_test_vector = vector;

	enter_guest();
	report(vmcs_read(EXI_REASON) == VMX_VMCALL,
	       "%s handled by L2", exception_mnemonic(vector));

	handle_exception(vector, old_handler);
}

static void handle_exception_in_l1(u32 vector)
{
	u32 old_eb = vmcs_read(EXC_BITMAP);

	vmcs_write(EXC_BITMAP, old_eb | (1u << vector));

	enter_guest();

	report((vmcs_read(EXI_REASON) == VMX_EXC_NMI) &&
	       ((vmcs_read(EXI_INTR_INFO) & 0xff) == vector),
	       "%s handled by L1", exception_mnemonic(vector));

	vmcs_write(EXC_BITMAP, old_eb);
}

static void vmx_exception_test(void)
{
	struct vmx_exception_test *t;
	int i;

	for (i = 0; i < ARRAY_SIZE(vmx_exception_tests); i++) {
		t = &vmx_exception_tests[i];

		/*
		 * Override the guest code before each run even though it's the
		 * same code, the VMCS guest state needs to be reinitialized.
		 */
		test_override_guest(t->guest_code);
		handle_exception_in_l2(t->vector);

		test_override_guest(t->guest_code);
		handle_exception_in_l1(t->vector);
	}

	test_set_guest_finished();
}

Re: [kvm-unit-tests PATCH v2 3/4] x86: Add a test framework for nested_vmx_reflect_vmexit() testing

[Aaron Lewis]

>
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index 9fcdcae..0353b69 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -368,6 +368,13 @@ arch = x86_64
> >  groups = vmx nested_exception
> >  check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y
> >
> > +[vmx_exception_test]
> > +file = vmx.flat
> > +extra_params = -cpu max,+vmx -append vmx_exception_test
> > +arch = x86_64
> > +groups = vmx nested_exception
> > +timeout = 10
>
> Why add a new test case instead of folding this into "vmx"?  It's quite speedy.
> The "vmx" bucket definitely needs some cleanup, but I don't thinking adding a bunch
> of one-off tests is the way forward.
>

I'm not sure I follow.  The test does run in the "vmx" bucket
AFAICT... Oh, do you mean it should be added to the extra_params here
along with the other tests?

[vmx]
file = vmx.flat
extra_params = -cpu max,+vmx -append "-exit_monitor_from_l2_test
-ept_access* -vmx_smp* -vmx_vmcs_shadow_test
-atomic_switch_overflow_msrs_test -vmx_init_signal_test
-vmx_apic_passthrough_tpr_threshold_test -apic_reg_virt_test
-virt_x2apic_mode_test -vmx_pf_exception_test -vmx_pf_no_vpid_test
-vmx_pf_invvpid_test -vmx_pf_vpid_test"
arch = x86_64
groups = vmx

>
> > +
> >  [debug]
> >  file = debug.flat
> >  arch = x86_64
> > diff --git a/x86/vmx.c b/x86/vmx.c
> > index f4fbb94..9908746 100644
> > --- a/x86/vmx.c
> > +++ b/x86/vmx.c
> > @@ -1895,6 +1895,23 @@ void test_set_guest(test_guest_func func)
>

Re: [kvm-unit-tests PATCH v2 3/4] x86: Add a test framework for nested_vmx_reflect_vmexit() testing

[Sean Christopherson]

On Wed, Jan 19, 2022, Aaron Lewis wrote:
> >
> > > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > > index 9fcdcae..0353b69 100644
> > > --- a/x86/unittests.cfg
> > > +++ b/x86/unittests.cfg
> > > @@ -368,6 +368,13 @@ arch = x86_64
> > >  groups = vmx nested_exception
> > >  check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y
> > >
> > > +[vmx_exception_test]
> > > +file = vmx.flat
> > > +extra_params = -cpu max,+vmx -append vmx_exception_test
> > > +arch = x86_64
> > > +groups = vmx nested_exception
> > > +timeout = 10
> >
> > Why add a new test case instead of folding this into "vmx"?  It's quite speedy.
> > The "vmx" bucket definitely needs some cleanup, but I don't thinking adding a bunch
> > of one-off tests is the way forward.
> >
> 
> I'm not sure I follow.  The test does run in the "vmx" bucket
> AFAICT... Oh, do you mean it should be added to the extra_params here
> along with the other tests?

Yep, exactly.  "vmx" really needs to be split up and/or reworked so that it's
command line isn't so ridiculous, but that's a future problem.