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

[Sean Christopherson <seanjc@google.com>]

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();
}