Re: [PATCH 4.19 067/306] KVM: nVMX: move check_vmentry_postreqs() call to nested_vmx_enter_non_root_mode()

[Dan Rue]

On Thu, Dec 05, 2019 at 10:51:18AM +0100, Jack Wang wrote:
> Dan Rue <dan.rue@xxxxxxxxxx> ä2019å12æ4æåä äå6:50åéï
> >
> > On Mon, Dec 02, 2019 at 03:40:04PM +0100, Jack Wang wrote:
> > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> ä2019å11æ27æåä äå10:30åéï
> > > >
> > > > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > >
> > > > [ Upstream commit 7671ce21b13b9596163a29f4712cb2451a9b97dc ]
> > > >
> > > > In preparation of supporting checkpoint/restore for nested state,
> > > > commit ca0bde28f2ed ("kvm: nVMX: Split VMCS checks from nested_vmx_run()")
> > > > modified check_vmentry_postreqs() to only perform the guest EFER
> > > > consistency checks when nested_run_pending is true.  But, in the
> > > > normal nested VMEntry flow, nested_run_pending is only set after
> > > > check_vmentry_postreqs(), i.e. the consistency check is being skipped.
> > > >
> > > > Alternatively, nested_run_pending could be set prior to calling
> > > > check_vmentry_postreqs() in nested_vmx_run(), but placing the
> > > > consistency checks in nested_vmx_enter_non_root_mode() allows us
> > > > to split prepare_vmcs02() and interleave the preparation with
> > > > the consistency checks without having to change the call sites
> > > > of nested_vmx_enter_non_root_mode().  In other words, the rest
> > > > of the consistency check code in nested_vmx_run() will be joining
> > > > the postreqs checks in future patches.
> > > >
> > > > Fixes: ca0bde28f2ed ("kvm: nVMX: Split VMCS checks from nested_vmx_run()")
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > > Cc: Jim Mattson <jmattson@xxxxxxxxxx>
> > > > Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > > > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > > > ---
> > > >  arch/x86/kvm/vmx.c | 10 +++-------
> > > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > > index fe7fdd666f091..bdf019f322117 100644
> > > > --- a/arch/x86/kvm/vmx.c
> > > > +++ b/arch/x86/kvm/vmx.c
> > > > @@ -12694,6 +12694,9 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> > > >         if (likely(!evaluate_pending_interrupts) && kvm_vcpu_apicv_active(vcpu))
> > > >                 evaluate_pending_interrupts |= vmx_has_apicv_interrupt(vcpu);
> > > >
> > > > +       if (from_vmentry && check_vmentry_postreqs(vcpu, vmcs12, exit_qual))
> > > > +               return EXIT_REASON_INVALID_STATE;
> > > > +
> > > >         enter_guest_mode(vcpu);
> > > >
> > > >         if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS))
> > > > @@ -12836,13 +12839,6 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
> > > >          */
> > > >         skip_emulated_instruction(vcpu);
> > > >
> > > > -       ret = check_vmentry_postreqs(vcpu, vmcs12, &exit_qual);
> > > > -       if (ret) {
> > > > -               nested_vmx_entry_failure(vcpu, vmcs12,
> > > > -                                        EXIT_REASON_INVALID_STATE, exit_qual);
> > > > -               return 1;
> > > > -       }
> > > > -
> > > >         /*
> > > >          * We're finally done with prerequisite checking, and can start with
> > > >          * the nested entry.
> > > > --
> > > > 2.20.1
> > > >
> > > >
> > > >
> > > Hi all,
> > >
> > > This commit caused many kvm-unit-tests regression, cherry-pick
> > > following commits from 4.20 fix the regression:
> >
> > Hi Jack - can you be more specific about the failing tests? What type of
> > environment and which tests failed, which version of kvm-unit-tests? Do
> > you have any logs available? I ask because we do run kvm-unit-tests on
> > x86 and arm64 but we did not see these regressions.
> >
> > Thanks,
> > Dan
> >
> Hi Dan,
> 
> I'm running at kvm-unit-tests commit b1414c5f0142 ("x86: vmx: fix
> required alignment for posted interrupt descriptor")
> 
> using "run_tests.sh -a -t -j8" with qemu-2.7.1
> 
> Left side has only 78 tests ok, and right side has 112 tests ok.

Thanks - so we run it with "run_tests.sh -v" and only see 43 passes in
the best case. Besides missing -a, we see a skip for the vmx related
tests because vmx isn't enabled in our environment.

We will fix those problems in LKFT so that we can catch regressions like
this before they are released.

Dan