From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [kvm-unit-tests PATCH] x86: vmx: test invalid vmcs Date: Thu, 22 Dec 2016 19:50:53 +0100 Message-ID: <20161222185052.GA26351@potion> References: <20161221202604.24248-1-rkrcmar@redhat.com> <60e2f70f-7842-3c0d-c1b1-ce54948d64f4@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51320 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966655AbcLVSu4 (ORCPT ); Thu, 22 Dec 2016 13:50:56 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4E3BC31B33B for ; Thu, 22 Dec 2016 18:50:56 +0000 (UTC) Content-Disposition: inline In-Reply-To: <60e2f70f-7842-3c0d-c1b1-ce54948d64f4@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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} }, >> }; >>