On Thu, Dec 22, 2016 at 07:50:53PM +0100, Radim Krčmář wrote:
> 2016-12-22 11:00+0100, Paolo Bonzini:
> > On 21/12/2016 21:26, Radim Krčmář wrote:
> >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> >> index 5fd95706c418..a1400b937254 100644
> >> --- a/x86/vmx_tests.c
> >> +++ b/x86/vmx_tests.c
> >> @@ -1816,6 +1816,42 @@ int into_exit_handler()
> >> return VMX_TEST_VMEXIT;
> >> }
> >>
> >> +#define __create_invalid_vmcs_test(name, value, test) \
> >
> > No need for __ here.
>
> Ok.
>
> >> + static int invalid_##name##_init() \
> >> + { \
> >> + vmcs_write(name, value); \
> >> + return VMX_TEST_START; \
> >> + } \
> >> + static int invalid_##name##_entry_failure(struct vmentry_failure *failure) \
> >> + { \
> >> + report("VM entry failure on invalid "#name, test); \
> >> + return VMX_TEST_VMEXIT; \
> >> + }
> >> +
> >> +#define create_invalid_host_vmcs_test(name, value, error) \
> >> + __create_invalid_vmcs_test(name, value, failure->early && \
> >> + vmcs_read(VMX_INST_ERROR) == error)
> >> +
> >> +#define create_invalid_guest_vmcs_test(name, value, reason, qualification) \
> >> + __create_invalid_vmcs_test(name, value, !failure->early && \
> >> + vmcs_read(EXI_REASON) == (reason | VMX_ENTRY_FAILURE) && \
> >> + vmcs_read(EXI_QUALIFICATION) == qualification)
> >> +
> >> +#define test_invalid_vmcs(name) \
> >> + { "invalid "#name, invalid_##name##_init, NULL, NULL, NULL, {0}, \
> >> + invalid_##name##_entry_failure }
> >> +
> >> +#define C create_invalid_host_vmcs_test
> >> +C(HOST_EFER, -1ull, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
> >
> > No need for the "C" macro here.
> >
> >> +#undef C
> >> +
> >> +#define C create_invalid_guest_vmcs_test
> >> +C(VMCS_LINK_PTR, 1, VMX_FAIL_STATE, ENTRY_FAIL_VMCS_LINK_PTR)
> >> +C(GUEST_CR0, -1ull, VMX_FAIL_STATE, ENTRY_FAIL_DEFAULT)
> >> +C(GUEST_CR4, -1ull, VMX_FAIL_STATE, ENTRY_FAIL_DEFAULT)
> >> +C(GUEST_EFER, -1ull, VMX_FAIL_STATE, ENTRY_FAIL_DEFAULT)
> >
> > Same here.
>
> I can change it into something like this:
>
> create_invalid_guest_vmcs_test(VMCS_LINK_PTR, 1, VMX_FAIL_STATE, ENTRY_FAIL_VMCS_LINK_PTR)
> create_invalid_guest_vmcs_test(GUEST_CR0, -1ull, VMX_FAIL_STATE, ENTRY_FAIL_DEFAULT)
> create_invalid_guest_vmcs_test(GUEST_CR4, -1ull, VMX_FAIL_STATE, ENTRY_FAIL_DEFAULT)
> create_invalid_guest_vmcs_test(GUEST_EFER, -1ull, VMX_FAIL_STATE, ENTRY_FAIL_DEFAULT)
>
> but not this:
>
> create_invalid_guest_vmcs_test(VMCS_LINK_PTR, 1, VMX_FAIL_STATE,
> ENTRY_FAIL_VMCS_LINK_PTR)
> create_invalid_guest_vmcs_test(GUEST_CR0, -1ull, VMX_FAIL_STATE,
> ENTRY_FAIL_DEFAULT)
> create_invalid_guest_vmcs_test(GUEST_CR4, -1ull, VMX_FAIL_STATE,
> ENTRY_FAIL_DEFAULT)
> create_invalid_guest_vmcs_test(GUEST_EFER, -1ull, VMX_FAIL_STATE,
> ENTRY_FAIL_DEFAULT)
>
> I assume we don't want to anger teletype users, so the macro shortcut is
> to improve readability.
Let's anger them :-)
I'm tired of making my patches 80ch-ugly, and have silently violated
it a few times already...
>
> > These tests look a bit different from the others that use the "stage"
> > mechanism. Perhaps you can have two tests only in vmx_tests, one for
> > host failures and one for guest failures?
>
> I avoided stages because their explicit stateful nature was not needed.
> The output is different, though ... I'll post a version that looks
> better.
>
> >> +#undef C
> >> +
> >> /* 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} },
> >> @@ -1845,5 +1881,10 @@ struct vmx_test vmx_tests[] = {
> >> disable_rdtscp_exit_handler, NULL, {0} },
> >> { "int3", int3_init, int3_guest_main, int3_exit_handler, NULL, {0} },
> >> { "into", into_init, into_guest_main, into_exit_handler, NULL, {0} },
> >> + test_invalid_vmcs(VMCS_LINK_PTR),
> >> + test_invalid_vmcs(GUEST_CR0),
> >> + test_invalid_vmcs(GUEST_CR4),
> >> + test_invalid_vmcs(GUEST_EFER),
> >> + test_invalid_vmcs(HOST_EFER),
> >> { NULL, NULL, NULL, NULL, NULL, {0} },
> >> };
> >>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
