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 10:40:54 +0200 Message-ID: <520C9416.8080404@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> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GrCnPfE1XFmXSgL8N0q30lvh8CR9qm1j9" Cc: kvm , Gleb Natapov , Paolo Bonzini To: Arthur Chunqi Li Return-path: Received: from mout.web.de ([212.227.17.12]:55592 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751437Ab3HOIk5 (ORCPT ); Thu, 15 Aug 2013 04:40:57 -0400 Received: from mchn199C.mchp.siemens.de ([95.157.58.223]) by smtp.web.de (mrweb102) with ESMTPSA (Nemesis) id 0MQvgI-1VXe8W2eac-00UJxY for ; Thu, 15 Aug 2013 10:40:55 +0200 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --GrCnPfE1XFmXSgL8N0q30lvh8CR9qm1j9 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 wrote= : >>>> On 2013-08-13 17:56, Arthur Chunqi Li wrote: >>>>> Add test cases for instruction interception, including three types:= >>>>> 1. Primary Processor-Based VM-Execution Controls (HLT/INVLPG/MWAIT/= >>>>> 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_TRUE_= PROC >>>>> : MSR_IA32_VMX_PROCBASED_CTLS); >>>>> - if (ctrl_cpu_rev[0].set & CPU_SECONDARY) >>>>> - ctrl_cpu_rev[1].val =3D rdmsr(MSR_IA32_VMX_PROCBASED_= 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 & CP= U_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 Primar= y >>> Processor-Based VM-Execution Controls, controlled by Secondary >>> Controls and always intercepted. The testing process is different for= >>> different types. >> >> This was a rhetorical questions. ;) Could you make the values symbolic= ? > 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 valid= >>>> content on exit. So you must not check it anyway. >>> Actually , "RDRAND" uses EXI_INST_INFO though it is not supported now= =2E >>> 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. > and I think writing like > this is OK since we just write a test case. >=20 > Arthur >> >>>> >>>>> +}; >>>>> + >>>>> +static struct insn_table insn_table[] =3D { >>>>> + // Flags for Primary Processor-Based VM-Execution Controls >>>>> + {"HLT", CPU_HLT, insn_hlt, 0, 12, 0, 0}, >>>>> + {"INVLPG", CPU_INVLPG, insn_invlpg, 0, 14, 0x12345678, 0}, >>>>> + {"MWAIT", CPU_MWAIT, insn_mwait, 0, 36, 0, 0}, >>>>> + {"RDPMC", CPU_RDPMC, insn_rdpmc, 0, 15, 0, 0}, >>>>> + {"RDTSC", CPU_RDTSC, insn_rdtsc, 0, 16, 0, 0}, >>>>> + {"MONITOR", CPU_MONITOR, insn_monitor, 0, 39, 0, 0}, >>>>> + {"PAUSE", CPU_PAUSE, insn_pause, 0, 40, 0, 0}, >>>>> + // Flags for Secondary Processor-Based VM-Execution Controls >>>>> + {"WBINVD", CPU_WBINVD, insn_wbinvd, 1, 54, 0, 0}, >>>>> + // Flags for Non-Processor-Based >>>>> + {"CPUID", 0, insn_cpuid, 2, 10, 0, 0}, >>>>> + {"INVD", 0, insn_invd, 2, 13, 0, 0}, >>>>> + {NULL}, >>>>> +}; >>>>> + >>>>> +static void insn_intercept_init() >>>>> +{ >>>>> + u32 ctrl_cpu[2]; >>>>> + >>>>> + ctrl_cpu[0] =3D vmcs_read(CPU_EXEC_CTRL0); >>>>> + ctrl_cpu[0] |=3D CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC= | CPU_RDTSC | >>>>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY; >>>>> + ctrl_cpu[0] &=3D ctrl_cpu_rev[0].clr; >>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]); >>>>> + ctrl_cpu[1] =3D vmcs_read(CPU_EXEC_CTRL1); >>>>> + ctrl_cpu[1] |=3D CPU_WBINVD | CPU_RDRAND; >>>>> + ctrl_cpu[1] &=3D ctrl_cpu_rev[1].clr; >>>>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]); >>>>> +} >>>>> + >>>>> +static void insn_intercept_main() >>>>> +{ >>>>> + cur_insn =3D 0; >>>>> + while(insn_table[cur_insn].name !=3D NULL) { >>>>> + set_stage(cur_insn); >>>>> + if ((insn_table[cur_insn].type =3D=3D 0 && !(ctrl_cpu= _rev[0].clr & insn_table[cur_insn].flag)) || >>>>> + (insn_table[cur_insn].type =3D=3D 1 && !(ctrl= _cpu_rev[1].clr & insn_table[cur_insn].flag))) { >>>>> + printf("\tCPU_CTRL.CPU_%s is not supported.\n= ", insn_table[cur_insn].name); >>>> >>>> >>>> Coding style, specifically overlong lines. >>>> >>>>> + } else { >>>>> + insn_table[cur_insn].insn_func(); >>>>> + if (stage !=3D cur_insn + 1) >>>>> + report(insn_table[cur_insn].name, 0);= >>>> >>>> Would be good to test the inverse as well, i.e. the intercept-free >>>> execution. >>> Since this is called "insn_intercept", I think we'd better test >>> intercept-free execution insns in a separate test suite. >> >> You have all the infrastructure here now. Isn't it trivial to check >> weather stage makes no progress when the intercept bit is cleared inst= ead? > No, thus maybe we need to rename it to "instruction test" :) This is an instruction intercept control test. Jan --GrCnPfE1XFmXSgL8N0q30lvh8CR9qm1j9 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/ iEYEARECAAYFAlIMlBYACgkQitSsb3rl5xQkhgCeJUwYSKX8eA5GddFMDyIk0Qx8 NQsAn3s1BF0OZULvCwWJN0C0h4U7JLEh =39qv -----END PGP SIGNATURE----- --GrCnPfE1XFmXSgL8N0q30lvh8CR9qm1j9--