From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception Date: Thu, 15 Aug 2013 11:15:01 +0200 Message-ID: <520C9C15.5050908@web.de> References: <1376409368-7016-1-git-send-email-yzt356@gmail.com> <1376409368-7016-5-git-send-email-yzt356@gmail.com> <520C8C15.2070905@web.de> <520C8F42.2010101@web.de> <520C9416.8080404@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="rRGAVmGPXdq2lAjogXbbtDlsfac7igniB" Cc: kvm , Gleb Natapov , Paolo Bonzini To: Arthur Chunqi Li Return-path: Received: from mout.web.de ([212.227.15.4]:57800 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751439Ab3HOJPE (ORCPT ); Thu, 15 Aug 2013 05:15:04 -0400 Received: from mchn199C.mchp.siemens.de ([95.157.58.223]) by smtp.web.de (mrweb001) with ESMTPSA (Nemesis) id 0MYvmV-1Vfboj0vZL-00Vjxl for ; Thu, 15 Aug 2013 11:15:02 +0200 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --rRGAVmGPXdq2lAjogXbbtDlsfac7igniB Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2013-08-15 10:48, Arthur Chunqi Li wrote: > On Thu, Aug 15, 2013 at 4:40 PM, Jan Kiszka wrote: >> On 2013-08-15 10:35, Arthur Chunqi Li wrote: >>> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka wrote= : >>>> On 2013-08-15 10:16, Arthur Chunqi Li wrote: >>>>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka wro= te: >>>>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote: >>>>>>> Add test cases for instruction interception, including three type= s: >>>>>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAI= T/ >>>>>>> RDPMC/RDTSC/MONITOR/PAUSE) >>>>>>> 2. Secondary Processor-Based VM-Execution Controls (WBINVD) >>>>>>> 3. No control flag (CPUID/INVD) >>>>>>> >>>>>>> Signed-off-by: Arthur Chunqi Li >>>>>>> --- >>>>>>> x86/vmx.c | 3 +- >>>>>>> x86/vmx.h | 7 ++++ >>>>>>> x86/vmx_tests.c | 117 +++++++++++++++++++++++++++++++++++++++++= ++++++++++++++ >>>>>>> 3 files changed, 125 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/x86/vmx.c b/x86/vmx.c >>>>>>> index ca36d35..c346070 100644 >>>>>>> --- a/x86/vmx.c >>>>>>> +++ b/x86/vmx.c >>>>>>> @@ -336,8 +336,7 @@ static void init_vmx(void) >>>>>>> : MSR_IA32_VMX_ENTRY_CTLS); >>>>>>> ctrl_cpu_rev[0].val =3D rdmsr(basic.ctrl ? MSR_IA32_VMX_TRU= E_PROC >>>>>>> : MSR_IA32_VMX_PROCBASED_CTLS); >>>>>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY) >>>>>>> - ctrl_cpu_rev[1].val =3D rdmsr(MSR_IA32_VMX_PROCBASE= D_CTLS2); >>>>>>> + ctrl_cpu_rev[1].val =3D rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2)= ; >>>>>>> if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & = CPU_VPID) >>>>>>> ept_vpid.val =3D rdmsr(MSR_IA32_VMX_EPT_VPID_CAP); >>>>>>> >>>>>>> diff --git a/x86/vmx.h b/x86/vmx.h >>>>>>> index dba8b20..d81d25d 100644 >>>>>>> --- a/x86/vmx.h >>>>>>> +++ b/x86/vmx.h >>>>>>> @@ -354,6 +354,9 @@ enum Ctrl0 { >>>>>>> CPU_INTR_WINDOW =3D 1ul << 2, >>>>>>> CPU_HLT =3D 1ul << 7, >>>>>>> CPU_INVLPG =3D 1ul << 9, >>>>>>> + CPU_MWAIT =3D 1ul << 10, >>>>>>> + CPU_RDPMC =3D 1ul << 11, >>>>>>> + CPU_RDTSC =3D 1ul << 12, >>>>>>> CPU_CR3_LOAD =3D 1ul << 15, >>>>>>> CPU_CR3_STORE =3D 1ul << 16, >>>>>>> CPU_TPR_SHADOW =3D 1ul << 21, >>>>>>> @@ -361,6 +364,8 @@ enum Ctrl0 { >>>>>>> CPU_IO =3D 1ul << 24, >>>>>>> CPU_IO_BITMAP =3D 1ul << 25, >>>>>>> CPU_MSR_BITMAP =3D 1ul << 28, >>>>>>> + CPU_MONITOR =3D 1ul << 29, >>>>>>> + CPU_PAUSE =3D 1ul << 30, >>>>>>> CPU_SECONDARY =3D 1ul << 31, >>>>>>> }; >>>>>>> >>>>>>> @@ -368,6 +373,8 @@ enum Ctrl1 { >>>>>>> CPU_EPT =3D 1ul << 1, >>>>>>> CPU_VPID =3D 1ul << 5, >>>>>>> CPU_URG =3D 1ul << 7, >>>>>>> + CPU_WBINVD =3D 1ul << 6, >>>>>>> + CPU_RDRAND =3D 1ul << 11, >>>>>>> }; >>>>>>> >>>>>>> #define SAVE_GPR \ >>>>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >>>>>>> index ad28c4c..66187f4 100644 >>>>>>> --- a/x86/vmx_tests.c >>>>>>> +++ b/x86/vmx_tests.c >>>>>>> @@ -20,6 +20,13 @@ static inline void set_stage(u32 s) >>>>>>> asm volatile("mov %0, stage\n\t"::"r"(s):"memory", "cc"); >>>>>>> } >>>>>>> >>>>>>> +static inline u32 get_stage() >>>>>>> +{ >>>>>>> + u32 s; >>>>>>> + asm volatile("mov stage, %0\n\t":"=3Dr"(s)::"memory", "cc")= ; >>>>>>> + return s; >>>>>>> +} >>>>>> >>>>>> Tagging "stage" volatile will obsolete this special assembly. >>>>>> >>>>>>> + >>>>>>> void basic_init() >>>>>>> { >>>>>>> } >>>>>>> @@ -638,6 +645,114 @@ static int iobmp_exit_handler() >>>>>>> return VMX_TEST_VMEXIT; >>>>>>> } >>>>>>> >>>>>>> +asm( >>>>>>> + "insn_hlt: hlt;ret\n\t" >>>>>>> + "insn_invlpg: invlpg 0x12345678;ret\n\t" >>>>>>> + "insn_mwait: mwait;ret\n\t" >>>>>>> + "insn_rdpmc: rdpmc;ret\n\t" >>>>>>> + "insn_rdtsc: rdtsc;ret\n\t" >>>>>>> + "insn_monitor: monitor;ret\n\t" >>>>>>> + "insn_pause: pause;ret\n\t" >>>>>>> + "insn_wbinvd: wbinvd;ret\n\t" >>>>>>> + "insn_cpuid: cpuid;ret\n\t" >>>>>>> + "insn_invd: invd;ret\n\t" >>>>>>> +); >>>>>>> +extern void insn_hlt(); >>>>>>> +extern void insn_invlpg(); >>>>>>> +extern void insn_mwait(); >>>>>>> +extern void insn_rdpmc(); >>>>>>> +extern void insn_rdtsc(); >>>>>>> +extern void insn_monitor(); >>>>>>> +extern void insn_pause(); >>>>>>> +extern void insn_wbinvd(); >>>>>>> +extern void insn_cpuid(); >>>>>>> +extern void insn_invd(); >>>>>>> + >>>>>>> +u32 cur_insn; >>>>>>> + >>>>>>> +struct insn_table { >>>>>>> + const char *name; >>>>>>> + u32 flag; >>>>>>> + void (*insn_func)(); >>>>>>> + u32 type; >>>>>> >>>>>> What do the "type" values mean? >>>>> For intercepted instructions we have three type: controlled by Prim= ary >>>>> Processor-Based VM-Execution Controls, controlled by Secondary >>>>> Controls and always intercepted. The testing process is different f= or >>>>> different types. >>>> >>>> This was a rhetorical questions. ;) Could you make the values symbol= ic? >>> OK. It's better to rename it to "ctrl_field" and define some macros >>> such as CTRL_CPU0, CTRL_CPU1, CTRL_NONE to make it more readable. >>>> >>>>>> >>>>>>> + u32 reason; >>>>>>> + ulong exit_qual; >>>>>>> + u32 insn_info; >>>>>> >>>>>> For none of the instructions you test, EXI_INST_INFO will have val= id >>>>>> content on exit. So you must not check it anyway. >>>>> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported n= ow. >>>>> Since for all intercepts these three vmcs fields are enough to >>>>> determine everything, I put it here for future use. >>>> >>>> OK, but don't test its value when it's undefined - like in all cases= >>>> implemented here. >>> Only test the used field will make it more complex because we need to= >>> define which field is used in insn_table. Besides, if any of these >>> three fields is unused, it will be set to 0; >> >> Please point me to the paragraph in the SDM where this is stated. > For qualification, see 27.2.1, "For all other VM exits, this field is > cleared.". For instruction information, see 27.2.4, "For all other VM > exits, the field is undefined." >=20 > Well, "undefined" doesn't means "cleared" :(, though it is actually > cleared in KVM. Do you think we actually need a flag to indicate which > fields need to test? Yes, that's safer. Even if existing hardware or KVM clears it so far, different implementations may not follow this, and your test may even start to randomly fail. Jan --rRGAVmGPXdq2lAjogXbbtDlsfac7igniB 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/ iEYEARECAAYFAlIMnBUACgkQitSsb3rl5xTKDgCgxwX/xB6rKkYGEDGRmuhKqhfj GNIAoKkobieOwDKxrj0AmxV+4XEA5XB2 =PtDv -----END PGP SIGNATURE----- --rRGAVmGPXdq2lAjogXbbtDlsfac7igniB--