Re: [PATCH] kvm-unit-tests: VMX: Comments on the framework and writing test cases

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

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

On 2013-09-17 04:29, Arthur Chunqi Li wrote:
> Add some comments on the framework of nested VMX testing, and guides of
> how to write new test cases.
> 
> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
> ---
>  x86/vmx.c       |   25 +++++++++++++++++++++++++
>  x86/vmx_tests.c |   13 +++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 9db4ef4..3aa8600 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -1,3 +1,28 @@
> +/*
> + * x86/vmx.c : Framework for testing nested virtualization
> + *	This is a framework to test nested VMX for KVM, which is
> + * 	a project of GSoC 2013. All test cases are located in

"...which started as a project of GSoC"

> + *	"vmx_tests", which is defined in x86/vmx_tests.c. All test
> + *	cases should be located in x86/vmx_tests.c and framework
> + *	related functions should be in this file.

There is some redundancy in the last two sentences. Please consolidate.

> + *
> + * How to write test suite?

Probably "test case" is better, suite is the whole thing.

> + *	Add functions of test suite in variant "vmx_tests". You can

"Callbacks" is probably clearer than "functions".

> + *	write:
> + *		init function used for initializing test suite
> + *		main function for codes running in L2 guest, 
> + *		exit_handler to handle vmexit of L2 to L1 (framework)

What do you mean with "framework" here?

> + *		syscall handler to handle L2 syscall vmexit
> + *		vmenter fail handler to handle direct failure of vmenter
> + *		init registers used to store register value in initialization

The last item is unclear to me (without reading the code). Can you
rephrase to clarify who set these registers when and why?

Also, please make this a bullet list, then you can describe things in
multiple lines where needed.

> + *	If no special function is needed for a test suite, you can use
> + *	basic_* series of functions. More handlers can be added to

Is it optional to set the callbacks to a basic_* handler or mandatory?
"Can" may also suggest that NULL is fine, but it isn't. So the sentence
should rather end like "..., use corresponding basic_* function as
callback."

BTW, test_run considers the init() callback optional.

> + *	"vmx_tests", see details of "struct vmx_test" and function
> + *	test_run().
> + *

It would be good to briefly explain what the vmx test framework does to
set up the test environment, e.g. that it uses the same paging for L2
and L1, that there is currently only one VCPU etc.

Jan

> + * Author : Arthur Chunqi Li <yzt356@gmail.com>
> + */
> +
>  #include "libcflat.h"
>  #include "processor.h"
>  #include "vm.h"
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 0759e10..5fc16a3 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1,3 +1,8 @@
> +/*
> + * All test cases of nested virtualization should be in this file
> + *
> + * Author : Arthur Chunqi Li <yzt356@gmail.com>
> + */
>  #include "vmx.h"
>  #include "msr.h"
>  #include "processor.h"
> @@ -782,6 +787,14 @@ struct insn_table {
>  	u32 test_field;
>  };
>  
> +/*
> + * Add more test cases of instruction intercept here. Elements in this
> + * table is:
> + *	name/control flag/insn function/type/exit reason/exit qulification/
> + *	instruction info/field to test
> + * The last field defines which fields (exit_qual and insn_info) need to be
> + * tested in exit handler. If set to 0, only "reason" is checked.
> + */
>  static struct insn_table insn_table[] = {
>  	// Flags for Primary Processor-Based VM-Execution Controls
>  	{"HLT",  CPU_HLT, insn_hlt, INSN_CPU0, 12, 0, 0, 0},
> 



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