Re: [kvm-unit-tests PATCH] x86: vmx: test invalid vmcs

["Radim Krčmář" <rkrcmar@redhat.com>]

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.

> 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} },
>>  };
>>