[6/6,v5,kvm-unit-test,nVMX] : Check "load IA32_PAT" on vmentry of L2 guests
diff mbox series

Message ID 20190408213516.17966-7-krish.sadhukhan@oracle.com
State New
Headers show
Series
  • [1/6,v5,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry
Related show

Commit Message

Krish Sadhukhan April 8, 2019, 9:35 p.m. UTC
.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>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 x86/vmx_tests.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

Comments

Paolo Bonzini April 10, 2019, 12:03 p.m. UTC | #1
On 08/04/19 23:35, 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>
> 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..380fd85 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);
>  }
>  
> +/*
> + * 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.
> + */
> +#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();
> +		}

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++) {
 			val = i << j * 8;
 			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
Sean Christopherson April 10, 2019, 5:17 p.m. UTC | #2
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
Paolo Bonzini April 10, 2019, 5:22 p.m. UTC | #3
On 10/04/19 19:17, Sean Christopherson wrote:
>>  	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.:

No, it is what I intended.  The reason is that i == 0 gives the same "i
<< (j * 8)" for all values of j.  Otherwise, the logs show 8 entries for
GUEST_IA32_PAT = 0. :)

Paolo
Sean Christopherson April 10, 2019, 5:34 p.m. UTC | #4
On Wed, Apr 10, 2019 at 07:22:51PM +0200, Paolo Bonzini wrote:
> On 10/04/19 19:17, Sean Christopherson wrote:
> >>  	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.:
> 
> No, it is what I intended.  The reason is that i == 0 gives the same "i
> << (j * 8)" for all values of j.  Otherwise, the logs show 8 entries for
> GUEST_IA32_PAT = 0. :)

*sigh*  Math is hard.  Thanks!

Patch
diff mbox series

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 66a87f6..380fd85 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);
 }
 
+/*
+ * 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.
+ */
+#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)