On 2013-08-15 10:35, Arthur Chunqi Li wrote:
> On Thu, Aug 15, 2013 at 4:20 PM, Jan Kiszka <jan.kis...@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.kis...@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 <yzt...@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.
> 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.
Jan
signature.asc
Description: OpenPGP digital signature
