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: 5502 bytes --]

On 2013-08-04 19:18, Arthur Chunqi Li wrote:
>>> 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.
> Here you mean put definition in vmx.c and "extern" definition in vmx.h?

Yes.

>>
>>>
>>> -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.
> You mean put it in vmx.c and "extern" it here?

Yes.

>>
>>> +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
> I have tried to put vmx_tests in vmx_tests.c and extern definition in
> vmx.h. But it is impossible to get its size statically when compiling,
> thus the codes "for (i = 1; i < ARRAY_SIZE(vmx_tests); ++i) {" in main
> of vmx.c is invalid. If we use ARRAY_SIZE to get its size, we should
> make it static.
> 
> An alternative solution is put vmx_tests in vmx_tests.h, which will be
> included by vmx.c

Another alternative is to iterate over vmx_tests until you hit a NULL
entry (one mandatory element of an entry is NULL), i.e. exploring its
size at runtime.

Jan



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