All of lore.kernel.org
 help / color / mirror / Atom feed
* [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4)
@ 2019-04-05  1:46 Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry Krish Sadhukhan
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

v3 -> v4:
	1. Two new patches, patch# 4 and 5, have been added. These patches
	   change the error return in nested_check_guest_cregs_dregs_msrs()
	   and nested_vmx_check_vmentry_postreqs().
	2. In patch# 6 (which was patch# 4 in v4), names of some of the
	   parameters and variables have been expanded.


[PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry
[PATCH 2/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-entry control on vmentry
[PATCH 3/6 v4][KVM nVMX]: Move the checks for Guest Control Registers and
[PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return
[PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return
[PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests

 arch/x86/kvm/vmx/nested.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

Krish Sadhukhan (5):
      nVMX: Check "load IA32_PAT" VM-exit control on vmentry
      nVMX: Check "load IA32_PAT" VM-entry control on vmentry
      nVMX: Move the checks for Guest Control Registers and Guest MSRs to a separate function
      nVMX: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions
      nVMX: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

 x86/vmx_tests.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

Krish Sadhukhan (1):
      nVMX: Check "load IA32_PAT" on vmentry of L2 guests


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry
  2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
@ 2019-04-05  1:46 ` Krish Sadhukhan
  2019-04-08 17:21   ` Sean Christopherson
  2019-04-05  1:46 ` [PATCH 2/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-entry " Krish Sadhukhan
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

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-).

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx/nested.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 3170e291215d..f601b156ca84 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
 	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
 		return -EINVAL;
+
+	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
+	    !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->host_ia32_pat))
+			return -EINVAL;
+
 	/*
 	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
 	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-entry control on vmentry
  2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry Krish Sadhukhan
@ 2019-04-05  1:46 ` Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 3/6 v4][KVM nVMX]: Move the checks for Guest Control Registers and Guest MSRs to a separate function Krish Sadhukhan
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

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>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f601b156ca84..b8bd449350b4 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2686,6 +2686,10 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 	    !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))
 		return 1;
 
+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
+	    !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->guest_ia32_pat))
+		return 1;
+
 	if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
 		*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
 		return 1;
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/6 v4][KVM nVMX]: Move the checks for Guest Control Registers and Guest MSRs to a separate function
  2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 2/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-entry " Krish Sadhukhan
@ 2019-04-05  1:46 ` Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions Krish Sadhukhan
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

 ..in order to align the checks to the order in which they are described in
Intel SDM vol 3C.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b8bd449350b4..8347e9066e26 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2674,6 +2674,24 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
 	return r;
 }
 
+/*
+ * Checks related to Control Registers, Debug Registers and MSRs in
+ * Guest State Area.
+ */
+static int nested_check_guest_cregs_dregs_msrs(struct kvm_vcpu *vcpu,
+					       struct vmcs12 *vmcs12)
+{
+	if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
+	    !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))
+		return 1;
+
+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
+	    !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->guest_ia32_pat))
+		return 1;
+
+	return 0;
+}
+
 static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 					     struct vmcs12 *vmcs12,
 					     u32 *exit_qual)
@@ -2682,12 +2700,7 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 
 	*exit_qual = ENTRY_FAIL_DEFAULT;
 
-	if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
-	    !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))
-		return 1;
-
-	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
-	    !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->guest_ia32_pat))
+	if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12))
 		return 1;
 
 	if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions
  2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
                   ` (2 preceding siblings ...)
  2019-04-05  1:46 ` [PATCH 3/6 v4][KVM nVMX]: Move the checks for Guest Control Registers and Guest MSRs to a separate function Krish Sadhukhan
@ 2019-04-05  1:46 ` Krish Sadhukhan
  2019-04-08 17:24   ` Sean Christopherson
  2019-04-05  1:46 ` [PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE " Krish Sadhukhan
  2019-04-05  1:46 ` [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests Krish Sadhukhan
  5 siblings, 1 reply; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

 ..to match the error return type of other similar functions.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8347e9066e26..efd226d4ea36 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2683,11 +2683,11 @@ static int nested_check_guest_cregs_dregs_msrs(struct kvm_vcpu *vcpu,
 {
 	if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
 	    !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))
-		return 1;
+		return -EINVAL;
 
 	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
 	    !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->guest_ia32_pat))
-		return 1;
+		return -EINVAL;
 
 	return 0;
 }
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions
  2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
                   ` (3 preceding siblings ...)
  2019-04-05  1:46 ` [PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions Krish Sadhukhan
@ 2019-04-05  1:46 ` Krish Sadhukhan
  2019-04-08 17:27   ` Sean Christopherson
  2019-04-05  1:46 ` [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests Krish Sadhukhan
  5 siblings, 1 reply; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

 ..to reflect the architectural Exit Reason for VM-entry failures due to
invalid guest state.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index efd226d4ea36..456bf6514bb3 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2705,7 +2705,9 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 
 	if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
 		*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
-		return 1;
+
+		return VMX_EXIT_REASONS_FAILED_VMENTRY |
+		       EXIT_REASON_INVALID_STATE;
 	}
 
 	/*
@@ -2724,13 +2726,17 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 		    ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) ||
 		    ((vmcs12->guest_cr0 & X86_CR0_PG) &&
 		     ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME)))
-			return 1;
+
+			return VMX_EXIT_REASONS_FAILED_VMENTRY |
+			       EXIT_REASON_INVALID_STATE;
 	}
 
 	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
 		(is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
 		(vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
-			return 1;
+
+			return VMX_EXIT_REASONS_FAILED_VMENTRY |
+			       EXIT_REASON_INVALID_STATE;
 
 	return 0;
 }
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests
  2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
                   ` (4 preceding siblings ...)
  2019-04-05  1:46 ` [PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE " Krish Sadhukhan
@ 2019-04-05  1:46 ` Krish Sadhukhan
  2019-04-08 17:34   ` Sean Christopherson
  5 siblings, 1 reply; 11+ messages in thread
From: Krish Sadhukhan @ 2019-04-05  1:46 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, jmattson

.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>
---
 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.
+ */
+#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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry
  2019-04-05  1:46 ` [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry Krish Sadhukhan
@ 2019-04-08 17:21   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:21 UTC (permalink / raw)
  To: Krish Sadhukhan; +Cc: kvm, pbonzini, rkrcmar, jmattson

On Thu, Apr 04, 2019 at 09:46:45PM -0400, Krish Sadhukhan wrote:
> 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-).
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 3170e291215d..f601b156ca84 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>  	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
>  	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
>  		return -EINVAL;
> +
> +	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
> +	    !kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, vmcs12->host_ia32_pat))
> +			return -EINVAL;

Doh, "return -EINVAL" is indented too far, only noticed it now because
this conflicts with the latest kvm/queue.  Paolo/Radim can likely fix it
up while applying since they'll need to deal with the conflict anyways.

> +
>  	/*
>  	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
>  	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
> -- 
> 2.17.2
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions
  2019-04-05  1:46 ` [PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions Krish Sadhukhan
@ 2019-04-08 17:24   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:24 UTC (permalink / raw)
  To: Krish Sadhukhan; +Cc: kvm, pbonzini, rkrcmar, jmattson

On Thu, Apr 04, 2019 at 09:46:48PM -0400, Krish Sadhukhan wrote:
>  ..to match the error return type of other similar functions.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions
  2019-04-05  1:46 ` [PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE " Krish Sadhukhan
@ 2019-04-08 17:27   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:27 UTC (permalink / raw)
  To: Krish Sadhukhan; +Cc: kvm, pbonzini, rkrcmar, jmattson

On Thu, Apr 04, 2019 at 09:46:49PM -0400, Krish Sadhukhan wrote:
>  ..to reflect the architectural Exit Reason for VM-entry failures due to
> invalid guest state.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index efd226d4ea36..456bf6514bb3 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2705,7 +2705,9 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
>  

You missed the newly added call to nested_check_guest_cregs_dregs_msrs().
Assuming you spin v5, please fix the indentation issue in patch 1/6 as well.

>  	if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
>  		*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
> -		return 1;
> +
> +		return VMX_EXIT_REASONS_FAILED_VMENTRY |
> +		       EXIT_REASON_INVALID_STATE;
>  	}
>  
>  	/*
> @@ -2724,13 +2726,17 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
>  		    ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA) ||
>  		    ((vmcs12->guest_cr0 & X86_CR0_PG) &&
>  		     ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME)))
> -			return 1;
> +
> +			return VMX_EXIT_REASONS_FAILED_VMENTRY |
> +			       EXIT_REASON_INVALID_STATE;
>  	}
>  
>  	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
>  		(is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
>  		(vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
> -			return 1;
> +
> +			return VMX_EXIT_REASONS_FAILED_VMENTRY |
> +			       EXIT_REASON_INVALID_STATE;
>  
>  	return 0;
>  }
> -- 
> 2.17.2
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests
  2019-04-05  1:46 ` [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests Krish Sadhukhan
@ 2019-04-08 17:34   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2019-04-08 17:34 UTC (permalink / raw)
  To: Krish Sadhukhan; +Cc: kvm, pbonzini, rkrcmar, jmattson

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-04-08 17:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05  1:46 [KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v4) Krish Sadhukhan
2019-04-05  1:46 ` [PATCH 1/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry Krish Sadhukhan
2019-04-08 17:21   ` Sean Christopherson
2019-04-05  1:46 ` [PATCH 2/6 v4][KVM nVMX]: Check "load IA32_PAT" VM-entry " Krish Sadhukhan
2019-04-05  1:46 ` [PATCH 3/6 v4][KVM nVMX]: Move the checks for Guest Control Registers and Guest MSRs to a separate function Krish Sadhukhan
2019-04-05  1:46 ` [PATCH 4/6 v4][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions Krish Sadhukhan
2019-04-08 17:24   ` Sean Christopherson
2019-04-05  1:46 ` [PATCH 5/6 v4][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE " Krish Sadhukhan
2019-04-08 17:27   ` Sean Christopherson
2019-04-05  1:46 ` [PATCH 6/6 v4][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests Krish Sadhukhan
2019-04-08 17:34   ` Sean Christopherson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.