Re: [PATCH 6/6 v5][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Wed, Apr 10, 2019 at 02:03:54PM +0200, Paolo Bonzini wrote:
> Here are some small changes to remove redundant tests and also
> improve coverage of values > 8:
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index fd1f483..7adc76a 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -6633,8 +6633,8 @@ static void test_host_ctl_regs(void)
>  
>  /*
>   * PAT values higher than 8 are uninteresting since they're likely lumped
> - * in with "8". We cap the tests at PAT value of 8 in order to reduce the
> - * number of VM-Entries and keep the runtime reasonable.
> + * in with "8". We only test values above 8 one bit at a time,
> + * in order to reduce the number of VM-Entries and keep the runtime reasonable.
>   */
>  #define	PAT_VAL_LIMIT	8
>  
> @@ -6648,9 +6648,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>  	int error;
>  
>  	vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit);
> -	for (i = 0; i <= PAT_VAL_LIMIT; i++) {
> +	for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) {
>  		/* Test PAT0..PAT7 fields */
> -		for (j = 0; j < 8; j++) {
> +		for (j = 0; j < (i ? 8 : 1); j++) {

I don't think "j < (i ? 8 : 1)" is what you intended.  As-is only i==0,
i.e. UC memtype, gets shortcircuited to test PAT0 only.  Did you perhaps
intend to test only PAT0 for i>8?  E.g.:

		for (j = 0; j < (i <= PAT_VAL_LIMIT : 8 ? 1); j++)

>  			val = i << j * 8;

As an alternative to iterating over PAT0..PAT7, which is the real source
of pain, what about randomizing the start index and shifting values through
that?  E.g.:

	j = rand();

	for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2, j++) {
		val = i << ((j % 8) * 8);
		vmcs_write(field, val);
		report_prefix_pushf("%s %lx", field_name, val);
	}

And at that point I'd be ok hitting all values [0..255].


Which indirectly broaches another topic: how do people feel about
introducing randomness into kvm-unit-tests?  Or perhaps selftests would
be a better landing spot since randomness would take us even further
away from true "unit tests".

>  			vmcs_write(field, val);
>  			report_prefix_pushf("%s %lx", field_name, val);
> @@ -6660,9 +6660,9 @@ static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
>  	}
>  
>  	vmcs_write(ctrl_field, ctrl_saved | ctrl_bit);
> -	for (i = 0; i <= PAT_VAL_LIMIT; i++) {
> +	for (i = 0; i < 256; i = (i < PAT_VAL_LIMIT) ? i + 1 : i * 2) {
>  		/* Test PAT0..PAT7 fields */
> -		for (j = 0; j < 8; j++) {
> +		for (j = 0; j < (i ? 8 : 1); j++) {
>  			val = i << j * 8;
>  			vmcs_write(field, val);
>  			report_prefix_pushf("%s %lx", field_name, val);
> 
> For now I queued the patch with thse changes, holler if you disagree!
> 
> Thanks,
> 
> Paolo