[PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks

[PATCH v6 0/7] KVM: nVMX Add IA32_PAT consistency checks

[Sean Christopherson]

This is a continuation of Krish's series, which has suffered a teeny
tiny bit of scope creep (sorry Krish).  This is essentially a new series,
e.g. only the actual PAT-related patches, which haven't changed since v2,
are retained from previous versions.  But, to avoid unnecessary conflicts
I'm resending everything in one happy series.

The direction change relative to v5 is to self-document the top-level
consistency check functions by a) renaming the functions and b) using a
dedicated parameter to "return" the failure information, as suggested by
Paolo.  Previous versions had been taking the approach of using the return
value itself to document that a failed "postreqs" leads to a VM-Exit,
which was simply putting lipstick on a pig.

Krish Sadhukhan (2):
  Check "load IA32_PAT" VM-exit control on vmentry
  Check "load IA32_PAT" VM-entry control on vmentry

Sean Christopherson (5):
  KVM: nVMX: Move guest non-reg state checks to VM-Exit path
  KVM: nVMX: Rename and split top-level consistency checks to match SDM
  KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val
  KVM: nVMX: Collapse nested_check_host_control_regs() into its caller
  KVM: nVMX: Return -EINVAL when signaling failure in VM-Entry helpers

 arch/x86/kvm/vmx/nested.c | 150 +++++++++++++++++++++-----------------
 1 file changed, 82 insertions(+), 68 deletions(-)

-- 
2.21.0

[PATCH v6 1/7] Check "load IA32_PAT" VM-exit control on vmentry

[Sean Christopherson]

From: Krish Sadhukhan <krish.sadhukhan@oracle.com>

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>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.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 153e539c29c9..5133e0cf6eb9 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2590,6 +2590,10 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	    is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
 		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.21.0

[PATCH v6 2/7] Check "load IA32_PAT" VM-entry control on vmentry

[Sean Christopherson]

From: Krish Sadhukhan <krish.sadhukhan@oracle.com>

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>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.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 5133e0cf6eb9..f487aae3233e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2680,6 +2680,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.21.0

[PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path

[Sean Christopherson]

Per Intel's SDM, volume 3, section Checking and Loading Guest State:

  Because the checking and the loading occur concurrently, a failure may
  be discovered only after some state has been loaded. For this reason,
  the logical processor responds to such failures by loading state from
  the host-state area, as it would for a VM exit.

In other words, a failed non-register state consistency check results in
a VM-Exit, not VM-Fail.  Moving the non-reg state checks also paves the
way for renaming nested_vmx_check_vmentry_postreqs() to align with the
SDM, i.e. nested_vmx_check_vmentry_guest_state().

Fixes: 26539bd0e446a ("KVM: nVMX: check vmcs12 for valid activity state")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f487aae3233e..fe1323ab6894 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2612,18 +2612,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-/*
- * Checks related to Guest Non-register State
- */
-static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
-{
-	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
-	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
-		return -EINVAL;
-
-	return 0;
-}
-
 static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
 					    struct vmcs12 *vmcs12)
 {
@@ -2635,9 +2623,6 @@ static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
 	if (nested_check_host_control_regs(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
 
-	if (nested_check_guest_non_reg_state(vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
 	return 0;
 }
 
@@ -2668,6 +2653,18 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
 	return r;
 }
 
+/*
+ * Checks related to Guest Non-register State
+ */
+static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
+{
+	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
+	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 					     struct vmcs12 *vmcs12,
 					     u32 *exit_qual)
@@ -2713,6 +2710,9 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 		(vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
 			return 1;
 
+	if (nested_check_guest_non_reg_state(vmcs12))
+		return 1;
+
 	return 0;
 }
 
-- 
2.21.0

Re: [PATCH v6 3/7] KVM: nVMX: Move guest non-reg state checks to VM-Exit path

[Krish Sadhukhan]

On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> Per Intel's SDM, volume 3, section Checking and Loading Guest State:
>
>    Because the checking and the loading occur concurrently, a failure may
>    be discovered only after some state has been loaded. For this reason,
>    the logical processor responds to such failures by loading state from
>    the host-state area, as it would for a VM exit.
>
> In other words, a failed non-register state consistency check results in
> a VM-Exit, not VM-Fail.  Moving the non-reg state checks also paves the
> way for renaming nested_vmx_check_vmentry_postreqs() to align with the
> SDM, i.e. nested_vmx_check_vmentry_guest_state().
>
> Fixes: 26539bd0e446a ("KVM: nVMX: check vmcs12 for valid activity state")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f487aae3233e..fe1323ab6894 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2612,18 +2612,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> -/*
> - * Checks related to Guest Non-register State
> - */
> -static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
> -{
> -	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> -	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>   static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
>   					    struct vmcs12 *vmcs12)
>   {
> @@ -2635,9 +2623,6 @@ static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
>   	if (nested_check_host_control_regs(vcpu, vmcs12))
>   		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
>   
> -	if (nested_check_guest_non_reg_state(vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
>   	return 0;
>   }
>   
> @@ -2668,6 +2653,18 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
>   	return r;
>   }
>   
> +/*
> + * Checks related to Guest Non-register State
> + */
> +static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
> +{
> +	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> +	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
>   					     struct vmcs12 *vmcs12,
>   					     u32 *exit_qual)
> @@ -2713,6 +2710,9 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
>   		(vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
>   			return 1;
>   
> +	if (nested_check_guest_non_reg_state(vmcs12))
> +		return 1;
> +
>   	return 0;
>   }
>   

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

[PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM

[Sean Christopherson]

Rename the top-level consistency check functions to (loosely) align with
the SDM.  Historically, KVM has used the terms "prereq" and "postreq" to
differentiate between consistency checks that lead to VM-Fail and those
that lead to VM-Exit.  The terms are vague and potentially misleading,
e.g. "postreq" might be interpreted as occurring after VM-Entry.

Note, while the SDM lumps controls and host state into a single section,
"Checks on VMX Controls and Host-State Area", split them into separate
top-level functions as the two categories of checks result in different
VM instruction errors.  This split will allow for additional cleanup.

Note #2, "vmentry" is intentionally dropped from the new function names
to avoid confusion with nested_check_vm_entry_controls(), and to keep
the length of the functions names somewhat manageable.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index fe1323ab6894..b22605d5ee9e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2573,6 +2573,17 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
+				     struct vmcs12 *vmcs12)
+{
+	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
+	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
+	    nested_check_vm_entry_controls(vcpu, vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
+	return 0;
+}
+
 /*
  * Checks related to Host Control Registers and MSRs
  */
@@ -2612,14 +2623,9 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
-					    struct vmcs12 *vmcs12)
+static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
 {
-	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
-	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
-	    nested_check_vm_entry_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
-
 	if (nested_check_host_control_regs(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
 
@@ -2665,9 +2671,9 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
 	return 0;
 }
 
-static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
-					     struct vmcs12 *vmcs12,
-					     u32 *exit_qual)
+static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
+					struct vmcs12 *vmcs12,
+					u32 *exit_qual)
 {
 	bool ia32e;
 
@@ -2985,7 +2991,7 @@ 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))
+		if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
 			goto vmentry_fail_vmexit;
 	}
 
@@ -3130,7 +3136,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
 			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
 
-	ret = nested_vmx_check_vmentry_prereqs(vcpu, vmcs12);
+	ret = nested_vmx_check_controls(vcpu, vmcs12);
+	if (ret)
+		return nested_vmx_failValid(vcpu, ret);
+
+	ret = nested_vmx_check_host_state(vcpu, vmcs12);
 	if (ret)
 		return nested_vmx_failValid(vcpu, ret);
 
@@ -5455,8 +5465,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 			return -EINVAL;
 	}
 
-	if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) ||
-	    nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
+	if (nested_vmx_check_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_host_state(vcpu, vmcs12) ||
+	    nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
 		return -EINVAL;
 
 	vmx->nested.dirty_vmcs12 = true;
-- 
2.21.0

Re: [PATCH v6 4/7] KVM: nVMX: Rename and split top-level consistency checks to match SDM

[Krish Sadhukhan]

On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> Rename the top-level consistency check functions to (loosely) align with
> the SDM.  Historically, KVM has used the terms "prereq" and "postreq" to
> differentiate between consistency checks that lead to VM-Fail and those
> that lead to VM-Exit.  The terms are vague and potentially misleading,
> e.g. "postreq" might be interpreted as occurring after VM-Entry.
>
> Note, while the SDM lumps controls and host state into a single section,
> "Checks on VMX Controls and Host-State Area", split them into separate
> top-level functions as the two categories of checks result in different
> VM instruction errors.  This split will allow for additional cleanup.
>
> Note #2, "vmentry" is intentionally dropped from the new function names
> to avoid confusion with nested_check_vm_entry_controls(), and to keep
> the length of the functions names somewhat manageable.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 39 +++++++++++++++++++++++++--------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index fe1323ab6894..b22605d5ee9e 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2573,6 +2573,17 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> +static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
> +				     struct vmcs12 *vmcs12)
> +{
> +	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
> +	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
> +	    nested_check_vm_entry_controls(vcpu, vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
> +	return 0;
> +}
> +
>   /*
>    * Checks related to Host Control Registers and MSRs
>    */
> @@ -2612,14 +2623,9 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> -static int nested_vmx_check_vmentry_prereqs(struct kvm_vcpu *vcpu,
> -					    struct vmcs12 *vmcs12)
> +static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> +				       struct vmcs12 *vmcs12)
>   {
> -	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
> -	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
> -	    nested_check_vm_entry_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> -
>   	if (nested_check_host_control_regs(vcpu, vmcs12))
>   		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
>   
> @@ -2665,9 +2671,9 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>   	return 0;
>   }
>   
> -static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
> -					     struct vmcs12 *vmcs12,
> -					     u32 *exit_qual)
> +static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> +					struct vmcs12 *vmcs12,
> +					u32 *exit_qual)
>   {
>   	bool ia32e;
>   
> @@ -2985,7 +2991,7 @@ 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))
> +		if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
>   			goto vmentry_fail_vmexit;
>   	}
>   
> @@ -3130,7 +3136,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>   			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
>   			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
>   
> -	ret = nested_vmx_check_vmentry_prereqs(vcpu, vmcs12);
> +	ret = nested_vmx_check_controls(vcpu, vmcs12);
> +	if (ret)
> +		return nested_vmx_failValid(vcpu, ret);
> +
> +	ret = nested_vmx_check_host_state(vcpu, vmcs12);
>   	if (ret)
>   		return nested_vmx_failValid(vcpu, ret);
>   
> @@ -5455,8 +5465,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>   			return -EINVAL;
>   	}
>   
> -	if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) ||
> -	    nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> +	if (nested_vmx_check_controls(vcpu, vmcs12) ||
> +	    nested_vmx_check_host_state(vcpu, vmcs12) ||
> +	    nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
>   		return -EINVAL;
>   
>   	vmx->nested.dirty_vmcs12 = true;

It's probably a good idea to add a one-line comment above each of the 
top-level check functions.

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

[PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val

[Sean Christopherson]

Convert all top-level nested VM-Enter consistency check functions to
use explicit parameters to pass failure information to the caller.
Using an explicit parameter achieves several goals:

  - Provides consistent prototypes for all functions.
  - Self-documents the net effect of failure, e.g. without the explicit
    parameter it may not be obvious that nested_vmx_check_guest_state()
    leads to a VM-Exit.
  - Does not give the false impression that failure information is
    always consumed and/or relevant, e.g. vmx_set_nested_state() only
    cares whether or not the checks were successful.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 61 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b22605d5ee9e..16cff40456ee 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -817,7 +817,8 @@ static int nested_vmx_store_msr_check(struct kvm_vcpu *vcpu,
  * Load guest's/host's msr at nested entry/exit.
  * return 0 for success, entry index for failure.
  */
-static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
+static int nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count,
+			       u32 *exit_reason, u32 *exit_qual)
 {
 	u32 i;
 	struct vmx_msr_entry e;
@@ -849,7 +850,9 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
 	}
 	return 0;
 fail:
-	return i + 1;
+	*exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
+	*exit_qual = i + 1;
+	return -EINVAL;
 }
 
 static int nested_vmx_store_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
@@ -2574,12 +2577,15 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
 }
 
 static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
-				     struct vmcs12 *vmcs12)
+				     struct vmcs12 *vmcs12,
+				     u32 *vm_instruction_error)
 {
+	*vm_instruction_error = VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
 	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
 	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
 	    nested_check_vm_entry_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+		return -EINVAL;
 
 	return 0;
 }
@@ -2624,10 +2630,13 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 }
 
 static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
-				       struct vmcs12 *vmcs12)
+				       struct vmcs12 *vmcs12,
+				       u32 *vm_instruction_error)
 {
+	*vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
+
 	if (nested_check_host_control_regs(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
+		return -EINVAL;
 
 	return 0;
 }
@@ -2673,10 +2682,12 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
 
 static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 					struct vmcs12 *vmcs12,
+					u32 *exit_reason,
 					u32 *exit_qual)
 {
 	bool ia32e;
 
+	*exit_reason = EXIT_REASON_INVALID_STATE;
 	*exit_qual = ENTRY_FAIL_DEFAULT;
 
 	if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
@@ -2965,7 +2976,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) &
@@ -2991,7 +3002,8 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 			return -1;
 		}
 
-		if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
+		if (nested_vmx_check_guest_state(vcpu, vmcs12,
+						 &exit_reason, &exit_qual))
 			goto vmentry_fail_vmexit;
 	}
 
@@ -3003,11 +3015,9 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
 		goto vmentry_fail_vmexit_guest_mode;
 
 	if (from_vmentry) {
-		exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
-		exit_qual = nested_vmx_load_msr(vcpu,
-						vmcs12->vm_entry_msr_load_addr,
-						vmcs12->vm_entry_msr_load_count);
-		if (exit_qual)
+		if (nested_vmx_load_msr(vcpu, vmcs12->vm_entry_msr_load_addr,
+					vmcs12->vm_entry_msr_load_count,
+					&exit_reason, &exit_qual))
 			goto vmentry_fail_vmexit_guest_mode;
 	} else {
 		/*
@@ -3087,6 +3097,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 	struct vmcs12 *vmcs12;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
+	u32 vm_instruction_error;
 	int ret;
 
 	if (!nested_vmx_check_permission(vcpu))
@@ -3136,13 +3147,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
 			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
 
-	ret = nested_vmx_check_controls(vcpu, vmcs12);
-	if (ret)
-		return nested_vmx_failValid(vcpu, ret);
+	if (nested_vmx_check_controls(vcpu, vmcs12, &vm_instruction_error))
+		return nested_vmx_failValid(vcpu, vm_instruction_error);
 
-	ret = nested_vmx_check_host_state(vcpu, vmcs12);
-	if (ret)
-		return nested_vmx_failValid(vcpu, ret);
+	if (nested_vmx_check_host_state(vcpu, vmcs12, &vm_instruction_error))
+		return nested_vmx_failValid(vcpu, vm_instruction_error);
 
 	/*
 	 * We're finally done with prerequisite checking, and can start with
@@ -3594,7 +3603,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 				   struct vmcs12 *vmcs12)
 {
 	struct kvm_segment seg;
-	u32 entry_failure_code;
+	u32 ign;
 
 	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
 		vcpu->arch.efer = vmcs12->host_ia32_efer;
@@ -3629,7 +3638,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 	 * Only PDPTE load can fail as the value of cr3 was checked on entry and
 	 * couldn't have changed.
 	 */
-	if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &entry_failure_code))
+	if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &ign))
 		nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
 
 	if (!enable_ept)
@@ -3727,7 +3736,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 		vmx_update_msr_bitmap(vcpu);
 
 	if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr,
-				vmcs12->vm_exit_msr_load_count))
+				vmcs12->vm_exit_msr_load_count, &ign, &ign))
 		nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL);
 }
 
@@ -5352,7 +5361,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct vmcs12 *vmcs12;
-	u32 exit_qual;
+	u32 ign;
 	int ret;
 
 	if (kvm_state->format != 0)
@@ -5465,9 +5474,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 			return -EINVAL;
 	}
 
-	if (nested_vmx_check_controls(vcpu, vmcs12) ||
-	    nested_vmx_check_host_state(vcpu, vmcs12) ||
-	    nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
+	if (nested_vmx_check_controls(vcpu, vmcs12, &ign) ||
+	    nested_vmx_check_host_state(vcpu, vmcs12, &ign) ||
+	    nested_vmx_check_guest_state(vcpu, vmcs12, &ign, &ign))
 		return -EINVAL;
 
 	vmx->nested.dirty_vmcs12 = true;
-- 
2.21.0

Re: [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val

[Krish Sadhukhan]

On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> Convert all top-level nested VM-Enter consistency check functions to
> use explicit parameters to pass failure information to the caller.
> Using an explicit parameter achieves several goals:
>
>    - Provides consistent prototypes for all functions.
>    - Self-documents the net effect of failure, e.g. without the explicit
>      parameter it may not be obvious that nested_vmx_check_guest_state()
>      leads to a VM-Exit.
>    - Does not give the false impression that failure information is
>      always consumed and/or relevant, e.g. vmx_set_nested_state() only
>      cares whether or not the checks were successful.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 61 ++++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b22605d5ee9e..16cff40456ee 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -817,7 +817,8 @@ static int nested_vmx_store_msr_check(struct kvm_vcpu *vcpu,
>    * Load guest's/host's msr at nested entry/exit.
>    * return 0 for success, entry index for failure.
>    */
> -static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
> +static int nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count,
> +			       u32 *exit_reason, u32 *exit_qual)
>   {
>   	u32 i;
>   	struct vmx_msr_entry e;
> @@ -849,7 +850,9 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
>   	}
>   	return 0;
>   fail:
> -	return i + 1;
> +	*exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> +	*exit_qual = i + 1;
> +	return -EINVAL;
>   }
>   
>   static int nested_vmx_store_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
> @@ -2574,12 +2577,15 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>   }
>   
>   static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
> -				     struct vmcs12 *vmcs12)
> +				     struct vmcs12 *vmcs12,
> +				     u32 *vm_instruction_error)
>   {
> +	*vm_instruction_error = VMXERR_ENTRY_INVALID_CONTROL_FIELD;

Although, all of the callee functions generate the same error code, 
isn't it cleaner to set the error in the callee themselves where the 
actual checking is done ?

> +
>   	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
>   	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
>   	    nested_check_vm_entry_controls(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +		return -EINVAL;
>   
>   	return 0;
>   }
> @@ -2624,10 +2630,13 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>   }
>   
>   static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> -				       struct vmcs12 *vmcs12)
> +				       struct vmcs12 *vmcs12,
> +				       u32 *vm_instruction_error)
>   {
> +	*vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> +
>   	if (nested_check_host_control_regs(vcpu, vmcs12))
> -		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> +		return -EINVAL;
>   
>   	return 0;
>   }
> @@ -2673,10 +2682,12 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>   
>   static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>   					struct vmcs12 *vmcs12,
> +					u32 *exit_reason,
>   					u32 *exit_qual)
>   {
>   	bool ia32e;
>   
> +	*exit_reason = EXIT_REASON_INVALID_STATE;

Shouldn't we also reflect VMX_EXIT_REASONS_FAILED_VMENTRY in 'exit_reason' ?

>   	*exit_qual = ENTRY_FAIL_DEFAULT;
>   
>   	if (!nested_guest_cr0_valid(vcpu, vmcs12->guest_cr0) ||
> @@ -2965,7 +2976,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) &
> @@ -2991,7 +3002,8 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
>   			return -1;
>   		}
>   
> -		if (nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
> +		if (nested_vmx_check_guest_state(vcpu, vmcs12,
> +						 &exit_reason, &exit_qual))
>   			goto vmentry_fail_vmexit;
>   	}
>   
> @@ -3003,11 +3015,9 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
>   		goto vmentry_fail_vmexit_guest_mode;
>   
>   	if (from_vmentry) {
> -		exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> -		exit_qual = nested_vmx_load_msr(vcpu,
> -						vmcs12->vm_entry_msr_load_addr,
> -						vmcs12->vm_entry_msr_load_count);
> -		if (exit_qual)
> +		if (nested_vmx_load_msr(vcpu, vmcs12->vm_entry_msr_load_addr,
> +					vmcs12->vm_entry_msr_load_count,
> +					&exit_reason, &exit_qual))
>   			goto vmentry_fail_vmexit_guest_mode;
>   	} else {
>   		/*
> @@ -3087,6 +3097,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>   	struct vmcs12 *vmcs12;
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>   	u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
> +	u32 vm_instruction_error;
>   	int ret;
>   
>   	if (!nested_vmx_check_permission(vcpu))
> @@ -3136,13 +3147,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>   			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
>   			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
>   
> -	ret = nested_vmx_check_controls(vcpu, vmcs12);
> -	if (ret)
> -		return nested_vmx_failValid(vcpu, ret);
> +	if (nested_vmx_check_controls(vcpu, vmcs12, &vm_instruction_error))
> +		return nested_vmx_failValid(vcpu, vm_instruction_error);
>   
> -	ret = nested_vmx_check_host_state(vcpu, vmcs12);
> -	if (ret)
> -		return nested_vmx_failValid(vcpu, ret);
> +	if (nested_vmx_check_host_state(vcpu, vmcs12, &vm_instruction_error))
> +		return nested_vmx_failValid(vcpu, vm_instruction_error);
>   
>   	/*
>   	 * We're finally done with prerequisite checking, and can start with
> @@ -3594,7 +3603,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>   				   struct vmcs12 *vmcs12)
>   {
>   	struct kvm_segment seg;
> -	u32 entry_failure_code;
> +	u32 ign;
>   
>   	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
>   		vcpu->arch.efer = vmcs12->host_ia32_efer;
> @@ -3629,7 +3638,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>   	 * Only PDPTE load can fail as the value of cr3 was checked on entry and
>   	 * couldn't have changed.
>   	 */
> -	if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &entry_failure_code))
> +	if (nested_vmx_load_cr3(vcpu, vmcs12->host_cr3, false, &ign))
>   		nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_PDPTE_FAIL);
>   
>   	if (!enable_ept)
> @@ -3727,7 +3736,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>   		vmx_update_msr_bitmap(vcpu);
>   
>   	if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr,
> -				vmcs12->vm_exit_msr_load_count))
> +				vmcs12->vm_exit_msr_load_count, &ign, &ign))
>   		nested_vmx_abort(vcpu, VMX_ABORT_LOAD_HOST_MSR_FAIL);
>   }
>   
> @@ -5352,7 +5361,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>   {
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>   	struct vmcs12 *vmcs12;
> -	u32 exit_qual;
> +	u32 ign;
>   	int ret;
>   
>   	if (kvm_state->format != 0)
> @@ -5465,9 +5474,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>   			return -EINVAL;
>   	}
>   
> -	if (nested_vmx_check_controls(vcpu, vmcs12) ||
> -	    nested_vmx_check_host_state(vcpu, vmcs12) ||
> -	    nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
> +	if (nested_vmx_check_controls(vcpu, vmcs12, &ign) ||
> +	    nested_vmx_check_host_state(vcpu, vmcs12, &ign) ||
> +	    nested_vmx_check_guest_state(vcpu, vmcs12, &ign, &ign))
>   		return -EINVAL;
>   
>   	vmx->nested.dirty_vmcs12 = true;

Re: [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val

[Paolo Bonzini]

On 11/04/19 21:18, Sean Christopherson wrote:
> Convert all top-level nested VM-Enter consistency check functions to
> use explicit parameters to pass failure information to the caller.
> Using an explicit parameter achieves several goals:
> 
>   - Provides consistent prototypes for all functions.
>   - Self-documents the net effect of failure, e.g. without the explicit
>     parameter it may not be obvious that nested_vmx_check_guest_state()
>     leads to a VM-Exit.
>   - Does not give the false impression that failure information is
>     always consumed and/or relevant, e.g. vmx_set_nested_state() only
>     cares whether or not the checks were successful.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---

I think we have to agree to disagree on this one. :)

I'd rather have something like this (untested) replacing patches 5 and 6:

-------------- 8< -----------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] KVM: nVMX: Return -EINVAL when signaling failure in pre-VM-Entry helpers
    
Convert all top-level nested VM-Enter consistency check functions to
return 0/-EINVAL instead of failure codes, since now they can only
ever return one failure code.

This also does not give the false impression that failure information is
always consumed and/or relevant, e.g. vmx_set_nested_state() only
cares whether or not the checks were successful.

nested_check_host_control_regs() can also now be inlined into its caller,
nested_vmx_check_host_state, since the two have effectively become the
same function.

Based on a patch by Sean Christopherson.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f58de538e6d3..c75edccdd3bc 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2595,16 +2595,13 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
 	if (nested_check_vm_execution_controls(vcpu, vmcs12) ||
 	    nested_check_vm_exit_controls(vcpu, vmcs12) ||
 	    nested_check_vm_entry_controls(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+		return -EINVAL;
 
 	return 0;
 }
 
-/*
- * Checks related to Host Control Registers and MSRs
- */
-static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
-                                          struct vmcs12 *vmcs12)
+static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
 {
 	bool ia32e;
 
@@ -2639,15 +2636,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
-				       struct vmcs12 *vmcs12)
-{
-	if (nested_check_host_control_regs(vcpu, vmcs12))
-		return VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
-
-	return 0;
-}
-
 static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
 					  struct vmcs12 *vmcs12)
 {
@@ -3152,13 +3140,11 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 			launch ? VMXERR_VMLAUNCH_NONCLEAR_VMCS
 			       : VMXERR_VMRESUME_NONLAUNCHED_VMCS);
 
-	ret = nested_vmx_check_controls(vcpu, vmcs12);
-	if (ret)
-		return nested_vmx_failValid(vcpu, ret);
+	if (nested_vmx_check_controls(vcpu, vmcs12))
+		return nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD);
 
-	ret = nested_vmx_check_host_state(vcpu, vmcs12);
-	if (ret)
-		return nested_vmx_failValid(vcpu, ret);
+	if (nested_vmx_check_host_state(vcpu, vmcs12))
+		return nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
 
 	/*
 	 * We're finally done with prerequisite checking, and can start with

Re: [PATCH v6 5/7] KVM: nVMX: Set VM-{Fail,Exit} failure info via params, not return val

[Sean Christopherson]

On Fri, Apr 12, 2019 at 10:30:51AM +0200, Paolo Bonzini wrote:
> On 11/04/19 21:18, Sean Christopherson wrote:
> > Convert all top-level nested VM-Enter consistency check functions to
> > use explicit parameters to pass failure information to the caller.
> > Using an explicit parameter achieves several goals:
> > 
> >   - Provides consistent prototypes for all functions.
> >   - Self-documents the net effect of failure, e.g. without the explicit
> >     parameter it may not be obvious that nested_vmx_check_guest_state()
> >     leads to a VM-Exit.
> >   - Does not give the false impression that failure information is
> >     always consumed and/or relevant, e.g. vmx_set_nested_state() only
> >     cares whether or not the checks were successful.
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> 
> I think we have to agree to disagree on this one. :)

Aaargh!  The dreaded Maintainer Hammer of Doom!  LGTM ;-)

[PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller

[Sean Christopherson]

...now that nested_vmx_check_host_state() returns -EINVAL to indicate
failure, i.e. moving the checks doesn't require changing their return
values to be 'VMXERR_ENTRY_INVALID_HOST_STATE_FIELD'.  This eliminates
a misleading function name since nested_check_host_control_regs() was
checking MSRs as well as control registers.

Alternatively, nested_check_host_control_regs() could be split up,
e.g. into _control_regs(), _debug_regs() and _msrs(), but that's a
bit gratutitous at this time given that the entire function is 30 LoC.

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

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 16cff40456ee..98afbe7c15a1 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2590,13 +2590,11 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-/*
- * Checks related to Host Control Registers and MSRs
- */
-static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
-                                          struct vmcs12 *vmcs12)
+static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12,
+				       u32 *vm_instruction_error)
 {
-	bool ia32e;
+	*vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
 
 	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
 	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
@@ -2618,8 +2616,8 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	 * the host address-space size VM-exit control.
 	 */
 	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER) {
-		ia32e = (vmcs12->vm_exit_controls &
-			 VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
+		bool ia32e = (vmcs12->vm_exit_controls &
+			      VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
 		if (!kvm_valid_efer(vcpu, vmcs12->host_ia32_efer) ||
 		    ia32e != !!(vmcs12->host_ia32_efer & EFER_LMA) ||
 		    ia32e != !!(vmcs12->host_ia32_efer & EFER_LME))
@@ -2629,18 +2627,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
-				       struct vmcs12 *vmcs12,
-				       u32 *vm_instruction_error)
-{
-	*vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
-
-	if (nested_check_host_control_regs(vcpu, vmcs12))
-		return -EINVAL;
-
-	return 0;
-}
-
 static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
 					  struct vmcs12 *vmcs12)
 {
-- 
2.21.0

Re: [PATCH v6 6/7] KVM: nVMX: Collapse nested_check_host_control_regs() into its caller

[Krish Sadhukhan]

On 04/11/2019 12:18 PM, Sean Christopherson wrote:
> ...now that nested_vmx_check_host_state() returns -EINVAL to indicate
> failure, i.e. moving the checks doesn't require changing their return
> values to be 'VMXERR_ENTRY_INVALID_HOST_STATE_FIELD'.  This eliminates
> a misleading function name since nested_check_host_control_regs() was
> checking MSRs as well as control registers.
>
> Alternatively, nested_check_host_control_regs() could be split up,
> e.g. into _control_regs(), _debug_regs() and _msrs(), but that's a
> bit gratutitous at this time given that the entire function is 30 LoC.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 26 ++++++--------------------
>   1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 16cff40456ee..98afbe7c15a1 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2590,13 +2590,11 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> -/*
> - * Checks related to Host Control Registers and MSRs
> - */
> -static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> -                                          struct vmcs12 *vmcs12)
> +static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> +				       struct vmcs12 *vmcs12,
> +				       u32 *vm_instruction_error)
>   {
> -	bool ia32e;
> +	*vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
>   
>   	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
>   	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> @@ -2618,8 +2616,8 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>   	 * the host address-space size VM-exit control.
>   	 */
>   	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER) {
> -		ia32e = (vmcs12->vm_exit_controls &
> -			 VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
> +		bool ia32e = (vmcs12->vm_exit_controls &
> +			      VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
>   		if (!kvm_valid_efer(vcpu, vmcs12->host_ia32_efer) ||
>   		    ia32e != !!(vmcs12->host_ia32_efer & EFER_LMA) ||
>   		    ia32e != !!(vmcs12->host_ia32_efer & EFER_LME))
> @@ -2629,18 +2627,6 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> -static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> -				       struct vmcs12 *vmcs12,
> -				       u32 *vm_instruction_error)
> -{
> -	*vm_instruction_error = VMXERR_ENTRY_INVALID_HOST_STATE_FIELD;
> -
> -	if (nested_check_host_control_regs(vcpu, vmcs12))
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>   static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
>   					  struct vmcs12 *vmcs12)
>   {

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

[PATCH v6 7/7] KVM: nVMX: Return -EINVAL when signaling failure in VM-Entry helpers

[Sean Christopherson]

Most, but not all, helpers that are related to emulating consistency
checks for nested VM-Entry return -EINVAL when a check fails.  Convert
the holdouts to have consistency throughout and to make it clear that
the functions are signaling pass/fail as opposed to "resume guest" vs.
"exit to userspace".

Opportunistically fix bad indentation in nested_vmx_check_guest_state().

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

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 98afbe7c15a1..b8b74a73e00b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -917,7 +917,7 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
 	if (cr3 != kvm_read_cr3(vcpu) || (!nested_ept && pdptrs_changed(vcpu))) {
 		if (!nested_cr3_valid(vcpu, cr3)) {
 			*entry_failure_code = ENTRY_FAIL_DEFAULT;
-			return 1;
+			return -EINVAL;
 		}
 
 		/*
@@ -928,7 +928,7 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
 		    !nested_ept) {
 			if (!load_pdptrs(vcpu, vcpu->arch.walk_mmu, cr3)) {
 				*entry_failure_code = ENTRY_FAIL_PDPTE;
-				return 1;
+				return -EINVAL;
 			}
 		}
 	}
@@ -2360,13 +2360,13 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	 */
 	if (vmx->emulation_required) {
 		*entry_failure_code = ENTRY_FAIL_DEFAULT;
-		return 1;
+		return -EINVAL;
 	}
 
 	/* Shadow page tables on either EPT or shadow page tables. */
 	if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12),
 				entry_failure_code))
-		return 1;
+		return -EINVAL;
 
 	if (!enable_ept)
 		vcpu->arch.walk_mmu->inject_page_fault = vmx_inject_page_fault_nested;
@@ -2678,15 +2678,15 @@ static int nested_vmx_check_guest_state(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;
 
 	if (nested_vmx_check_vmcs_link_ptr(vcpu, vmcs12)) {
 		*exit_qual = ENTRY_FAIL_VMCS_LINK_PTR;
-		return 1;
+		return -EINVAL;
 	}
 
 	/*
@@ -2705,16 +2705,16 @@ static int nested_vmx_check_guest_state(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 -EINVAL;
 	}
 
 	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;
+	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
+	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
+		return -EINVAL;
 
 	if (nested_check_guest_non_reg_state(vmcs12))
-		return 1;
+		return -EINVAL;
 
 	return 0;
 }
-- 
2.21.0