Re: [PATCH 4/4] kvm-unit-tests: VMX: Add test cases for instruction interception

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 6889 bytes --]

On 2013-08-15 10:48, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 4:40 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2013-08-15 10:35, Arthur Chunqi Li wrote:
>>> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2013-08-15 10:16, Arthur Chunqi Li wrote:
>>>>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@web.de> 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 <yzt356@gmail.com>
>>>>>>> ---
>>>>>>>  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 = 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 = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>>>> +     ctrl_cpu_rev[1].val = rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2);
>>>>>>>       if (ctrl_cpu_rev[1].set & CPU_EPT || ctrl_cpu_rev[1].set & CPU_VPID)
>>>>>>>               ept_vpid.val = 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         = 1ul << 2,
>>>>>>>       CPU_HLT                 = 1ul << 7,
>>>>>>>       CPU_INVLPG              = 1ul << 9,
>>>>>>> +     CPU_MWAIT               = 1ul << 10,
>>>>>>> +     CPU_RDPMC               = 1ul << 11,
>>>>>>> +     CPU_RDTSC               = 1ul << 12,
>>>>>>>       CPU_CR3_LOAD            = 1ul << 15,
>>>>>>>       CPU_CR3_STORE           = 1ul << 16,
>>>>>>>       CPU_TPR_SHADOW          = 1ul << 21,
>>>>>>> @@ -361,6 +364,8 @@ enum Ctrl0 {
>>>>>>>       CPU_IO                  = 1ul << 24,
>>>>>>>       CPU_IO_BITMAP           = 1ul << 25,
>>>>>>>       CPU_MSR_BITMAP          = 1ul << 28,
>>>>>>> +     CPU_MONITOR             = 1ul << 29,
>>>>>>> +     CPU_PAUSE               = 1ul << 30,
>>>>>>>       CPU_SECONDARY           = 1ul << 31,
>>>>>>>  };
>>>>>>>
>>>>>>> @@ -368,6 +373,8 @@ enum Ctrl1 {
>>>>>>>       CPU_EPT                 = 1ul << 1,
>>>>>>>       CPU_VPID                = 1ul << 5,
>>>>>>>       CPU_URG                 = 1ul << 7,
>>>>>>> +     CPU_WBINVD              = 1ul << 6,
>>>>>>> +     CPU_RDRAND              = 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":"=r"(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 Primary
>>>>> 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.
>>>>> 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."
> 
> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]