[KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v5)

[KVM nVMX]: Check "load IA32_PAT" VM-exit{entry} controls on vmentry of L2 guests (v5)

[Krish Sadhukhan]

v4 -> v5:
	1. Added back the missing code (call to
	   nested_check_guest_cregs_dregs_msr) in patch# 5.
	2. Updated the comment for PAT_VAL_LIMIT in patch# 6


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

 arch/x86/kvm/vmx/nested.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 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

[PATCH 1/6 v5][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry

[Krish Sadhukhan]

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..25aa4c4cf42a 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

[PATCH 2/6 v5][KVM nVMX]: Check "load IA32_PAT" VM-entry control on vmentry

[Krish Sadhukhan]

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

[PATCH 3/6 v5][KVM nVMX]: Move the checks for Guest Control Registers and Guest MSRs to a separate function

[Krish Sadhukhan]

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

[PATCH 4/6 v5][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions

[Krish Sadhukhan]

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

[PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Krish Sadhukhan]

 ..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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1ec5ddc4ea50..bde17d079a36 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2701,11 +2701,14 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 	*exit_qual = ENTRY_FAIL_DEFAULT;
 
 	if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12))
-		return 1;
+		return VMX_EXIT_REASONS_FAILED_VMENTRY |
+		       EXIT_REASON_INVALID_STATE;
 
 	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 +2727,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

[PATCH 6/6 v5][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 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 "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();
+		}
+	}
+
+	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

Re: [PATCH 4/6 v5][KVM nVMX]: nested_check_guest_cregs_dregs_msrs() should return -EINVAL for error conditions

[Sean Christopherson]

On Mon, Apr 08, 2019 at 05:35:14PM -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>

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Sean Christopherson]

On Mon, Apr 08, 2019 at 05:35:15PM -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>
> ---

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

Re: [PATCH 1/6 v5][KVM nVMX]: Check "load IA32_PAT" VM-exit control on vmentry

[Paolo Bonzini]

On 08/04/19 23:35, 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..25aa4c4cf42a 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,
> 

I've sent a patch to optimize the check using bitwise operations.  Apart
from that, looks good.

Paolo

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Paolo Bonzini]

On 08/04/19 23:35, 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 | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 1ec5ddc4ea50..bde17d079a36 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2701,11 +2701,14 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
>  	*exit_qual = ENTRY_FAIL_DEFAULT;
>  
>  	if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12))
> -		return 1;
> +		return VMX_EXIT_REASONS_FAILED_VMENTRY |
> +		       EXIT_REASON_INVALID_STATE;
>  
>  	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 +2727,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;
>  }
> 

This gives the reader a false impression that the return value is
actually reflected in the exit reason If anything I would change those
to -EINVAL, similar to what you did in patch 4 (but without applying
patch 3 which, as I understand it, is mostly a "trick" to make this
patch less verbose).

Paolo

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

[Paolo Bonzini]

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

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

On Wed, Apr 10, 2019 at 11:50:50AM +0200, Paolo Bonzini wrote:
> On 08/04/19 23:35, 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 | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 1ec5ddc4ea50..bde17d079a36 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2701,11 +2701,14 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
> >  	*exit_qual = ENTRY_FAIL_DEFAULT;
> >  
> >  	if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12))
> > -		return 1;
> > +		return VMX_EXIT_REASONS_FAILED_VMENTRY |
> > +		       EXIT_REASON_INVALID_STATE;
> >  
> >  	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 +2727,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;
> >  }
> > 
> 
> This gives the reader a false impression that the return value is
> actually reflected in the exit reason If anything I would change those
> to -EINVAL, similar to what you did in patch 4 (but without applying
> patch 3 which, as I understand it, is mostly a "trick" to make this
> patch less verbose).

Good point, though IMO it'd be better to go one step further and actually
consume the return value in nested_vmx_enter_non_root_mode().  For me,
having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice
mental reminder that "postreqs" is referring to checks that happen once
the CPU has "committed" to VM-Enter.

What about the attached patch as fixup?

[-- Attachment #2: 0001-KVM-nVMX-Fixup-nested_vmx_check_vmentry_postreqs-ret.patch --]
[-- Type: text/x-diff, Size: 2570 bytes --]

From 377aa4cc91aa2af140ade76f61c30040a77fd660 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@intel.com>
Date: Wed, 10 Apr 2019 09:03:16 -0700
Subject: [PATCH] KVM: nVMX: Fixup nested_vmx_check_vmentry_postreqs() return
 val handling

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 6bbb28ef313e..94405dcaccec 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2691,14 +2691,11 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 	*exit_qual = ENTRY_FAIL_DEFAULT;
 
 	if (nested_check_guest_cregs_dregs_msrs(vcpu, vmcs12))
-		return VMX_EXIT_REASONS_FAILED_VMENTRY |
-		       EXIT_REASON_INVALID_STATE;
+		return EXIT_REASON_INVALID_STATE;
 
 	if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
 		*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
-
-		return VMX_EXIT_REASONS_FAILED_VMENTRY |
-		       EXIT_REASON_INVALID_STATE;
+		return EXIT_REASON_INVALID_STATE;
 	}
 
 	/*
@@ -2717,17 +2714,13 @@ 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 VMX_EXIT_REASONS_FAILED_VMENTRY |
-			       EXIT_REASON_INVALID_STATE;
+			return 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 VMX_EXIT_REASONS_FAILED_VMENTRY |
-			       EXIT_REASON_INVALID_STATE;
+			return EXIT_REASON_INVALID_STATE;
 
 	return 0;
 }
@@ -2975,7 +2968,7 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 	bool evaluate_pending_interrupts;
-	u32 exit_reason = EXIT_REASON_INVALID_STATE;
+	u32 exit_reason;
 	u32 exit_qual;
 
 	evaluate_pending_interrupts = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL) &
@@ -3001,7 +2994,9 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 			return -1;
 		}
 
-		if (nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
+		exit_reason = nested_vmx_check_vmentry_postreqs(vcpu, vmcs12,
+								&exit_qual);
+		if (exit_reason)
 			goto vmentry_fail_vmexit;
 	}
 
-- 
2.21.0

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Paolo Bonzini]

On 10/04/19 18:08, Sean Christopherson wrote:
> Good point, though IMO it'd be better to go one step further and actually
> consume the return value in nested_vmx_enter_non_root_mode().  For me,
> having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice
> mental reminder that "postreqs" is referring to checks that happen once
> the CPU has "committed" to VM-Enter.

It's certainly better if you don't have to return
VMX_EXIT_REASONS_FAILED_VMENTRY.  However, I think it still complicates
things a bit, after all the result is always EXIT_REASON_INVALID_STATE.

The SDM says "VM-entry failure due to invalid guest state. A VM entry
failed one of the checks identified in Section 26.3.1" so the bool (or
0/-EINVAL) return code is a nice reminder that the function covers a
subset of 26.3.1.

Paolo

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

[Sean Christopherson]

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

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

[Paolo Bonzini]

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

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

[Sean Christopherson]

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!

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Sean Christopherson]

On Wed, Apr 10, 2019 at 07:05:30PM +0200, Paolo Bonzini wrote:
> On 10/04/19 18:08, Sean Christopherson wrote:
> > Good point, though IMO it'd be better to go one step further and actually
> > consume the return value in nested_vmx_enter_non_root_mode().  For me,
> > having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice
> > mental reminder that "postreqs" is referring to checks that happen once
> > the CPU has "committed" to VM-Enter.
> 
> It's certainly better if you don't have to return
> VMX_EXIT_REASONS_FAILED_VMENTRY.  However, I think it still complicates
> things a bit, after all the result is always EXIT_REASON_INVALID_STATE.
> 
> The SDM says "VM-entry failure due to invalid guest state. A VM entry
> failed one of the checks identified in Section 26.3.1" so the bool (or
> 0/-EINVAL) return code is a nice reminder that the function covers a
> subset of 26.3.1.

Heh, for me, returning EXIT_REASON_INVALID_STATE is the reminder that
the function covers 26.3.1.

What if we rename the function to nested_vmx_check_vmentry_guest_state()?
My desire to return the exit reason mostly stems from the name "postreqs"
since I tend to forget what "postreqs" is referring to.  And it'd be more
appropriate since the MSR load checks are handled elsewhere and really
should be considered "postreqs" as well.

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Krish Sadhukhan]

On 04/10/2019 10:55 AM, Sean Christopherson wrote:
> On Wed, Apr 10, 2019 at 07:05:30PM +0200, Paolo Bonzini wrote:
>> On 10/04/19 18:08, Sean Christopherson wrote:
>>> Good point, though IMO it'd be better to go one step further and actually
>>> consume the return value in nested_vmx_enter_non_root_mode().  For me,
>>> having the exit reason in nested_vmx_check_vmentry_postreqs() is a nice
>>> mental reminder that "postreqs" is referring to checks that happen once
>>> the CPU has "committed" to VM-Enter.
>> It's certainly better if you don't have to return
>> VMX_EXIT_REASONS_FAILED_VMENTRY.  However, I think it still complicates
>> things a bit, after all the result is always EXIT_REASON_INVALID_STATE.
>>
>> The SDM says "VM-entry failure due to invalid guest state. A VM entry
>> failed one of the checks identified in Section 26.3.1" so the bool (or
>> 0/-EINVAL) return code is a nice reminder that the function covers a
>> subset of 26.3.1.
> Heh, for me, returning EXIT_REASON_INVALID_STATE is the reminder that
> the function covers 26.3.1.

I think the cleaner approach would be to return -EINVAL in both 
*_pre{post}reqs functions and carry the exit code via a new function 
parameter. That will make the code more readable.

> What if we rename the function to nested_vmx_check_vmentry_guest_state()?
> My desire to return the exit reason mostly stems from the name "postreqs"
> since I tend to forget what "postreqs" is referring to.  And it'd be more
> appropriate since the MSR load checks are handled elsewhere and really
> should be considered "postreqs" as well.

May be we should also rename its counterpart 
(i.e.,nested_vmx_check_vmentry_prereqs) to something like 
nested_vmx_check_vmentry_pre_guest_state or something similar ?

The SDM doesn't explicitly mention whether MSR loading happens right 
after guest state is loaded, though it seems that's the order. If so, we 
should probably put these checks in function called 
nested_vmx_check_vmentry_msr_load or something like that.

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Paolo Bonzini]

On 11/04/19 02:15, Krish Sadhukhan wrote:
> May be we should also rename its counterpart
> (i.e.,nested_vmx_check_vmentry_prereqs) to something like
> nested_vmx_check_vmentry_pre_guest_state or something similar ?

I think we should have three functions
nested_vmx_vmentry_check_{controls,host_state,guest_state}.

Paolo

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Sean Christopherson]

On Thu, Apr 11, 2019 at 02:14:35PM +0200, Paolo Bonzini wrote:
> On 11/04/19 02:15, Krish Sadhukhan wrote:
> > May be we should also rename its counterpart
> > (i.e.,nested_vmx_check_vmentry_prereqs) to something like
> > nested_vmx_check_vmentry_pre_guest_state or something similar ?
> 
> I think we should have three functions
> nested_vmx_vmentry_check_{controls,host_state,guest_state}.

Four if you count nested_vmx_load_msr().  But anyways, I agree.

On a related subject, an invalid guest activity state results in a VM-Exit,
not VM-Fail, i.e. nested_check_guest_non_reg_state() belongs in
nested_vmx_check_vmentry_postreqs().  Moving that code to its proper
location would eliminate the last odditiy for renaming "postreqs" to
nested_vmx_vmentry_check_guest_state().

I'll send a series, including patches 1 and 2 (the PAT checks) from
Krish's series?

Re: [PATCH 5/6 v5][KVM nVMX]: nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions

[Krish Sadhukhan]

On 04/11/2019 09:29 AM, Sean Christopherson wrote:
> On Thu, Apr 11, 2019 at 02:14:35PM +0200, Paolo Bonzini wrote:
>> On 11/04/19 02:15, Krish Sadhukhan wrote:
>>> May be we should also rename its counterpart
>>> (i.e.,nested_vmx_check_vmentry_prereqs) to something like
>>> nested_vmx_check_vmentry_pre_guest_state or something similar ?
>> I think we should have three functions
>> nested_vmx_vmentry_check_{controls,host_state,guest_state}.
> Four if you count nested_vmx_load_msr().  But anyways, I agree.
>
> On a related subject, an invalid guest activity state results in a VM-Exit,
> not VM-Fail, i.e. nested_check_guest_non_reg_state() belongs in
> nested_vmx_check_vmentry_postreqs().

That's correct !

>   Moving that code to its proper
> location would eliminate the last odditiy for renaming "postreqs" to
> nested_vmx_vmentry_check_guest_state().
>
> I'll send a series, including patches 1 and 2 (the PAT checks) from
> Krish's series?

Sounds good. Thanks.