Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
Hi there, This is a version calling for comments. Some minor changes should be done before final commitment (TODOs in it), because these places are related to the bugs I have commited in the previous weeks and the relevant patches are not accpeted. But these bugs are all about some unexpected occasions and the hypervisor can run if we simply ignore them. After all bugs fixed, everything will be OK. Arthur On Tue, Jul 16, 2013 at 5:27 PM, Arthur Chunqi Li yzt...@gmail.com wrote: This is the first version for VMX nested environment test case. It contains the basic VMX instructions test cases, including VMXON/ VMXOFF/VMXPTRLD/VMXPTRST/VMCLEAR/VMLAUNCH/VMRESUME/VMCALL. This patch also tests the basic execution routine in VMX nested environment and let the VM print Hello World to inform its successfully run. New files added: x86/vmx.h : contains all VMX related macro declerations x86/vmx.c : main file for VMX nested test case Signed-off-by: Arthur Chunqi Li yzt...@gmail.com --- config-x86-common.mak |2 + config-x86_64.mak |1 + lib/x86/msr.h |5 + x86/cstart64.S|4 + x86/unittests.cfg |6 + x86/vmx.c | 568 + x86/vmx.h | 406 +++ 7 files changed, 992 insertions(+) create mode 100644 x86/vmx.c create mode 100644 x86/vmx.h diff --git a/config-x86-common.mak b/config-x86-common.mak index 455032b..34a41e1 100644 --- a/config-x86-common.mak +++ b/config-x86-common.mak @@ -101,6 +101,8 @@ $(TEST_DIR)/asyncpf.elf: $(cstart.o) $(TEST_DIR)/asyncpf.o $(TEST_DIR)/pcid.elf: $(cstart.o) $(TEST_DIR)/pcid.o +$(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o + arch_clean: $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \ $(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o diff --git a/config-x86_64.mak b/config-x86_64.mak index 4e525f5..bb8ee89 100644 --- a/config-x86_64.mak +++ b/config-x86_64.mak @@ -9,5 +9,6 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \ $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \ $(TEST_DIR)/pcid.flat tests += $(TEST_DIR)/svm.flat +tests += $(TEST_DIR)/vmx.flat include config-x86-common.mak diff --git a/lib/x86/msr.h b/lib/x86/msr.h index 509a421..281255a 100644 --- a/lib/x86/msr.h +++ b/lib/x86/msr.h @@ -396,6 +396,11 @@ #define MSR_IA32_VMX_VMCS_ENUM 0x048a #define MSR_IA32_VMX_PROCBASED_CTLS20x048b #define MSR_IA32_VMX_EPT_VPID_CAP 0x048c +#define MSR_IA32_VMX_TRUE_PIN 0x048d +#define MSR_IA32_VMX_TRUE_PROC 0x048e +#define MSR_IA32_VMX_TRUE_EXIT 0x048f +#define MSR_IA32_VMX_TRUE_ENTRY0x0490 + /* AMD-V MSRs */ diff --git a/x86/cstart64.S b/x86/cstart64.S index 24df5f8..0fe76da 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -4,6 +4,10 @@ .globl boot_idt boot_idt = 0 +.globl idt_descr +.globl tss_descr +.globl gdt64_desc + ipi_vector = 0x20 max_cpus = 64 diff --git a/x86/unittests.cfg b/x86/unittests.cfg index bc9643e..e846739 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -149,3 +149,9 @@ extra_params = --append 1000 `date +%s` file = pcid.flat extra_params = -cpu qemu64,+pcid arch = x86_64 + +[vmx] +file = vmx.flat +extra_params = -cpu Nehalem,+vmx +arch = x86_64 + diff --git a/x86/vmx.c b/x86/vmx.c new file mode 100644 index 000..0435746 --- /dev/null +++ b/x86/vmx.c @@ -0,0 +1,568 @@ +#include libcflat.h +#include processor.h +#include vm.h +#include desc.h +#include vmx.h +#include msr.h +#include smp.h +#include io.h + + +int fails = 0, tests = 0; +u32 *vmxon_region; +struct vmcs *vmcs_root; +void *io_bmp1, *io_bmp2; +void *msr_bmp; +u32 vpid_ctr; +char *guest_stack, *host_stack; +char *guest_syscall_stack, *host_syscall_stack; +u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2]; +ulong fix_cr0_set, fix_cr0_clr; +ulong fix_cr4_set, fix_cr4_clr; +struct regs regs; + +extern u64 gdt64_desc[]; +extern u64 idt_descr[]; +extern u64 tss_descr[]; +extern void *entry_vmx; +extern void *entry_sysenter; +extern void *entry_guest; + +void report(const char *name, int result) +{ + ++tests; + if (result) + printf(PASS: %s\n, name); + else { + printf(FAIL: %s\n, name); + ++fails; + } +} + +inline u64 get_rflags(void) +{ + u64 r; + asm volatile(pushf; pop %0\n\t : =q(r) : : cc); + return r; +} + +inline void set_rflags(u64 r) +{ + asm volatile(push %0; popf\n\t : : q(r) : cc); +} + +int vmcs_clear(struct vmcs *vmcs) +{ + bool ret; + asm volatile (vmclear %1; seta %0 : =q (ret) : m (vmcs) : cc); + return !ret; +} + +u64 vmcs_read(enum Encoding enc) +{ + u64 val; + asm volatile (vmread
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Tue, Jul 16, 2013 at 05:35:16PM +0800, Arthur Chunqi Li wrote: Hi there, This is a version calling for comments. Some minor changes should be Add RFC before PATCH for such submission please. done before final commitment (TODOs in it), because these places are related to the bugs I have commited in the previous weeks and the relevant patches are not accpeted. But these bugs are all about some I am aware of only one patch. Did I miss/forgot something? unexpected occasions and the hypervisor can run if we simply ignore them. After all bugs fixed, everything will be OK. Arthur -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Tue, Jul 16, 2013 at 5:45 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 05:35:16PM +0800, Arthur Chunqi Li wrote: Hi there, This is a version calling for comments. Some minor changes should be Add RFC before PATCH for such submission please. done before final commitment (TODOs in it), because these places are related to the bugs I have commited in the previous weeks and the relevant patches are not accpeted. But these bugs are all about some I am aware of only one patch. Did I miss/forgot something? I have only commit one patch related to VMX tests. Other patches cited here is about the bug fixes such as [http://www.mail-archive.com/kvm@vger.kernel.org/msg92932.html] [http://www.mail-archive.com/kvm@vger.kernel.org/msg93046.html], which will cause some test cases fail in this patch. I have been discussing with Jan in the past weeks because most of the questions are technical affairs, which I don't think it's suitable to discuss in the community. This is a rather mature version and I think I can commit it with some minor changes. unexpected occasions and the hypervisor can run if we simply ignore them. After all bugs fixed, everything will be OK. Arthur -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Tue, Jul 16, 2013 at 05:53:56PM +0800, Arthur Chunqi Li wrote: On Tue, Jul 16, 2013 at 5:45 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 05:35:16PM +0800, Arthur Chunqi Li wrote: Hi there, This is a version calling for comments. Some minor changes should be Add RFC before PATCH for such submission please. done before final commitment (TODOs in it), because these places are related to the bugs I have commited in the previous weeks and the relevant patches are not accpeted. But these bugs are all about some I am aware of only one patch. Did I miss/forgot something? I have only commit one patch related to VMX tests. Other patches cited here is about the bug fixes such as [http://www.mail-archive.com/kvm@vger.kernel.org/msg92932.html] [http://www.mail-archive.com/kvm@vger.kernel.org/msg93046.html], which will cause some test cases fail in this patch. Those are applied to queue already. I am aware of one that is not applied yet but it looks fine to me, waiting for Paolo's review. I have been discussing with Jan in the past weeks because most of the questions are technical affairs, which I don't think it's suitable to discuss in the community. This is a rather mature version and I think I can commit it with some minor changes. unexpected occasions and the hypervisor can run if we simply ignore them. After all bugs fixed, everything will be OK. Arthur -- Gleb. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
Il 16/07/2013 11:27, Arthur Chunqi Li ha scritto: This is the first version for VMX nested environment test case. It contains the basic VMX instructions test cases, including VMXON/ VMXOFF/VMXPTRLD/VMXPTRST/VMCLEAR/VMLAUNCH/VMRESUME/VMCALL. This patch also tests the basic execution routine in VMX nested environment and let the VM print Hello World to inform its successfully run. New files added: x86/vmx.h : contains all VMX related macro declerations x86/vmx.c : main file for VMX nested test case Signed-off-by: Arthur Chunqi Li yzt...@gmail.com --- config-x86-common.mak |2 + config-x86_64.mak |1 + lib/x86/msr.h |5 + x86/cstart64.S|4 + x86/unittests.cfg |6 + x86/vmx.c | 568 + x86/vmx.h | 406 +++ 7 files changed, 992 insertions(+) create mode 100644 x86/vmx.c create mode 100644 x86/vmx.h diff --git a/config-x86-common.mak b/config-x86-common.mak index 455032b..34a41e1 100644 --- a/config-x86-common.mak +++ b/config-x86-common.mak @@ -101,6 +101,8 @@ $(TEST_DIR)/asyncpf.elf: $(cstart.o) $(TEST_DIR)/asyncpf.o $(TEST_DIR)/pcid.elf: $(cstart.o) $(TEST_DIR)/pcid.o +$(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o + arch_clean: $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \ $(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o diff --git a/config-x86_64.mak b/config-x86_64.mak index 4e525f5..bb8ee89 100644 --- a/config-x86_64.mak +++ b/config-x86_64.mak @@ -9,5 +9,6 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \ $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \ $(TEST_DIR)/pcid.flat tests += $(TEST_DIR)/svm.flat +tests += $(TEST_DIR)/vmx.flat include config-x86-common.mak diff --git a/lib/x86/msr.h b/lib/x86/msr.h index 509a421..281255a 100644 --- a/lib/x86/msr.h +++ b/lib/x86/msr.h @@ -396,6 +396,11 @@ #define MSR_IA32_VMX_VMCS_ENUM 0x048a #define MSR_IA32_VMX_PROCBASED_CTLS20x048b #define MSR_IA32_VMX_EPT_VPID_CAP 0x048c +#define MSR_IA32_VMX_TRUE_PIN0x048d +#define MSR_IA32_VMX_TRUE_PROC 0x048e +#define MSR_IA32_VMX_TRUE_EXIT 0x048f +#define MSR_IA32_VMX_TRUE_ENTRY 0x0490 + /* AMD-V MSRs */ diff --git a/x86/cstart64.S b/x86/cstart64.S index 24df5f8..0fe76da 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -4,6 +4,10 @@ .globl boot_idt boot_idt = 0 +.globl idt_descr +.globl tss_descr +.globl gdt64_desc + ipi_vector = 0x20 max_cpus = 64 diff --git a/x86/unittests.cfg b/x86/unittests.cfg index bc9643e..e846739 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -149,3 +149,9 @@ extra_params = --append 1000 `date +%s` file = pcid.flat extra_params = -cpu qemu64,+pcid arch = x86_64 + +[vmx] +file = vmx.flat +extra_params = -cpu Nehalem,+vmx Should this use -cpu host instead? (Or -cpu host,+vmx, I don't remember). +arch = x86_64 + diff --git a/x86/vmx.c b/x86/vmx.c new file mode 100644 index 000..0435746 --- /dev/null +++ b/x86/vmx.c @@ -0,0 +1,568 @@ +#include libcflat.h +#include processor.h +#include vm.h +#include desc.h +#include vmx.h +#include msr.h +#include smp.h +#include io.h + + +int fails = 0, tests = 0; +u32 *vmxon_region; +struct vmcs *vmcs_root; +void *io_bmp1, *io_bmp2; +void *msr_bmp; +u32 vpid_ctr; +char *guest_stack, *host_stack; +char *guest_syscall_stack, *host_syscall_stack; +u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2]; +ulong fix_cr0_set, fix_cr0_clr; +ulong fix_cr4_set, fix_cr4_clr; +struct regs regs; + +extern u64 gdt64_desc[]; +extern u64 idt_descr[]; +extern u64 tss_descr[]; +extern void *entry_vmx; +extern void *entry_sysenter; +extern void *entry_guest; + +void report(const char *name, int result) +{ + ++tests; + if (result) + printf(PASS: %s\n, name); + else { + printf(FAIL: %s\n, name); + ++fails; + } +} + +inline u64 get_rflags(void) +{ + u64 r; + asm volatile(pushf; pop %0\n\t : =q(r) : : cc); + return r; +} + +inline void set_rflags(u64 r) +{ + asm volatile(push %0; popf\n\t : : q(r) : cc); +} + +int vmcs_clear(struct vmcs *vmcs) +{ + bool ret; + asm volatile (vmclear %1; seta %0 : =q (ret) : m (vmcs) : cc); You can use setbe, it's clearer and avoids the ! in the return statement. We should later add tests for failure conditions, since failing to detect errors could give rise to L2-L1 attack vectors. When we do so, we will have to distinguish CF from ZF. + return !ret; +} + +u64 vmcs_read(enum Encoding enc) +{ + u64 val; + asm volatile (vmread %1, %0 : =rm (val) : r ((u64)enc) : cc); + return val; +} + +int vmcs_write(enum Encoding enc, u64 val)
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
Hi Paolo, On Tue, Jul 16, 2013 at 6:28 PM, Paolo Bonzini pbonz...@redhat.com wrote: Il 16/07/2013 11:27, Arthur Chunqi Li ha scritto: This is the first version for VMX nested environment test case. It contains the basic VMX instructions test cases, including VMXON/ VMXOFF/VMXPTRLD/VMXPTRST/VMCLEAR/VMLAUNCH/VMRESUME/VMCALL. This patch also tests the basic execution routine in VMX nested environment and let the VM print Hello World to inform its successfully run. New files added: x86/vmx.h : contains all VMX related macro declerations x86/vmx.c : main file for VMX nested test case Signed-off-by: Arthur Chunqi Li yzt...@gmail.com --- config-x86-common.mak |2 + config-x86_64.mak |1 + lib/x86/msr.h |5 + x86/cstart64.S|4 + x86/unittests.cfg |6 + x86/vmx.c | 568 + x86/vmx.h | 406 +++ 7 files changed, 992 insertions(+) create mode 100644 x86/vmx.c create mode 100644 x86/vmx.h diff --git a/config-x86-common.mak b/config-x86-common.mak index 455032b..34a41e1 100644 --- a/config-x86-common.mak +++ b/config-x86-common.mak @@ -101,6 +101,8 @@ $(TEST_DIR)/asyncpf.elf: $(cstart.o) $(TEST_DIR)/asyncpf.o $(TEST_DIR)/pcid.elf: $(cstart.o) $(TEST_DIR)/pcid.o +$(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o + arch_clean: $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \ $(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o diff --git a/config-x86_64.mak b/config-x86_64.mak index 4e525f5..bb8ee89 100644 --- a/config-x86_64.mak +++ b/config-x86_64.mak @@ -9,5 +9,6 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \ $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \ $(TEST_DIR)/pcid.flat tests += $(TEST_DIR)/svm.flat +tests += $(TEST_DIR)/vmx.flat include config-x86-common.mak diff --git a/lib/x86/msr.h b/lib/x86/msr.h index 509a421..281255a 100644 --- a/lib/x86/msr.h +++ b/lib/x86/msr.h @@ -396,6 +396,11 @@ #define MSR_IA32_VMX_VMCS_ENUM 0x048a #define MSR_IA32_VMX_PROCBASED_CTLS20x048b #define MSR_IA32_VMX_EPT_VPID_CAP 0x048c +#define MSR_IA32_VMX_TRUE_PIN0x048d +#define MSR_IA32_VMX_TRUE_PROC 0x048e +#define MSR_IA32_VMX_TRUE_EXIT 0x048f +#define MSR_IA32_VMX_TRUE_ENTRY 0x0490 + /* AMD-V MSRs */ diff --git a/x86/cstart64.S b/x86/cstart64.S index 24df5f8..0fe76da 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -4,6 +4,10 @@ .globl boot_idt boot_idt = 0 +.globl idt_descr +.globl tss_descr +.globl gdt64_desc + ipi_vector = 0x20 max_cpus = 64 diff --git a/x86/unittests.cfg b/x86/unittests.cfg index bc9643e..e846739 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -149,3 +149,9 @@ extra_params = --append 1000 `date +%s` file = pcid.flat extra_params = -cpu qemu64,+pcid arch = x86_64 + +[vmx] +file = vmx.flat +extra_params = -cpu Nehalem,+vmx Should this use -cpu host instead? (Or -cpu host,+vmx, I don't remember). +arch = x86_64 + diff --git a/x86/vmx.c b/x86/vmx.c new file mode 100644 index 000..0435746 --- /dev/null +++ b/x86/vmx.c @@ -0,0 +1,568 @@ +#include libcflat.h +#include processor.h +#include vm.h +#include desc.h +#include vmx.h +#include msr.h +#include smp.h +#include io.h + + +int fails = 0, tests = 0; +u32 *vmxon_region; +struct vmcs *vmcs_root; +void *io_bmp1, *io_bmp2; +void *msr_bmp; +u32 vpid_ctr; +char *guest_stack, *host_stack; +char *guest_syscall_stack, *host_syscall_stack; +u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2]; +ulong fix_cr0_set, fix_cr0_clr; +ulong fix_cr4_set, fix_cr4_clr; +struct regs regs; + +extern u64 gdt64_desc[]; +extern u64 idt_descr[]; +extern u64 tss_descr[]; +extern void *entry_vmx; +extern void *entry_sysenter; +extern void *entry_guest; + +void report(const char *name, int result) +{ + ++tests; + if (result) + printf(PASS: %s\n, name); + else { + printf(FAIL: %s\n, name); + ++fails; + } +} + +inline u64 get_rflags(void) +{ + u64 r; + asm volatile(pushf; pop %0\n\t : =q(r) : : cc); + return r; +} + +inline void set_rflags(u64 r) +{ + asm volatile(push %0; popf\n\t : : q(r) : cc); +} + +int vmcs_clear(struct vmcs *vmcs) +{ + bool ret; + asm volatile (vmclear %1; seta %0 : =q (ret) : m (vmcs) : cc); You can use setbe, it's clearer and avoids the ! in the return statement. We should later add tests for failure conditions, since failing to detect errors could give rise to L2-L1 attack vectors. When we do so, we will have to distinguish CF from ZF. + return !ret; +} + +u64 vmcs_read(enum Encoding enc) +{ + u64 val; + asm volatile (vmread %1, %0 : =rm (val) : r ((u64)enc) :
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
Il 16/07/2013 13:47, Arthur Chunqi Li ha scritto: setjmp and longjmp is not implemented in our test environment, and these two functions are highly depend on architecture. Do you think we need to write a general code for both 32bit and 64bit, or just write specific one just for this test case? Supporting it in x86-64 is enough for now. Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Tue, Jul 16, 2013 at 11:20 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote: +void vmx_exit(void) +{ + test_vmxoff(); + printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); + exit(fails ? -1 : 0); +} Can you try to jump back to main, and do test_vmxoff there? This will avoid having to write our tests in callback style, which is a pain. Basically something similar to setjmp/longjmp. In main: if (setjmp(jmpbuf) == 0) { vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } test_vmxoff(); exit: printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); return fails ? 1 : 0; In vmx_handler: case VMX_HLT: printf(\nVM exit.\n); longjmp(jmpbuf, 1); Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It will make code much more straightforward and easer to follow. The concept easier to follow may have different meanings in different view. This achievement puts all the test cases in main function instead of scattering everywhere, which is another view to easy to follow. As this is just a test case, I prefer this one. Besides, this way we can start another VM following the previous one simply in main function. This is flexible if we want to test re-enter to VMX mode or so. Arthur -- Gleb. -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote: +void vmx_exit(void) +{ + test_vmxoff(); + printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); + exit(fails ? -1 : 0); +} Can you try to jump back to main, and do test_vmxoff there? This will avoid having to write our tests in callback style, which is a pain. Basically something similar to setjmp/longjmp. In main: if (setjmp(jmpbuf) == 0) { vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } test_vmxoff(); exit: printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); return fails ? 1 : 0; In vmx_handler: case VMX_HLT: printf(\nVM exit.\n); longjmp(jmpbuf, 1); Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It will make code much more straightforward and easer to follow. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Tue, Jul 16, 2013 at 11:29:20PM +0800, Arthur Chunqi Li wrote: On Tue, Jul 16, 2013 at 11:20 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote: +void vmx_exit(void) +{ + test_vmxoff(); + printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); + exit(fails ? -1 : 0); +} Can you try to jump back to main, and do test_vmxoff there? This will avoid having to write our tests in callback style, which is a pain. Basically something similar to setjmp/longjmp. In main: if (setjmp(jmpbuf) == 0) { vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } test_vmxoff(); exit: printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); return fails ? 1 : 0; In vmx_handler: case VMX_HLT: printf(\nVM exit.\n); longjmp(jmpbuf, 1); Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It will make code much more straightforward and easer to follow. The concept easier to follow may have different meanings in different view. This achievement puts all the test cases in main function instead of scattering everywhere, which is another view to easy to follow. As this is just a test case, I prefer this one. I do not see why what I propose will prevent you to put all tests into main. vmx_run() will looks like that: vmlaunch while(1) { vmresume vmexit jumps here switch(exit reason) { case reason1: break; case reason2: break; case HLT return; } } Besides, this way we can start another VM following the previous one simply in main function. This is flexible if we want to test re-enter to VMX mode or so. That's what I am missing. How do one writes more tests now? I was thinking about interface like that: guest_func_test1() { } tes1t_exit_handlers[] = {test1_handle_hlt, test1_handle_exception, } main() { init_vmcs(); /* generic stuff */ init_vmcs_test1(); /* test1 related stuff */ r = run_in_guest(guest_func_test1, test1_exit_handlers); report(test1, r); } -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Wed, Jul 17, 2013 at 12:45 AM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 11:29:20PM +0800, Arthur Chunqi Li wrote: On Tue, Jul 16, 2013 at 11:20 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote: +void vmx_exit(void) +{ + test_vmxoff(); + printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); + exit(fails ? -1 : 0); +} Can you try to jump back to main, and do test_vmxoff there? This will avoid having to write our tests in callback style, which is a pain. Basically something similar to setjmp/longjmp. In main: if (setjmp(jmpbuf) == 0) { vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } test_vmxoff(); exit: printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); return fails ? 1 : 0; In vmx_handler: case VMX_HLT: printf(\nVM exit.\n); longjmp(jmpbuf, 1); Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It will make code much more straightforward and easer to follow. The concept easier to follow may have different meanings in different view. This achievement puts all the test cases in main function instead of scattering everywhere, which is another view to easy to follow. As this is just a test case, I prefer this one. I do not see why what I propose will prevent you to put all tests into main. vmx_run() will looks like that: vmlaunch while(1) { vmresume vmexit jumps here switch(exit reason) { case reason1: break; case reason2: break; case HLT return; } } Yes, this recalls me some KVM codes I have read before. This mixes vmlaunch/resume and vmx_handler into one piece of code. It is a good way to explicitly show the execution sequence though, it increases LOC in one function. Besides, this way we can start another VM following the previous one simply in main function. This is flexible if we want to test re-enter to VMX mode or so. That's what I am missing. How do one writes more tests now? I was thinking about interface like that: guest_func_test1() { } tes1t_exit_handlers[] = {test1_handle_hlt, test1_handle_exception, } main() { init_vmcs(); /* generic stuff */ init_vmcs_test1(); /* test1 related stuff */ r = run_in_guest(guest_func_test1, test1_exit_handlers); report(test1, r); } I have thought about this question and I'm not quite sure how to solve it now. I have two ways. The first is that we just leave vmx.c as the VMX instructions and execution routine test suite, and develop other test cases in other files. Since all other tests of nested vmx is independent to the basic routine and it is hard for us to put all test cases for nested VMX in one file, so we just let this file do simple things and reuse some of its functions in other test suites of nested vmx. Your proposal of adding new test cases can be implemented in other test suites. The other way is not splitting nested vmx tests cases in contrast. This method may cause a HUGE vmx.c file, and tests for different parts are not distinctive. Actually, I prefer the former solution. -- Gleb. Besides, there is also another pseudo bug in PATCH 2/2, here: +int test_vmx_capability(void) +{ + struct cpuid r; + u64 ret1, ret2; + r = cpuid(1); + ret1 = ((r.c) 5) 1; + ret2 = ((rdmsr(MSR_IA32_FEATURE_CONTROL) 0x5) == 0x5); + report(test vmx capability, ret1 ret2); + return !(ret1 ret2); +} The IA32_FEATURE_CONTROL MSR should be set by seabios. SInce there's no patch for seabios and in fact software can also set this MSR if its lock bit is unset. So I prefer to change it like this: +int test_vmx_capability(void) +{ + struct cpuid r; + u64 ret1, ret2; + u64 ia32_feature_control; + r = cpuid(1); + ret1 = ((r.c) 5) 1; + ia32_feature_control = rdmsr(MSR_IA32_FEATURE_CONTROL); + ret2 = ((ia32_feature_control 0x5) == 0x5); + if ((!ret2) ((ia32_feature_control 0x1) == 0)){ + wrmsr(MSR_IA32_FEATURE_CONTROL, 0x5); + ia32_feature_control = rdmsr(MSR_IA32_FEATURE_CONTROL); + ret2 = ((ia32_feature_control 0x5) == 0x5); + } + report(test vmx capability, ret1 ret2); + return !(ret1 ret2); +} Arthur -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
Il 16/07/2013 19:13, Arthur Chunqi Li ha scritto: On Wed, Jul 17, 2013 at 12:45 AM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 11:29:20PM +0800, Arthur Chunqi Li wrote: On Tue, Jul 16, 2013 at 11:20 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote: +void vmx_exit(void) +{ + test_vmxoff(); + printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); + exit(fails ? -1 : 0); +} Can you try to jump back to main, and do test_vmxoff there? This will avoid having to write our tests in callback style, which is a pain. Basically something similar to setjmp/longjmp. In main: if (setjmp(jmpbuf) == 0) { vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } test_vmxoff(); exit: printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); return fails ? 1 : 0; In vmx_handler: case VMX_HLT: printf(\nVM exit.\n); longjmp(jmpbuf, 1); Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It will make code much more straightforward and easer to follow. The concept easier to follow may have different meanings in different view. This achievement puts all the test cases in main function instead of scattering everywhere, which is another view to easy to follow. As this is just a test case, I prefer this one. I do not see why what I propose will prevent you to put all tests into main. vmx_run() will looks like that: vmlaunch while(1) { vmresume vmexit jumps here switch(exit reason) { case reason1: break; case reason2: break; case HLT return; } } Yes, this recalls me some KVM codes I have read before. This mixes vmlaunch/resume and vmx_handler into one piece of code. It is a good way to explicitly show the execution sequence though, it increases LOC in one function. Besides, this way we can start another VM following the previous one simply in main function. This is flexible if we want to test re-enter to VMX mode or so. That's what I am missing. How do one writes more tests now? I was thinking about interface like that: guest_func_test1() { } tes1t_exit_handlers[] = {test1_handle_hlt, test1_handle_exception, } main() { init_vmcs(); /* generic stuff */ init_vmcs_test1(); /* test1 related stuff */ r = run_in_guest(guest_func_test1, test1_exit_handlers); report(test1, r); } I have thought about this question and I'm not quite sure how to solve it now. Why can't you just use a different vmx_handler (e.g. with an indirect call in entry_vmx) for each test (as in Gleb's test1_exit_handlers)? run_in_guest would prepare the function pointers and do init_vmcs(vmcs_root); if (setjmp(env) == 0){ vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } as in your current testcase. vmx.c would be a library, and testcases could be either grouped in the same file or spread across many of them, as you see fit. Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvm-unit-tests : The first version of VMX nested test case
On Wed, Jul 17, 2013 at 01:13:56AM +0800, Arthur Chunqi Li wrote: On Wed, Jul 17, 2013 at 12:45 AM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 11:29:20PM +0800, Arthur Chunqi Li wrote: On Tue, Jul 16, 2013 at 11:20 PM, Gleb Natapov g...@redhat.com wrote: On Tue, Jul 16, 2013 at 12:28:05PM +0200, Paolo Bonzini wrote: +void vmx_exit(void) +{ + test_vmxoff(); + printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); + exit(fails ? -1 : 0); +} Can you try to jump back to main, and do test_vmxoff there? This will avoid having to write our tests in callback style, which is a pain. Basically something similar to setjmp/longjmp. In main: if (setjmp(jmpbuf) == 0) { vmx_run(); /* Should not reach here */ report(test vmlaunch, 0); } test_vmxoff(); exit: printf(\nSUMMARY: %d tests, %d failures\n, tests, fails); return fails ? 1 : 0; In vmx_handler: case VMX_HLT: printf(\nVM exit.\n); longjmp(jmpbuf, 1); Why not just make vmexit occur after vmlaunch/vmresume like KVM does. It will make code much more straightforward and easer to follow. The concept easier to follow may have different meanings in different view. This achievement puts all the test cases in main function instead of scattering everywhere, which is another view to easy to follow. As this is just a test case, I prefer this one. I do not see why what I propose will prevent you to put all tests into main. vmx_run() will looks like that: vmlaunch while(1) { vmresume vmexit jumps here switch(exit reason) { case reason1: break; case reason2: break; case HLT return; } } Yes, this recalls me some KVM codes I have read before. This mixes vmlaunch/resume and vmx_handler into one piece of code. It is a good way to explicitly show the execution sequence though, it increases LOC in one function. LOC in one function is not an issue to be considered at all. Besides you can put the switch into separate vmx_handler() function, or have an array of vmexits. Besides, this way we can start another VM following the previous one simply in main function. This is flexible if we want to test re-enter to VMX mode or so. That's what I am missing. How do one writes more tests now? I was thinking about interface like that: guest_func_test1() { } tes1t_exit_handlers[] = {test1_handle_hlt, test1_handle_exception, } main() { init_vmcs(); /* generic stuff */ init_vmcs_test1(); /* test1 related stuff */ r = run_in_guest(guest_func_test1, test1_exit_handlers); report(test1, r); } I have thought about this question and I'm not quite sure how to solve it now. I have two ways. The first is that we just leave vmx.c as the VMX instructions and execution routine test suite, and develop other test cases in other files. Since all other tests of nested vmx is independent to the basic routine and it is hard for us to put all test cases for nested VMX in one file, so we just let this file do simple things and reuse some of its functions in other test suites of nested vmx. Your proposal of adding new test cases can be implemented in other test suites. The other way is not splitting nested vmx tests cases in contrast. This method may cause a HUGE vmx.c file, and tests for different parts are not distinctive. Actually, I prefer the former solution. I do not think we need separate infrastructure just to test basic instructions. Actually testing those are less interesting part (well just to sorely test vmlaunch/vmresume we will likely have to write hundred of tests to verify that all things that should cause failure do that, but we likely settle for only a couple). I am not worrying about vmx.c become a huge file as long as writing test is easy and more or less self contained task. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html