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? >> >>> + 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. >> >>> +}; >>> + >>> +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? Jan
Attachment:
signature.asc
Description: OpenPGP digital signature