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 <sean.j.christopherson@intel.com>]

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