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

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, rkrcmar@redhat.com,
	jmattson@google.com
Subject: Re: [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests
Date: Mon, 8 Apr 2019 10:34:41 -0700	[thread overview]
Message-ID: <20190408173441.GI25880@linux.intel.com> (raw)
In-Reply-To: <20190405014650.16880-7-krish.sadhukhan@oracle.com>

On Thu, Apr 04, 2019 at 09:46:50PM -0400, Krish Sadhukhan wrote:
> .to verify KVM performs the appropriate consistency checks for loading
> IA32_PAT as part of running a nested guest.
> 
> According to section "Checks on Host Control Registers and MSRs" in Intel
> SDM vol 3C, the following check is performed on vmentry:
> 
>     If the “load IA32_PAT” VM-exit control is 1, the value of the field
>     for the IA32_PAT MSR must be one that could be written by WRMSR
>     without fault at CPL 0. Specifically, each of the 8 bytes in the
>     field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
>     6 (WB), or 7 (UC-).
> 
> Since a PAT value higher than 8 will yield the same test result as that
> of 8, we want to confine our tests only up to 8 in order to reduce
> redundancy of tests and to avoid too many vmentries.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---

One nit below.

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

>  x86/vmx_tests.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 66a87f6..96e6f17 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4995,6 +4995,76 @@ static void test_sysenter_field(u32 field, const char *name)
>  	vmcs_write(field, addr_saved);
>  }
>  
> +/*
> + * Since a PAT value higher than 8 will yield the same test result as that
> + * of 8, we want to confine our tests only up to 8 in order to reduce
> + * redundancy of tests and to avoid too many vmentries.
> + */

For this type of comment, it's better to say that those cases are
"uninteresting" as opposed to "will yield the same test result", as we
can't actually assume or guarantee anything about KVM's behavior.  E.g.:

/*
 * PAT values higher than 8 are uninteresting since they're likely lumped in
 * with "8", e.g. "val >= 8".  Cap the tests at "8" to reduce the number of
 * VM-Entries and keep the runtime reasonable.
 */

> +#define	PAT_VAL_LIMIT	8
> +
> +static void test_pat(u32 field, const char * field_name, u32 ctrl_field,
> +		     u64 ctrl_bit)
> +{
> +	u32 ctrl_saved = vmcs_read(ctrl_field);
> +	u64 pat_saved = vmcs_read(field);
> +	u64 i, val;
> +	u32 j;
> +	int error;
> +
> +	vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit);
> +	for (i = 0; i <= PAT_VAL_LIMIT; i++) {
> +		/* Test PAT0..PAT7 fields */
> +		for (j = 0; j < 8; j++) {
> +			val = i << j * 8;
> +			vmcs_write(field, val);
> +			report_prefix_pushf("%s %lx", field_name, val);
> +			test_vmx_vmlaunch(0, false);
> +			report_prefix_pop();
> +		}
> +	}
> +
> +	vmcs_write(ctrl_field, ctrl_saved | ctrl_bit);
> +	for (i = 0; i <= PAT_VAL_LIMIT; i++) {
> +		/* Test PAT0..PAT7 fields */
> +		for (j = 0; j < 8; j++) {
> +			val = i << j * 8;
> +			vmcs_write(field, val);
> +			report_prefix_pushf("%s %lx", field_name, val);
> +			if (i == 0x2 || i == 0x3 || i >= 0x8)
> +				error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> +			else
> +				error = 0;
> +			test_vmx_vmlaunch(error, false);
> +			report_prefix_pop();
> +		}
> +	}
> +
> +	vmcs_write(ctrl_field, ctrl_saved);
> +	vmcs_write(field, pat_saved);
> +}
> +
> +/*
> + *  If the "load IA32_PAT" VM-exit control is 1, the value of the field
> + *  for the IA32_PAT MSR must be one that could be written by WRMSR
> + *  without fault at CPL 0. Specifically, each of the 8 bytes in the
> + *  field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> + *  6 (WB), or 7 (UC-).
> + *
> + *  [Intel SDM]
> + */
> +static void test_load_host_pat(void)
> +{
> +	/*
> +	 * "load IA32_PAT" VM-exit control
> +	 */
> +	if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT)) {
> +		printf("\"Load-IA32-PAT\" exit control not supported\n");
> +		return;
> +	}
> +
> +	test_pat(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT);
> +}
> +
>  /*
>   * Check that the virtual CPU checks the VMX Host State Area as
>   * documented in the Intel SDM.
> @@ -5010,6 +5080,8 @@ static void vmx_host_state_area_test(void)
>  
>  	test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP");
>  	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
> +
> +	test_load_host_pat();
>  }
>  
>  static bool valid_vmcs_for_vmentry(void)
> -- 
> 2.17.2
> 