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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 7849 bytes --]

On 2013-07-31 11:22, 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>
> ---
>  config-x86-common.mak |    2 +-
>  x86/vmx.c             |   71 +++----------------------------------------------
>  x86/vmx.h             |   43 ++++++++++++++++++++++++------
>  x86/vmx_tests.c       |   43 ++++++++++++++++++++++++++++++
>  x86/vmx_tests.h       |    7 +++++
>  5 files changed, 90 insertions(+), 76 deletions(-)
>  create mode 100644 x86/vmx_tests.c
>  create mode 100644 x86/vmx_tests.h
> 
> 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..397554a 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -6,19 +6,7 @@
>  #include "msr.h"
>  #include "smp.h"
>  #include "io.h"
> -
> -int fails = 0, tests = 0;
> -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;
> -bool launched;
> +#include "vmx_tests.h"
>  
>  extern u64 gdt64_desc[];
>  extern u64 idt_descr[];
> @@ -27,17 +15,6 @@ extern void *vmx_return;
>  extern void *entry_sysenter;
>  extern void *guest_entry;
>  
> -static void report(const char *name, int result)
> -{
> -	++tests;
> -	if (result)
> -		printf("PASS: %s\n", name);
> -	else {
> -		printf("FAIL: %s\n", name);
> -		++fails;
> -	}
> -}
> -
>  static int make_vmcs_current(struct vmcs *vmcs)
>  {
>  	bool ret;
> @@ -80,7 +57,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;
> @@ -587,48 +564,6 @@ 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[] = {
> @@ -644,6 +579,8 @@ int main(void)
>  
>  	setup_vm();
>  	setup_idt();
> +	fails = tests = 0;
> +	hypercall_field = 0;
>  
>  	if (test_vmx_capability() != 0) {
>  		printf("ERROR : vmx not supported, check +vmx option\n");
> diff --git a/x86/vmx.h b/x86/vmx.h
> index d80e000..f82bf5a 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;
> @@ -55,35 +55,35 @@ static union vmx_basic {
>  	};
>  } basic;

extern union vmx_basic basic, and stick the definition in vmx.c. Same
pattern for the other cases.

>  
> -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,
> @@ -432,6 +432,20 @@ enum Ctrl1 {
>  #define HYPERCALL_MASK		0xFFF
>  #define HYPERCALL_VMEXIT	0x1
>  
> +
> +u32 *vmxon_region;
> +struct vmcs *vmcs_root;
> +void *guest_stack, *guest_syscall_stack;
> +int fails, tests;
> +u64 hypercall_field;
> +u32 vpid_cnt;
> +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;
> +bool launched;
> +

extern <type> <varname>

But do all these variables need to be exported to the test cases? Better
confine it to a minimum, exporting more when test cases come along that
actually need them.

>  static inline int vmcs_clear(struct vmcs *vmcs)
>  {
>  	bool ret;
> @@ -462,5 +476,18 @@ static inline int vmcs_save(struct vmcs **vmcs)
>  	return ret;
>  }
>  
> +static inline void report(const char *name, int result)
> +{
> +	++tests;
> +	if (result)
> +		printf("PASS: %s\n", name);
> +	else {
> +		printf("FAIL: %s\n", name);
> +		++fails;
> +	}
> +}
> +

Why static inline? Better leave this as service of the vmx core.

> +void print_vmexit_info();
> +
>  #endif
>  
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> new file mode 100644
> index 0000000..6bd36b0
> --- /dev/null
> +++ b/x86/vmx_tests.c
> @@ -0,0 +1,43 @@
> +#include "vmx.h"
> +
> +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 (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;
> +}
> +
> diff --git a/x86/vmx_tests.h b/x86/vmx_tests.h
> new file mode 100644
> index 0000000..41580ff
> --- /dev/null
> +++ b/x86/vmx_tests.h
> @@ -0,0 +1,7 @@
> +#ifndef __VMX_TESTS_H
> +#define __VMX_TESTS_H
> +
> +void vmenter_main();
> +int vmenter_exit_handler();

I'd rather move vmx_tests to, well, vmx_tests.c

> +
> +#endif
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]