From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] kvm-unit-tests: VMX: Split VMX test suites to separate file Date: Sun, 04 Aug 2013 19:23:30 +0200 Message-ID: <51FE8E12.60201@web.de> References: <1375262546-32151-1-git-send-email-yzt356@gmail.com> <51FE8732.6080003@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="FRMik57tovKkS92infJSF1LP1nWOqQexb" Cc: kvm , Gleb Natapov , Paolo Bonzini To: Arthur Chunqi Li Return-path: Received: from mout.web.de ([212.227.15.3]:59747 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751649Ab3HDRXd (ORCPT ); Sun, 4 Aug 2013 13:23:33 -0400 Received: from mchn199C.mchp.siemens.de ([95.157.58.223]) by smtp.web.de (mrweb101) with ESMTPSA (Nemesis) id 0MBTQo-1UvbuP1iUw-00ASot for ; Sun, 04 Aug 2013 19:23:31 +0200 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --FRMik57tovKkS92infJSF1LP1nWOqQexb Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2013-08-04 19:18, Arthur Chunqi Li wrote: >>> diff --git a/x86/vmx.h b/x86/vmx.h >>> index d80e000..f82bf5a 100644 >>> --- a/x86/vmx.h >>> +++ b/x86/vmx.h >>> @@ -1,5 +1,5 @@ >>> -#ifndef __HYPERVISOR_H >>> -#define __HYPERVISOR_H >>> +#ifndef __VMX_H >>> +#define __VMX_H >>> >>> #include "libcflat.h" >>> >>> @@ -41,7 +41,7 @@ struct vmx_test { >>> int exits; >>> }; >>> >>> -static union vmx_basic { >>> +union vmx_basic { >>> u64 val; >>> struct { >>> u32 revision; >>> @@ -55,35 +55,35 @@ static union vmx_basic { >>> }; >>> } basic; >> >> extern union vmx_basic basic, and stick the definition in vmx.c. Same >> pattern for the other cases. > Here you mean put definition in vmx.c and "extern" definition in vmx.h?= Yes. >> >>> >>> -static union vmx_ctrl_pin { >>> +union vmx_ctrl_pin { >>> u64 val; >>> struct { >>> u32 set, clr; >>> }; >>> } ctrl_pin_rev; >>> >>> -static union vmx_ctrl_cpu { >>> +union vmx_ctrl_cpu { >>> u64 val; >>> struct { >>> u32 set, clr; >>> }; >>> } ctrl_cpu_rev[2]; >>> >>> -static union vmx_ctrl_exit { >>> +union vmx_ctrl_exit { >>> u64 val; >>> struct { >>> u32 set, clr; >>> }; >>> } ctrl_exit_rev; >>> >>> -static union vmx_ctrl_ent { >>> +union vmx_ctrl_ent { >>> u64 val; >>> struct { >>> u32 set, clr; >>> }; >>> } ctrl_enter_rev; >>> >>> -static union vmx_ept_vpid { >>> +union vmx_ept_vpid { >>> u64 val; >>> struct { >>> u32:16, >>> @@ -432,6 +432,20 @@ enum Ctrl1 { >>> #define HYPERCALL_MASK 0xFFF >>> #define HYPERCALL_VMEXIT 0x1 >>> >>> + >>> +u32 *vmxon_region; >>> +struct vmcs *vmcs_root; >>> +void *guest_stack, *guest_syscall_stack; >>> +int fails, tests; >>> +u64 hypercall_field; >>> +u32 vpid_cnt; >>> +u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2]; >>> +ulong fix_cr0_set, fix_cr0_clr; >>> +ulong fix_cr4_set, fix_cr4_clr; >>> +struct regs regs; >>> +struct vmx_test *current; >>> +bool launched; >>> + >> >> extern >> >> But do all these variables need to be exported to the test cases? Bett= er >> confine it to a minimum, exporting more when test cases come along tha= t >> actually need them. >> >>> static inline int vmcs_clear(struct vmcs *vmcs) >>> { >>> bool ret; >>> @@ -462,5 +476,18 @@ static inline int vmcs_save(struct vmcs **vmcs) >>> return ret; >>> } >>> >>> +static inline void report(const char *name, int result) >>> +{ >>> + ++tests; >>> + if (result) >>> + printf("PASS: %s\n", name); >>> + else { >>> + printf("FAIL: %s\n", name); >>> + ++fails; >>> + } >>> +} >>> + >> >> Why static inline? Better leave this as service of the vmx core. > You mean put it in vmx.c and "extern" it here? Yes. >> >>> +void print_vmexit_info(); >>> + >>> #endif >>> >>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >>> new file mode 100644 >>> index 0000000..6bd36b0 >>> --- /dev/null >>> +++ b/x86/vmx_tests.c >>> @@ -0,0 +1,43 @@ >>> +#include "vmx.h" >>> + >>> +void vmenter_main() >>> +{ >>> + u64 rax; >>> + u64 rsp, resume_rsp; >>> + >>> + report("test vmlaunch", 1); >>> + >>> + asm volatile( >>> + "mov %%rsp, %0\n\t" >>> + "mov %3, %%rax\n\t" >>> + "vmcall\n\t" >>> + "mov %%rax, %1\n\t" >>> + "mov %%rsp, %2\n\t" >>> + : "=3Dr"(rsp), "=3Dr"(rax), "=3Dr"(resume_rsp) >>> + : "g"(0xABCD)); >>> + report("test vmresume", (rax =3D=3D 0xFFFF) && (rsp =3D=3D resu= me_rsp)); >>> +} >>> + >>> +int vmenter_exit_handler() >>> +{ >>> + u64 guest_rip; >>> + ulong reason; >>> + >>> + guest_rip =3D vmcs_read(GUEST_RIP); >>> + reason =3D vmcs_read(EXI_REASON) & 0xff; >>> + switch (reason) { >>> + case VMX_VMCALL: >>> + if (current->guest_regs.rax !=3D 0xABCD) { >>> + report("test vmresume", 0); >>> + return VMX_TEST_VMEXIT; >>> + } >>> + current->guest_regs.rax =3D 0xFFFF; >>> + vmcs_write(GUEST_RIP, guest_rip + 3); >>> + return VMX_TEST_RESUME; >>> + default: >>> + report("test vmresume", 0); >>> + print_vmexit_info(); >>> + } >>> + return VMX_TEST_VMEXIT; >>> +} >>> + >>> diff --git a/x86/vmx_tests.h b/x86/vmx_tests.h >>> new file mode 100644 >>> index 0000000..41580ff >>> --- /dev/null >>> +++ b/x86/vmx_tests.h >>> @@ -0,0 +1,7 @@ >>> +#ifndef __VMX_TESTS_H >>> +#define __VMX_TESTS_H >>> + >>> +void vmenter_main(); >>> +int vmenter_exit_handler(); >> >> I'd rather move vmx_tests to, well, vmx_tests.c > I have tried to put vmx_tests in vmx_tests.c and extern definition in > vmx.h. But it is impossible to get its size statically when compiling, > thus the codes "for (i =3D 1; i < ARRAY_SIZE(vmx_tests); ++i) {" in mai= n > of vmx.c is invalid. If we use ARRAY_SIZE to get its size, we should > make it static. >=20 > An alternative solution is put vmx_tests in vmx_tests.h, which will be > included by vmx.c Another alternative is to iterate over vmx_tests until you hit a NULL entry (one mandatory element of an entry is NULL), i.e. exploring its size at runtime. Jan --FRMik57tovKkS92infJSF1LP1nWOqQexb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlH+jhIACgkQitSsb3rl5xR1egCgkiSB6A7rX1UxA+1HOdLTaW4p RzcAn3XqU5hP7xyNjE2N0dV8rf/gq75K =DZZC -----END PGP SIGNATURE----- --FRMik57tovKkS92infJSF1LP1nWOqQexb--