On Thu, Sep 20, 2018 at 10:45:58AM -0700, Jim Mattson wrote:
> On Tue, Aug 28, 2018 at 9:04 AM, Sean Christopherson
> <sean.j.christopherson@xxxxxxxxx> wrote:
> > Handling consistency check VMExits in nested_vmx_enter_non_root_mode()
> > consolidates all relevant code into a single location, and removing
> > nested_vmx_entry_failure() eliminates a confusing function name and
> > label. For a VMEntry, "fail" and its derivatives has a very specific
> > meaning due to the different behavior of a VMEnter VMFail versus
> > VMExit, i.e. a more appropriate name for nested_vmx_entry_failure()
> > would have been nested_vmx_entry_consistency_check_vmexit().
>
> Though there is definitely some confusion regarding VMFail scenarios
> versus "VM-entry failure" scenarios, "VM-entry failure" is a real
> thing, and this is the terminology used in the SDM. Specifically,
> Table 24-14 Format of Exit Reason describes bit 31 as "VM-entry
> failure (0 = true VM exit; 1 = VM-entry failure)." I think it's better
> to use the vendor's terminology than to make up terminology of our
> own. These aren't "consistency check VM-exits;" they are "VM-entry
> failure VM-exits."
Darn external documentation :) I'll rewrite the label, comment and
changelog to match the SDM.
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > ---
> > arch/x86/kvm/vmx.c | 90 ++++++++++++++++++++--------------------------
> > 1 file changed, 39 insertions(+), 51 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 43e87a2e172e..cb8df73e9b49 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -2056,9 +2056,6 @@ static inline bool is_nmi(u32 intr_info)
> > static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> > u32 exit_intr_info,
> > unsigned long exit_qualification);
> > -static void nested_vmx_entry_failure(struct kvm_vcpu *vcpu,
> > - struct vmcs12 *vmcs12,
> > - u32 reason, unsigned long qualification);
> >
> > static int __find_msr_index(struct vcpu_vmx *vmx, u32 msr)
> > {
> > @@ -12546,25 +12543,23 @@ static int check_vmentry_postreqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> > return 0;
> > }
> >
> > +static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> > + struct vmcs12 *vmcs12);
> > /*
> > * If exit_qual is NULL, this is being called from state restore (either RSM
> > * or KVM_SET_NESTED_STATE). Otherwise it's called from vmlaunch/vmresume.
> > */
> > -static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> > +static 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 from_vmentry = !!exit_qual;
> > - u32 dummy_exit_qual;
> > - int r;
> > + u32 exit_reason = EXIT_REASON_INVALID_STATE;
> > + u32 exit_qual;
> >
> > if (from_vmentry) {
> > - r = check_vmentry_postreqs(vcpu, vmcs12, exit_qual);
> > - if (r) {
> > - nested_vmx_entry_failure(vcpu, vmcs12,
> > - EXIT_REASON_INVALID_STATE, *exit_qual);
> > - return 1;
> > - }
> > + if (check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> > + goto consistency_check_vmexit;
> > }
> >
> > enter_guest_mode(vcpu);
> > @@ -12578,18 +12573,17 @@ static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> > if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
> > vcpu->arch.tsc_offset += vmcs12->tsc_offset;
> >
> > - r = EXIT_REASON_INVALID_STATE;
> > - if (prepare_vmcs02(vcpu, vmcs12, from_vmentry ? exit_qual : &dummy_exit_qual))
> > + if (prepare_vmcs02(vcpu, vmcs12, &exit_qual))
> > goto fail;
> >
> > if (from_vmentry) {
> > nested_get_vmcs12_pages(vcpu);
> >
> > - r = 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)
> > + 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)
> > goto fail;
> > } else {
> > /*
> > @@ -12615,7 +12609,28 @@ static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> > vcpu->arch.tsc_offset -= vmcs12->tsc_offset;
> > leave_guest_mode(vcpu);
> > vmx_switch_vmcs(vcpu, &vmx->vmcs01);
> > - return r;
> > +
> > + /*
> > + * A consistency check VMExit during L1's VMEnter to L2 is a subset
> > + * of a normal VMexit, as explained in 23.7 "VM-entry failures during
> > + * or after loading guest state" (this also lists the acceptable exit-
> > + * reason and exit-qualification parameters).
> > + */
> > +consistency_check_vmexit:
>
> Obviously, I prefer vmentry_failure_vmexit, or some other label based
> on the terminology used in the SDM.
Works for me.
> > + vm_entry_controls_reset_shadow(vmx);
> > + vm_exit_controls_reset_shadow(vmx);
> > + vmx_segment_cache_clear(vmx);
>
> Move to vmx_switch_vmcs.
>
> > + if (!from_vmentry)
> > + return 1;
> > +
> > + load_vmcs12_host_state(vcpu, vmcs12);
> > + vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
> > + vmcs12->exit_qualification = exit_qual;
> > + nested_vmx_succeed(vcpu);
>
> > + if (enable_shadow_vmcs)
> > + vmx->nested.sync_shadow_vmcs = true;
>
> And does this belong in vmx_switch_vmcs as well?
I don't think so, that would cause sync_shadow_vmcs to be incorrectly
set in the nested_vmx_vmexit() flow for a late VMFail.
