Re: [PATCH 2/2] KVM: x86/mmu: Ensure TLBs are flushed when yielding during NX zapping

[Sean Christopherson]

On Mon, Mar 22, 2021, Ben Gardon wrote:
> On Fri, Mar 19, 2021 at 4:20 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > @@ -5960,19 +5963,21 @@ static void kvm_recover_nx_lpages(struct kvm *kvm)
> >                                       lpage_disallowed_link);
> >                 WARN_ON_ONCE(!sp->lpage_disallowed);
> >                 if (is_tdp_mmu_page(sp)) {
> > -                       kvm_tdp_mmu_zap_gfn_range(kvm, sp->gfn,
> > -                               sp->gfn + KVM_PAGES_PER_HPAGE(sp->role.level));
> > +                       gfn_end = sp->gfn + KVM_PAGES_PER_HPAGE(sp->role.level);
> > +                       flush = kvm_tdp_mmu_zap_gfn_range(kvm, sp->gfn, gfn_end,
> > +                                                         flush || !list_empty(&invalid_list));
> >                 } else {
> >                         kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list);
> >                         WARN_ON_ONCE(sp->lpage_disallowed);
> >                 }
> >
> >                 if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) {
> > -                       kvm_mmu_commit_zap_page(kvm, &invalid_list);
> > +                       kvm_mmu_remote_flush_or_zap(kvm, &invalid_list, flush);
> 
> This pattern of waiting until a yield is needed or lock contention is
> detected has always been a little suspect to me because
> kvm_mmu_commit_zap_page does work proportional to the work done before
> the yield was needed. That seems like more work than we should like to
> be doing at that point.
> 
> The yield in kvm_tdp_mmu_zap_gfn_range makes that phenomenon even
> worse. Because we can satisfy the need to yield without clearing out
> the invalid list, we can potentially queue many more pages which will
> then all need to have their zaps committed at once. This is an
> admittedly contrived case which could only be hit in a high load
> nested scenario.
> 
> It could be fixed by forbidding kvm_tdp_mmu_zap_gfn_range from
> yielding. Since we should only need to zap one SPTE, the yield should
> not be needed within the kvm_tdp_mmu_zap_gfn_range call. To ensure
> that only one SPTE is zapped we would have to specify the root though.
> Otherwise we could end up zapping all the entries for the same GFN
> range under an unrelated root.

Hmm, I originally did exactly that, but changed my mind because this zaps far
more than 1 SPTE.  This is zapping a SP that could be huge, but is not, which
means it's guaranteed to have a non-zero number of child SPTEs.  The worst case
scenario is that SP is a PUD (potential 1gb page) and the leafs are 4k SPTEs.

But, I didn't consider the interplay between invalid_list and the TDP MMU
yielding.  Hrm.