Re: [PATCH v3] kvm-unit-tests: VMX: Split VMX test suites to separate file

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/05/2013 05:32 AM, Arthur Chunqi Li wrote:
> Reconstruct VMX codes and put all VMX test suites in x86/vmx_tests.c.
>
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> ---
>
> ChangeLog to v2:
> 	Remove some unused extern definitions in vmx.h.
>
>   config-x86-common.mak |    2 +-
>   x86/vmx.c             |  115 ++++++++-----------------------------------------
>   x86/vmx.h             |   41 ++++++++++++------
>   x86/vmx_tests.c       |   88 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 135 insertions(+), 111 deletions(-)
>   create mode 100644 x86/vmx_tests.c
>
> diff --git a/config-x86-common.mak b/config-x86-common.mak
> index 34a41e1..bf88c67 100644
> --- a/config-x86-common.mak
> +++ b/config-x86-common.mak
> @@ -101,7 +101,7 @@ $(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
> +$(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o $(TEST_DIR)/vmx_tests.o
>
>   arch_clean:
>   	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 082c3bb..efee3df 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -7,19 +7,24 @@
>   #include "smp.h"
>   #include "io.h"
>
> -int fails = 0, tests = 0;
> +int fails, tests;
>   u32 *vmxon_region;
>   struct vmcs *vmcs_root;
>   u32 vpid_cnt;
>   void *guest_stack, *guest_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;
>   struct vmx_test *current;
> -u64 hypercall_field = 0;
> +u64 hypercall_field;
>   bool launched;

If these are not used outside vmx.c, please make them static.  Otherwise 
looks good.  I'll apply this patch as soon as I test it.

Paolo

> +union vmx_basic basic;
> +union vmx_ctrl_pin ctrl_pin_rev;
> +union vmx_ctrl_cpu ctrl_cpu_rev[2];
> +union vmx_ctrl_exit ctrl_exit_rev;
> +union vmx_ctrl_ent ctrl_enter_rev;
> +union vmx_ept_vpid  ept_vpid;
> +
>   extern u64 gdt64_desc[];
>   extern u64 idt_descr[];
>   extern u64 tss_descr[];
> @@ -27,7 +32,7 @@ extern void *vmx_return;
>   extern void *entry_sysenter;
>   extern void *guest_entry;
>
> -static void report(const char *name, int result)
> +void report(const char *name, int result)
>   {
>   	++tests;
>   	if (result)
> @@ -80,7 +85,7 @@ static inline int vmx_off()
>   	return ret;
>   }
>
> -static void print_vmexit_info()
> +void print_vmexit_info()
>   {
>   	u64 guest_rip, guest_rsp;
>   	ulong reason = vmcs_read(EXI_REASON) & 0xff;
> @@ -311,6 +316,9 @@ static int init_vmcs(struct vmcs **vmcs)
>
>   static void init_vmx(void)
>   {
> +	ulong fix_cr0_set, fix_cr0_clr;
> +	ulong fix_cr4_set, fix_cr4_clr;
> +
>   	vmxon_region = alloc_page();
>   	memset(vmxon_region, 0, PAGE_SIZE);
>
> @@ -440,12 +448,10 @@ static int exit_handler()
>   	int ret;
>
>   	current->exits++;
> -	current->guest_regs = regs;
>   	if (is_hypercall())
>   		ret = handle_hypercall();
>   	else
>   		ret = current->exit_handler();
> -	regs = current->guest_regs;
>   	switch (ret) {
>   	case VMX_TEST_VMEXIT:
>   	case VMX_TEST_RESUME:
> @@ -552,98 +558,16 @@ static int test_run(struct vmx_test *test)
>   	return 0;
>   }
>
> -static void basic_init()
> -{
> -}
> -
> -static void basic_guest_main()
> -{
> -	/* Here is null guest_main, print Hello World */
> -	printf("\tHello World, this is null_guest_main!\n");
> -}
> -
> -static int basic_exit_handler()
> -{
> -	u64 guest_rip;
> -	ulong reason;
> -
> -	guest_rip = vmcs_read(GUEST_RIP);
> -	reason = vmcs_read(EXI_REASON) & 0xff;
> -
> -	switch (reason) {
> -	case VMX_VMCALL:
> -		print_vmexit_info();
> -		vmcs_write(GUEST_RIP, guest_rip + 3);
> -		return VMX_TEST_RESUME;
> -	default:
> -		break;
> -	}
> -	printf("ERROR : Unhandled vmx exit.\n");
> -	print_vmexit_info();
> -	return VMX_TEST_EXIT;
> -}
> -
> -static void basic_syscall_handler(u64 syscall_no)
> -{
> -}
> -
> -static void vmenter_main()
> -{
> -	u64 rax;
> -	u64 rsp, resume_rsp;
> -
> -	report("test vmlaunch", 1);
> -
> -	asm volatile(
> -		"mov %%rsp, %0\n\t"
> -		"mov %3, %%rax\n\t"
> -		"vmcall\n\t"
> -		"mov %%rax, %1\n\t"
> -		"mov %%rsp, %2\n\t"
> -		: "=r"(rsp), "=r"(rax), "=r"(resume_rsp)
> -		: "g"(0xABCD));
> -	report("test vmresume", (rax == 0xFFFF) && (rsp == resume_rsp));
> -}
> -
> -static int vmenter_exit_handler()
> -{
> -	u64 guest_rip;
> -	ulong reason;
> -
> -	guest_rip = vmcs_read(GUEST_RIP);
> -	reason = vmcs_read(EXI_REASON) & 0xff;
> -	switch (reason) {
> -	case VMX_VMCALL:
> -		if (current->guest_regs.rax != 0xABCD) {
> -			report("test vmresume", 0);
> -			return VMX_TEST_VMEXIT;
> -		}
> -		current->guest_regs.rax = 0xFFFF;
> -		vmcs_write(GUEST_RIP, guest_rip + 3);
> -		return VMX_TEST_RESUME;
> -	default:
> -		report("test vmresume", 0);
> -		print_vmexit_info();
> -	}
> -	return VMX_TEST_VMEXIT;
> -}
> -
> -
> -/* name/init/guest_main/exit_handler/syscall_handler/guest_regs
> -   basic_* just implement some basic functions */
> -static struct vmx_test vmx_tests[] = {
> -	{ "null", basic_init, basic_guest_main, basic_exit_handler,
> -		basic_syscall_handler, {0} },
> -	{ "vmenter", basic_init, vmenter_main, vmenter_exit_handler,
> -		basic_syscall_handler, {0} },
> -};
> +extern struct vmx_test vmx_tests[];
>
>   int main(void)
>   {
> -	int i;
> +	int i = 0;
>
>   	setup_vm();
>   	setup_idt();
> +	fails = tests = 0;
> +	hypercall_field = 0;
>
>   	if (test_vmx_capability() != 0) {
>   		printf("ERROR : vmx not supported, check +vmx option\n");
> @@ -664,10 +588,9 @@ int main(void)
>   	}
>   	test_vmxoff();
>
> -	for (i = 1; i < ARRAY_SIZE(vmx_tests); ++i) {
> +	while (vmx_tests[++i].name != NULL)
>   		if (test_run(&vmx_tests[i]))
>   			goto exit;
> -	}
>
>   exit:
>   	printf("\nSUMMARY: %d tests, %d failures\n", tests, fails);
> diff --git a/x86/vmx.h b/x86/vmx.h
> index d80e000..06d31c7 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -1,5 +1,5 @@
> -#ifndef __HYPERVISOR_H
> -#define __HYPERVISOR_H
> +#ifndef __VMX_H
> +#define __VMX_H
>
>   #include "libcflat.h"
>
> @@ -41,7 +41,7 @@ struct vmx_test {
>   	int exits;
>   };
>
> -static union vmx_basic {
> +union vmx_basic {
>   	u64 val;
>   	struct {
>   		u32 revision;
> @@ -53,37 +53,37 @@ static union vmx_basic {
>   			insouts:1,
>   			ctrl:1;
>   	};
> -} basic;
> +};
>
> -static union vmx_ctrl_pin {
> +union vmx_ctrl_pin {
>   	u64 val;
>   	struct {
>   		u32 set, clr;
>   	};
> -} ctrl_pin_rev;
> +};
>
> -static union vmx_ctrl_cpu {
> +union vmx_ctrl_cpu {
>   	u64 val;
>   	struct {
>   		u32 set, clr;
>   	};
> -} ctrl_cpu_rev[2];
> +};
>
> -static union vmx_ctrl_exit {
> +union vmx_ctrl_exit {
>   	u64 val;
>   	struct {
>   		u32 set, clr;
>   	};
> -} ctrl_exit_rev;
> +};
>
> -static union vmx_ctrl_ent {
> +union vmx_ctrl_ent {
>   	u64 val;
>   	struct {
>   		u32 set, clr;
>   	};
> -} ctrl_enter_rev;
> +};
>
> -static union vmx_ept_vpid {
> +union vmx_ept_vpid {
>   	u64 val;
>   	struct {
>   		u32:16,
> @@ -93,7 +93,7 @@ static union vmx_ept_vpid {
>   			: 11;
>   		u32	invvpid:1;
>   	};
> -} ept_vpid;
> +};
>
>   struct descr {
>   	u16 limit;
> @@ -432,6 +432,16 @@ enum Ctrl1 {
>   #define HYPERCALL_MASK		0xFFF
>   #define HYPERCALL_VMEXIT	0x1
>
> +
> +extern struct regs regs;
> +
> +extern union vmx_basic basic;
> +extern union vmx_ctrl_pin ctrl_pin_rev;
> +extern union vmx_ctrl_cpu ctrl_cpu_rev[2];
> +extern union vmx_ctrl_exit ctrl_exit_rev;
> +extern union vmx_ctrl_ent ctrl_enter_rev;
> +extern union vmx_ept_vpid  ept_vpid;
> +
>   static inline int vmcs_clear(struct vmcs *vmcs)
>   {
>   	bool ret;
> @@ -462,5 +472,8 @@ static inline int vmcs_save(struct vmcs **vmcs)
>   	return ret;
>   }
>
> +void report(const char *name, int result);
> +void print_vmexit_info();
> +
>   #endif
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> new file mode 100644
> index 0000000..1995837
> --- /dev/null
> +++ b/x86/vmx_tests.c
> @@ -0,0 +1,88 @@
> +#include "vmx.h"
> +
> +void basic_init()
> +{
> +}
> +
> +void basic_guest_main()
> +{
> +	/* Here is a basic guest_main, print Hello World */
> +	printf("\tHello World, this is null_guest_main!\n");
> +}
> +
> +int basic_exit_handler()
> +{
> +	u64 guest_rip;
> +	ulong reason;
> +
> +	guest_rip = vmcs_read(GUEST_RIP);
> +	reason = vmcs_read(EXI_REASON) & 0xff;
> +
> +	switch (reason) {
> +	case VMX_VMCALL:
> +		print_vmexit_info();
> +		vmcs_write(GUEST_RIP, guest_rip + 3);
> +		return VMX_TEST_RESUME;
> +	default:
> +		break;
> +	}
> +	printf("ERROR : Unhandled vmx exit.\n");
> +	print_vmexit_info();
> +	return VMX_TEST_EXIT;
> +}
> +
> +void basic_syscall_handler(u64 syscall_no)
> +{
> +}
> +
> +void vmenter_main()
> +{
> +	u64 rax;
> +	u64 rsp, resume_rsp;
> +
> +	report("test vmlaunch", 1);
> +
> +	asm volatile(
> +		"mov %%rsp, %0\n\t"
> +		"mov %3, %%rax\n\t"
> +		"vmcall\n\t"
> +		"mov %%rax, %1\n\t"
> +		"mov %%rsp, %2\n\t"
> +		: "=r"(rsp), "=r"(rax), "=r"(resume_rsp)
> +		: "g"(0xABCD));
> +	report("test vmresume", (rax == 0xFFFF) && (rsp == resume_rsp));
> +}
> +
> +int vmenter_exit_handler()
> +{
> +	u64 guest_rip;
> +	ulong reason;
> +
> +	guest_rip = vmcs_read(GUEST_RIP);
> +	reason = vmcs_read(EXI_REASON) & 0xff;
> +	switch (reason) {
> +	case VMX_VMCALL:
> +		if (regs.rax != 0xABCD) {
> +			report("test vmresume", 0);
> +			return VMX_TEST_VMEXIT;
> +		}
> +		regs.rax = 0xFFFF;
> +		vmcs_write(GUEST_RIP, guest_rip + 3);
> +		return VMX_TEST_RESUME;
> +	default:
> +		report("test vmresume", 0);
> +		print_vmexit_info();
> +	}
> +	return VMX_TEST_VMEXIT;
> +}
> +
> +/* name/init/guest_main/exit_handler/syscall_handler/guest_regs
> +   basic_* just implement some basic functions */
> +struct vmx_test vmx_tests[] = {
> +	{ "null", basic_init, basic_guest_main, basic_exit_handler,
> +		basic_syscall_handler, {0} },
> +	{ "vmenter", basic_init, vmenter_main, vmenter_exit_handler,
> +		basic_syscall_handler, {0} },
> +	{ NULL, NULL, NULL, NULL, NULL, {0} },
> +};
> +
>