[5/6,v5,nVMX] : nested_vmx_check_vmentry_postreqs() should return VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_INVALID_STATE for error conditions
diff mbox series

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

Commit Message

Krish Sadhukhan April 8, 2019, 9:35 p.m. UTC
..to 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(-)

Comments

Sean Christopherson April 9, 2019, 4:20 p.m. UTC | #1
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>
Paolo Bonzini April 10, 2019, 9:50 a.m. UTC | #2
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
Sean Christopherson April 10, 2019, 4:08 p.m. UTC | #3
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?
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;
 	}
Paolo Bonzini April 10, 2019, 5:05 p.m. UTC | #4
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
Sean Christopherson April 10, 2019, 5:55 p.m. UTC | #5
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.
Krish Sadhukhan April 11, 2019, 12:15 a.m. UTC | #6
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.
Paolo Bonzini April 11, 2019, 12:14 p.m. UTC | #7
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
Sean Christopherson April 11, 2019, 4:29 p.m. UTC | #8
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?
Krish Sadhukhan April 11, 2019, 6:15 p.m. UTC | #9
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.

Patch
diff mbox series

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;
 }