[kvm-unit-test nVMX]: Test "load IA32_PAT" VM-entry control on vmentry of nested guests

[kvm-unit-test nVMX]: Test "load IA32_PAT" VM-entry control on vmentry of nested guests

[Krish Sadhukhan]

This is the unit test for the "load IA32_PAT" VM-entry control. Patch# 2
builds on top of my previous patch,

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


[PATCH 1/2][kvm-unit-test nVMX]: Move the functionality of enter_guest() to
[PATCH 2/2][kvm-unit-test nVMX]: Check "load IA32_PAT" VM-entry control on vmentry

 x86/vmx.c       |  27 +++++++----
 x86/vmx.h       |   4 ++
 x86/vmx_tests.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 143 insertions(+), 28 deletions(-)

Krish Sadhukhan (2):
      nVMX: Move the functionality of enter_guest() to __enter_guest() and make the former a wrapper of the latter
      nVMX: Check "load IA32_PAT" VM-entry control on vmentry of nested guests

[PATCH 1/2][kvm-unit-test nVMX]: Move the functionality of enter_guest() to __enter_guest() and make the former a wrapper of the latter

[Krish Sadhukhan]

Currently enter_guest() aborts on any type of vmentry failures (early
failures or invalid guest state). But the vmentry tests that want to
validate the guest state need to continue running in spite of vmentry
failures arising out of an invalid guest state.

This patch moves the functionality of enter_guest() to __enter_guest() which
accepts a parameter to conditionally abort the test as opposed to the behavior
of enter_guest(). enter_guest() now calls __enter_guest() and preserves
compatibility of existing callers which expect the test to be aborted on
any type of vmentry failures.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx.c | 27 ++++++++++++++++++---------
 x86/vmx.h |  4 ++++
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 6ba56bc..d29d21a 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -1583,10 +1583,10 @@ entry_failure_handler(struct vmentry_failure *failure)
 }
 
 /*
- * Tries to enter the guest. Returns true iff entry succeeded. Otherwise,
+ * Tries to enter the guest. Returns true if entry succeeded. Otherwise,
  * populates @failure.
  */
-static bool vmx_enter_guest(struct vmentry_failure *failure)
+static void vmx_enter_guest(struct vmentry_failure *failure)
 {
 	failure->early = 0;
 
@@ -1620,8 +1620,6 @@ static bool vmx_enter_guest(struct vmentry_failure *failure)
 
 	failure->vmlaunch = !launched;
 	failure->instr = launched ? "vmresume" : "vmlaunch";
-
-	return !failure->early && !(vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE);
 }
 
 static int vmx_run(void)
@@ -1631,7 +1629,9 @@ static int vmx_run(void)
 		bool entered;
 		struct vmentry_failure failure;
 
-		entered = vmx_enter_guest(&failure);
+		vmx_enter_guest(&failure);
+		entered = !failure.early &&
+			  !(vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE);
 
 		if (entered) {
 			/*
@@ -1765,10 +1765,9 @@ void test_set_guest(test_guest_func func)
 
 /*
  * Enters the guest (or launches it for the first time). Error to call once the
- * guest has returned (i.e., run past the end of its guest() function). Also
- * aborts if guest entry fails.
+ * guest has returned (i.e., run past the end of its guest() function).
  */
-void enter_guest(void)
+void __enter_guest(u8 abort_flag)
 {
 	struct vmentry_failure failure;
 
@@ -1778,7 +1777,11 @@ void enter_guest(void)
 	TEST_ASSERT_MSG(!guest_finished,
 			"Called enter_guest() after guest returned.");
 
-	if (!vmx_enter_guest(&failure)) {
+	vmx_enter_guest(&failure);
+	if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure.early) ||
+	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
+	    vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
+
 		print_vmentry_failure_info(&failure);
 		abort();
 	}
@@ -1807,6 +1810,12 @@ void enter_guest(void)
 	}
 }
 
+void enter_guest(void)
+{
+	__enter_guest(ABORT_ON_EARLY_VMENTRY_FAIL |
+		      ABORT_ON_INVALID_GUEST_STATE);
+}
+
 extern struct vmx_test vmx_tests[];
 
 static bool
diff --git a/x86/vmx.h b/x86/vmx.h
index 8a00f73..eefd5dc 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -645,6 +645,9 @@ enum vm_instruction_error_number {
 #define VMCS_FIELD_RESERVED_SHIFT	(15)
 #define VMCS_FIELD_BIT_SIZE		(BITS_PER_LONG)
 
+#define        ABORT_ON_EARLY_VMENTRY_FAIL     0x1
+#define        ABORT_ON_INVALID_GUEST_STATE    0x2
+
 extern struct regs regs;
 
 extern union vmx_basic basic;
@@ -830,6 +833,7 @@ bool ept_execute_only_supported(void);
 bool ept_ad_bits_supported(void);
 
 void enter_guest(void);
+void __enter_guest(u8);
 
 typedef void (*test_guest_func)(void);
 typedef void (*test_teardown_func)(void *data);
-- 
2.17.2

[PATCH 2/2][kvm-unit-test nVMX]: Check "load IA32_PAT" VM-entry control on vmentry of nested guests

[Krish Sadhukhan]

 ..to verify KVM performs the appropriate consistency checks for loading
IA32_PAT as part of running a nested guest.

According to section "Checking and Loading Guest State" in Intel SDM
vol 3C, the following check is performed on vmentry:

    If the "load IA32_PAT" VM-entry 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-).

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 140 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 121 insertions(+), 19 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 04b1aee..8186b4f 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4995,10 +4995,37 @@ static void test_sysenter_field(u32 field, const char *name)
 	vmcs_write(field, addr_saved);
 }
 
+static void guest_pat_main(void)
+{
+	while (1) {
+		if (vmx_get_test_stage() != 2)
+			vmcall();
+		else
+			break;
+	}
+
+	asm volatile("fnop");
+}
+
+static void report_guest_pat_test(const char *test, u32 xreason, u64 guest_pat)
+{
+	u32 reason = vmcs_read(EXI_REASON);
+	u64 guest_rip;
+	u32 insn_len;
+
+	report("%s, GUEST_PAT %lx", reason == xreason, test, guest_pat);
+
+	guest_rip = vmcs_read(GUEST_RIP);
+	insn_len = vmcs_read(EXI_INST_LEN);
+	if (! (reason & 0x80000021))
+		vmcs_write(GUEST_RIP, guest_rip + insn_len);
+}
+
 /*
- * 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.
+ * PAT values higher than 8 are uninteresting since they're likely lumped
+ * 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
 
@@ -5010,34 +5037,77 @@ static void test_pat(u32 fld, const char * fld_name, u32 ctrl_fld, u64 ctrl_bit)
 	u32 j;
 	int error;
 
-	vmcs_write(ctrl_fld, ctrl_saved & ~ctrl_bit);
-	for (i = 0; i <= PAT_VAL_LIMIT; i++) {
+	vmcs_clear_bits(ctrl_fld, ctrl_bit);
+	if (fld == GUEST_PAT) {
+		vmx_set_test_stage(1);
+		test_set_guest(guest_pat_main);
+	}
+
+	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(fld, val);
-			report_prefix_pushf("%s %lx", fld_name, val);
-			test_vmx_vmlaunch(0, false);
-			report_prefix_pop();
+			if (fld == HOST_PAT) {
+				report_prefix_pushf("%s %lx", fld_name, val);
+				test_vmx_vmlaunch(0, false);
+				report_prefix_pop();
+
+			} else {	// GUEST_PAT
+				__enter_guest(ABORT_ON_EARLY_VMENTRY_FAIL);
+				report_guest_pat_test("ENT_LOAD_PAT enabled",
+						       VMX_VMCALL, val);
+			}
 		}
 	}
 
-	vmcs_write(ctrl_fld, ctrl_saved | ctrl_bit);
-	for (i = 0; i <= PAT_VAL_LIMIT; i++) {
+	vmcs_set_bits(ctrl_fld, ctrl_bit);
+	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(fld, val);
-			report_prefix_pushf("%s %lx", fld_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();
+
+			if (fld == HOST_PAT) {
+				report_prefix_pushf("%s %lx", fld_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();
+
+			} else {	// GUEST_PAT
+				if (i == 0x2 || i == 0x3 || i == 0x8) {
+					__enter_guest(ABORT_ON_EARLY_VMENTRY_FAIL);
+					report_guest_pat_test("ENT_LOAD_PAT "
+								"enabled",
+							     VMX_FAIL_STATE |
+							     VMX_ENTRY_FAILURE,
+							     val);
+				} else {
+					__enter_guest(ABORT_ON_EARLY_VMENTRY_FAIL);
+					report_guest_pat_test("ENT_LOAD_PAT "
+							      "enabled",
+							      VMX_VMCALL,
+							      val);
+				}
+			}
+
 		}
 	}
 
+	if (fld == GUEST_PAT) {
+		/*
+		 * Let the guest finish execution
+		 */
+		vmx_set_test_stage(2);
+		vmcs_write(fld, pat_saved);
+		__enter_guest(ABORT_ON_EARLY_VMENTRY_FAIL);
+	}
+
 	vmcs_write(ctrl_fld, ctrl_saved);
 	vmcs_write(fld, pat_saved);
 }
@@ -5083,6 +5153,37 @@ static void vmx_host_state_area_test(void)
 	test_load_host_pat();
 }
 
+/*
+ *  If the "load IA32_PAT" VM-entry 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_guest_pat(void)
+{
+	/*
+	 * "load IA32_PAT" VM-entry control
+	 */
+	if (!(ctrl_exit_rev.clr & ENT_LOAD_PAT)) {
+		printf("\"Load-IA32-PAT\" entry control not supported\n");
+		return;
+	}
+
+	test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT);
+}
+
+/*
+ * Check that the virtual CPU checks the VMX Guest State Area as
+ * documented in the Intel SDM.
+ */
+static void vmx_guest_state_area_test(void)
+{
+	test_load_guest_pat();
+}
+
 static bool valid_vmcs_for_vmentry(void)
 {
 	struct vmcs *current_vmcs = NULL;
@@ -6505,6 +6606,7 @@ struct vmx_test vmx_tests[] = {
 	/* VM-entry tests */
 	TEST(vmx_controls_test),
 	TEST(vmx_host_state_area_test),
+	TEST(vmx_guest_state_area_test),
 	TEST(vmentry_movss_shadow_test),
 	/* APICv tests */
 	TEST(vmx_eoi_bitmap_ioapic_scan_test),
-- 
2.17.2

Re: [kvm-unit-test nVMX]: Test "load IA32_PAT" VM-entry control on vmentry of nested guests

[Krish Sadhukhan]

Ping...

On 4/18/19 2:39 PM, Krish Sadhukhan wrote:
> This is the unit test for the "load IA32_PAT" VM-entry control. Patch# 2
> builds on top of my previous patch,
>
>     [PATCH 6/6 v5][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests
>
>
> [PATCH 1/2][kvm-unit-test nVMX]: Move the functionality of enter_guest() to
> [PATCH 2/2][kvm-unit-test nVMX]: Check "load IA32_PAT" VM-entry control on vmentry
>
>   x86/vmx.c       |  27 +++++++----
>   x86/vmx.h       |   4 ++
>   x86/vmx_tests.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++--------
>   3 files changed, 143 insertions(+), 28 deletions(-)
>
> Krish Sadhukhan (2):
>        nVMX: Move the functionality of enter_guest() to __enter_guest() and make the former a wrapper of the latter
>        nVMX: Check "load IA32_PAT" VM-entry control on vmentry of nested guests
>

Re: [kvm-unit-test nVMX]: Test "load IA32_PAT" VM-entry control on vmentry of nested guests

[Paolo Bonzini]

On 08/05/19 19:43, Krish Sadhukhan wrote:
> Ping...

There have been some changes inthe meanwhile so the patches needed some
work to rebase.  I hope I haven't butchered them too much, please take a
look at the master branch. :)

Thanks,

Paolo

> On 4/18/19 2:39 PM, Krish Sadhukhan wrote:
>> This is the unit test for the "load IA32_PAT" VM-entry control. Patch# 2
>> builds on top of my previous patch,
>>
>>     [PATCH 6/6 v5][kvm-unit-test nVMX]: Check "load IA32_PAT" on
>> vmentry of L2 guests
>>
>>
>> [PATCH 1/2][kvm-unit-test nVMX]: Move the functionality of
>> enter_guest() to
>> [PATCH 2/2][kvm-unit-test nVMX]: Check "load IA32_PAT" VM-entry
>> control on vmentry
>>
>>   x86/vmx.c       |  27 +++++++----
>>   x86/vmx.h       |   4 ++
>>   x86/vmx_tests.c | 140
>> ++++++++++++++++++++++++++++++++++++++++++++++--------
>>   3 files changed, 143 insertions(+), 28 deletions(-)
>>
>> Krish Sadhukhan (2):
>>        nVMX: Move the functionality of enter_guest() to
>> __enter_guest() and make the former a wrapper of the latter
>>        nVMX: Check "load IA32_PAT" VM-entry control on vmentry of
>> nested guests
>>

Re: [kvm-unit-test nVMX]: Test "load IA32_PAT" VM-entry control on vmentry of nested guests

[Krish Sadhukhan]

On 05/20/2019 06:22 AM, Paolo Bonzini wrote:
> On 08/05/19 19:43, Krish Sadhukhan wrote:
>> Ping...
> There have been some changes inthe meanwhile so the patches needed some
> work to rebase.  I hope I haven't butchered them too much, please take a
> look at the master branch. :)

They look fine. Thanks !
It seems you have fixed two call sites in test_pat() by replacing 
__enter_guest(ABORT_ON_EARLY_VMENTRY_FAIL)   with
enter_guest().  There is still one call site we need to fix:  the first 
for-loop in test_pat(). There we should call enter_guest() instead of 
enter_guest_with_invalid_guest_state()  because there the guest state is 
valid as far as PAT is concerned and so we should abort on both an early 
vmentry failure as well as an invalid guest state.

Please let me know if you want to fix it or want me to send a patch.

>
> Thanks,
>
> Paolo
>
>> On 4/18/19 2:39 PM, Krish Sadhukhan wrote:
>>> This is the unit test for the "load IA32_PAT" VM-entry control. Patch# 2
>>> builds on top of my previous patch,
>>>
>>>      [PATCH 6/6 v5][kvm-unit-test nVMX]: Check "load IA32_PAT" on
>>> vmentry of L2 guests
>>>
>>>
>>> [PATCH 1/2][kvm-unit-test nVMX]: Move the functionality of
>>> enter_guest() to
>>> [PATCH 2/2][kvm-unit-test nVMX]: Check "load IA32_PAT" VM-entry
>>> control on vmentry
>>>
>>>    x86/vmx.c       |  27 +++++++----
>>>    x86/vmx.h       |   4 ++
>>>    x86/vmx_tests.c | 140
>>> ++++++++++++++++++++++++++++++++++++++++++++++--------
>>>    3 files changed, 143 insertions(+), 28 deletions(-)
>>>
>>> Krish Sadhukhan (2):
>>>         nVMX: Move the functionality of enter_guest() to
>>> __enter_guest() and make the former a wrapper of the latter
>>>         nVMX: Check "load IA32_PAT" VM-entry control on vmentry of
>>> nested guests
>>>