From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 2/4] kvm-unit-tests: VMX: Add test cases for CR0/4 shadowing Date: Thu, 15 Aug 2013 09:47:39 +0200 Message-ID: <520C879B.30908@web.de> References: <1376409368-7016-1-git-send-email-yzt356@gmail.com> <1376409368-7016-3-git-send-email-yzt356@gmail.com> <520C837E.7030407@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="22jM6g1lV8OLGLLf4o1lMD0npNsE33rIl" Cc: kvm , Gleb Natapov , Paolo Bonzini To: Arthur Chunqi Li Return-path: Received: from mout.web.de ([212.227.17.12]:65165 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760337Ab3HOHrm (ORCPT ); Thu, 15 Aug 2013 03:47:42 -0400 Received: from mchn199C.mchp.siemens.de ([95.157.58.223]) by smtp.web.de (mrweb103) with ESMTPSA (Nemesis) id 0MbhNZ-1VQiD316Pd-00J09p for ; Thu, 15 Aug 2013 09:47:40 +0200 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --22jM6g1lV8OLGLLf4o1lMD0npNsE33rIl Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2013-08-15 09:40, Arthur Chunqi Li wrote: > On Thu, Aug 15, 2013 at 3:30 PM, Jan Kiszka wrote: >> On 2013-08-13 17:56, Arthur Chunqi Li wrote: >>> Add testing for CR0/4 shadowing. >> >> A few sentences on the test strategy would be good. >> >>> >>> Signed-off-by: Arthur Chunqi Li >>> --- >>> lib/x86/vm.h | 4 + >>> x86/vmx_tests.c | 218 +++++++++++++++++++++++++++++++++++++++++++++= ++++++++++ >>> 2 files changed, 222 insertions(+) >>> >>> diff --git a/lib/x86/vm.h b/lib/x86/vm.h >>> index eff6f72..6e0ce2b 100644 >>> --- a/lib/x86/vm.h >>> +++ b/lib/x86/vm.h >>> @@ -17,9 +17,13 @@ >>> #define PTE_ADDR (0xffffffffff000ull) >>> >>> #define X86_CR0_PE 0x00000001 >>> +#define X86_CR0_MP 0x00000002 >>> +#define X86_CR0_TS 0x00000008 >>> #define X86_CR0_WP 0x00010000 >>> #define X86_CR0_PG 0x80000000 >>> #define X86_CR4_VMXE 0x00000001 >>> +#define X86_CR4_TSD 0x00000004 >>> +#define X86_CR4_DE 0x00000008 >>> #define X86_CR4_PSE 0x00000010 >>> #define X86_CR4_PAE 0x00000020 >>> #define X86_CR4_PCIDE 0x00020000 >>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >>> index 61b0cef..44be3f4 100644 >>> --- a/x86/vmx_tests.c >>> +++ b/x86/vmx_tests.c >>> @@ -5,12 +5,18 @@ >>> >>> u64 ia32_pat; >>> u64 ia32_efer; >>> +u32 stage; >>> >>> static inline void vmcall() >>> { >>> asm volatile("vmcall"); >>> } >>> >>> +static inline void set_stage(u32 s) >>> +{ >>> + asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc"); >>> +} >>> + >> >> Why do we need "state =3D s" as assembler instruction? > This is due to assembler optimization. If we simply use "state =3D s", > assembler will sometimes optimize it and state may not be set indeed. volatile u32 stage? And we have barrier() to avoid reordering. >> >>> void basic_init() >>> { >>> } >>> @@ -257,6 +263,216 @@ static int test_ctrl_efer_exit_handler() >>> return VMX_TEST_VMEXIT; >>> } >>> >>> +u32 guest_cr0, guest_cr4; >>> + >>> +static void cr_shadowing_main() >>> +{ >>> + u32 cr0, cr4, tmp; >>> + >>> + // Test read through >>> + set_stage(0); >>> + guest_cr0 =3D read_cr0(); >>> + if (stage =3D=3D 1) >>> + report("Read through CR0", 0); >>> + else >>> + vmcall(); >>> + set_stage(1); >>> + guest_cr4 =3D read_cr4(); >>> + if (stage =3D=3D 2) >>> + report("Read through CR4", 0); >>> + else >>> + vmcall(); >>> + // Test write through >>> + guest_cr0 =3D guest_cr0 ^ (X86_CR0_TS | X86_CR0_MP); >>> + guest_cr4 =3D guest_cr4 ^ (X86_CR4_TSD | X86_CR4_DE); >>> + set_stage(2); >>> + write_cr0(guest_cr0); >>> + if (stage =3D=3D 3) >>> + report("Write throuth CR0", 0); >>> + else >>> + vmcall(); >>> + set_stage(3); >>> + write_cr4(guest_cr4); >>> + if (stage =3D=3D 4) >>> + report("Write through CR4", 0); >>> + else >>> + vmcall(); >>> + // Test read shadow >>> + set_stage(4); >>> + vmcall(); >>> + cr0 =3D read_cr0(); >>> + if (stage !=3D 5) { >>> + if (cr0 =3D=3D guest_cr0) >>> + report("Read shadowing CR0", 1); >>> + else >>> + report("Read shadowing CR0", 0); >>> + } >>> + set_stage(5); >>> + cr4 =3D read_cr4(); >>> + if (stage !=3D 6) { >>> + if (cr4 =3D=3D guest_cr4) >>> + report("Read shadowing CR4", 1); >>> + else >>> + report("Read shadowing CR4", 0); >>> + } >>> + // Test write shadow (same value with shadow) >>> + set_stage(6); >>> + write_cr0(guest_cr0); >>> + if (stage =3D=3D 7) >>> + report("Write shadowing CR0 (same value with shadow)", = 0); >>> + else >>> + vmcall(); >>> + set_stage(7); >>> + write_cr4(guest_cr4); >>> + if (stage =3D=3D 8) >>> + report("Write shadowing CR4 (same value with shadow)", = 0); >>> + else >>> + vmcall(); >>> + // Test write shadow (different value) >>> + set_stage(8); >>> + tmp =3D guest_cr0 ^ X86_CR0_TS; >>> + asm volatile("mov %0, %%rsi\n\t" >>> + "mov %%rsi, %%cr0\n\t" >>> + ::"m"(tmp) >>> + :"rsi", "memory", "cc"); >>> + if (stage !=3D 9) >>> + report("Write shadowing different X86_CR0_TS", 0); >>> + else >>> + report("Write shadowing different X86_CR0_TS", 1); >>> + set_stage(9); >>> + tmp =3D guest_cr0 ^ X86_CR0_MP; >>> + asm volatile("mov %0, %%rsi\n\t" >>> + "mov %%rsi, %%cr0\n\t" >>> + ::"m"(tmp) >>> + :"rsi", "memory", "cc"); >>> + if (stage !=3D 10) >>> + report("Write shadowing different X86_CR0_MP", 0); >>> + else >>> + report("Write shadowing different X86_CR0_MP", 1); >>> + set_stage(10); >>> + tmp =3D guest_cr4 ^ X86_CR4_TSD; >>> + asm volatile("mov %0, %%rsi\n\t" >>> + "mov %%rsi, %%cr4\n\t" >>> + ::"m"(tmp) >>> + :"rsi", "memory", "cc"); >>> + if (stage !=3D 11) >>> + report("Write shadowing different X86_CR4_TSD", 0); >>> + else >>> + report("Write shadowing different X86_CR4_TSD", 1); >>> + set_stage(11); >>> + tmp =3D guest_cr4 ^ X86_CR4_DE; >>> + asm volatile("mov %0, %%rsi\n\t" >>> + "mov %%rsi, %%cr4\n\t" >>> + ::"m"(tmp) >>> + :"rsi", "memory", "cc"); >>> + if (stage !=3D 12) >>> + report("Write shadowing different X86_CR4_DE", 0); >>> + else >>> + report("Write shadowing different X86_CR4_DE", 1); >>> +} >>> + >>> +static int cr_shadowing_exit_handler() >>> +{ >>> + u64 guest_rip; >>> + ulong reason; >>> + u32 insn_len; >>> + u32 exit_qual; >>> + >>> + guest_rip =3D vmcs_read(GUEST_RIP); >>> + reason =3D vmcs_read(EXI_REASON) & 0xff; >>> + insn_len =3D vmcs_read(EXI_INST_LEN); >>> + exit_qual =3D vmcs_read(EXI_QUALIFICATION); >>> + switch (reason) { >>> + case VMX_VMCALL: >>> + switch (stage) { >>> + case 0: >>> + if (guest_cr0 =3D=3D vmcs_read(GUEST_CR0)) >>> + report("Read through CR0", 1); >>> + else >>> + report("Read through CR0", 0); >>> + break; >>> + case 1: >>> + if (guest_cr4 =3D=3D vmcs_read(GUEST_CR4)) >>> + report("Read through CR4", 1); >>> + else >>> + report("Read through CR4", 0); >>> + break; >>> + case 2: >>> + if (guest_cr0 =3D=3D vmcs_read(GUEST_CR0)) >>> + report("Write through CR0", 1); >>> + else >>> + report("Write through CR0", 0); >>> + break; >>> + case 3: >>> + if (guest_cr4 =3D=3D vmcs_read(GUEST_CR4)) >>> + report("Write through CR4", 1); >>> + else >>> + report("Write through CR4", 0); >>> + break; >>> + case 4: >>> + guest_cr0 =3D vmcs_read(GUEST_CR0) ^ (X86_CR0_T= S | X86_CR0_MP); >>> + guest_cr4 =3D vmcs_read(GUEST_CR4) ^ (X86_CR4_T= SD | X86_CR4_DE); >>> + vmcs_write(CR0_MASK, X86_CR0_TS | X86_CR0_MP); >>> + vmcs_write(CR0_READ_SHADOW, guest_cr0 & (X86_CR= 0_TS | X86_CR0_MP)); >>> + vmcs_write(CR4_MASK, X86_CR4_TSD | X86_CR4_DE);= >>> + vmcs_write(CR4_READ_SHADOW, guest_cr4 & (X86_CR= 4_TSD | X86_CR4_DE)); >>> + break; >>> + case 6: >>> + if (guest_cr0 =3D=3D (vmcs_read(GUEST_CR0) ^ (X= 86_CR0_TS | X86_CR0_MP))) >>> + report("Write shadowing CR0 (same value= )", 1); >>> + else >>> + report("Write shadowing CR0 (same value= )", 0); >>> + break; >>> + case 7: >>> + if (guest_cr4 =3D=3D (vmcs_read(GUEST_CR4) ^ (X= 86_CR4_TSD | X86_CR4_DE))) >>> + report("Write shadowing CR4 (same value= )", 1); >>> + else >>> + report("Write shadowing CR4 (same value= )", 0); >>> + break; >>> + default: >> >> report("something went wrong", 0); ??? > Actually, we cannot reach here for any reason. If necessary, we can > put an error message here. It's always better to document this, specifically via code, instead of letting the reader wonder. >> >>> + break; >>> + } >>> + vmcs_write(GUEST_RIP, guest_rip + insn_len); >>> + return VMX_TEST_RESUME; >>> + case VMX_CR: >>> + switch (stage) { >>> + case 4: >>> + report("Read shadowing CR0", 0); >>> + set_stage(stage + 1); >>> + break; >>> + case 5: >>> + report("Read shadowing CR4", 0); >>> + set_stage(stage + 1); >>> + break; >>> + case 6: >>> + report("Write shadowing CR0 (same value)", 0); >>> + set_stage(stage + 1); >>> + break; >>> + case 7: >>> + report("Write shadowing CR4 (same value)", 0); >>> + set_stage(stage + 1); >>> + break; >>> + case 8: >>> + case 9: >>> + if (exit_qual =3D=3D 0x600) >>> + set_stage(stage + 1); >> >> What is this qualification about? Yes, ESI to CR0, but that takes a >> manual to find out. And what if exit_qual is something else? Is that a= >> test error? > For stage 9, it is a "MOV to CR0/4" vmexit, qualification indicates > the detail of this exit. Take a look at the codes above of stage 9, I > think it means a test error if exit_qual is something else. Again, better make it clear in the code what the expected exit_qual encodes and that any other value is an error. Jan --22jM6g1lV8OLGLLf4o1lMD0npNsE33rIl 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/ iEYEARECAAYFAlIMh5sACgkQitSsb3rl5xTfFACZARwIXAdeionB+O4qMcfKpAW1 LhIAoOuIIb1Gltg8ZrLqoEmjNWyqX9t0 =uona -----END PGP SIGNATURE----- --22jM6g1lV8OLGLLf4o1lMD0npNsE33rIl--