| Message ID | 20211214011823.3277011-4-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add additional testing for routing L2 exceptions | expand |
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(); }
> > > 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) >
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.
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} }, };
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(-)