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@xxxxxxxxxx>
> > Suggested-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
