On Thu, Aug 15, 2013 at 4:40 PM, Jan Kiszka <jan.kiszka@xxxxxx> wrote:
> On 2013-08-15 10:35, Arthur Chunqi Li wrote:
>> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kiszka@xxxxxx> wrote:
>>> On 2013-08-15 10:16, Arthur Chunqi Li wrote:
>>>> On Thu, Aug 15, 2013 at 4:06 PM, Jan Kiszka <jan.kiszka@xxxxxx> 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@xxxxxxxxx>
>>>>>> ---
>>>>>> 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?
>
>> and I think writing like
>> this is OK since we just write a test case.
>>
>> Arthur
>>>
>>>>>
>>>>>> +};
>>>>>> +
>>>>>> +static struct insn_table insn_table[] = {
>>>>>> + // 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] = vmcs_read(CPU_EXEC_CTRL0);
>>>>>> + ctrl_cpu[0] |= CPU_HLT | CPU_INVLPG | CPU_MWAIT | CPU_RDPMC | CPU_RDTSC |
>>>>>> + CPU_MONITOR | CPU_PAUSE | CPU_SECONDARY;
>>>>>> + ctrl_cpu[0] &= ctrl_cpu_rev[0].clr;
>>>>>> + vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu[0]);
>>>>>> + ctrl_cpu[1] = vmcs_read(CPU_EXEC_CTRL1);
>>>>>> + ctrl_cpu[1] |= CPU_WBINVD | CPU_RDRAND;
>>>>>> + ctrl_cpu[1] &= ctrl_cpu_rev[1].clr;
>>>>>> + vmcs_write(CPU_EXEC_CTRL1, ctrl_cpu[1]);
>>>>>> +}
>>>>>> +
>>>>>> +static void insn_intercept_main()
>>>>>> +{
>>>>>> + cur_insn = 0;
>>>>>> + while(insn_table[cur_insn].name != NULL) {
>>>>>> + set_stage(cur_insn);
>>>>>> + if ((insn_table[cur_insn].type == 0 && !(ctrl_cpu_rev[0].clr & insn_table[cur_insn].flag)) ||
>>>>>> + (insn_table[cur_insn].type == 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 != 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 instead?
>> No, thus maybe we need to rename it to "instruction test" :)
>
> This is an instruction intercept control test.
Well, OK.
Arthur
>
> Jan
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
