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?