Re: [PATCH v4 18/30] KVM: x86/mmu: Zap only TDP MMU leafs in kvm_zap_gfn_range()

[Sean Christopherson <seanjc@google.com>]

On Sat, Mar 26, 2022, Mingwei Zhang wrote:
> On Fri, Mar 25, 2022, Sean Christopherson wrote:
> > On Sun, Mar 13, 2022, Mingwei Zhang wrote:
> > > On Thu, Mar 3, 2022 at 11:39 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > @@ -898,13 +879,13 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
> > > >   * SPTEs have been cleared and a TLB flush is needed before releasing the
> > > >   * MMU lock.
> > > >   */
> > > > -bool __kvm_tdp_mmu_zap_gfn_range(struct kvm *kvm, int as_id, gfn_t start,
> > > > -                                gfn_t end, bool can_yield, bool flush)
> > > > +bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, int as_id, gfn_t start, gfn_t end,
> > > > +                          bool can_yield, bool flush)
> > > >  {
> > > >         struct kvm_mmu_page *root;
> > > >
> > > >         for_each_tdp_mmu_root_yield_safe(kvm, root, as_id)
> > > > -               flush = zap_gfn_range(kvm, root, start, end, can_yield, flush);
> > > > +               flush = tdp_mmu_zap_leafs(kvm, root, start, end, can_yield, false);
> > > 
> > > hmm, I think we might have to be very careful here. If we only zap
> > > leafs, then there could be side effects. For instance, the code in
> > > disallowed_hugepage_adjust() may not work as intended. If you check
> > > the following condition in arch/x86/kvm/mmu/mmu.c:2918
> > > 
> > > if (cur_level > PG_LEVEL_4K &&
> > >     cur_level == fault->goal_level &&
> > >     is_shadow_present_pte(spte) &&
> > >     !is_large_pte(spte)) {
> > > 
> > > If we previously use 4K mappings in this range due to various reasons
> > > (dirty logging etc), then afterwards, we zap the range. Then the guest
> > > touches a 4K and now we should map the range with whatever the maximum
> > > level we can for the guest.
> > > 
> > > However, if we just zap only the leafs, then when the code comes to
> > > the above location, is_shadow_present_pte(spte) will return true,
> > > since the spte is a non-leaf (say a regular PMD entry). The whole if
> > > statement will be true, then we never allow remapping guest memory
> > > with huge pages.
> > 
> > But that's at worst a performance issue, and arguably working as intended.  The
> > zap in this case is never due to the _guest_ unmapping the pfn, so odds are good
> > the guest will want to map back in the same pfns with the same permissions.
> > Zapping shadow pages so that the guest can maybe create a hugepage may end up
> > being a lot of extra work for no benefit.  Or it may be a net positive.  Either
> > way, it's not a functional issue.
> 
> This should be a performance bug instead of a functional one. But it
> does affect both dirty logging (before Ben's early page promotion) and
> our demand paging.

I'd buy the argument that KVM should zap shadow pages when zapping specifically to
recreate huge pages, but that's a different path entirely.  Disabling of dirty
logging uses a dedicated path, zap_collapsible_spte_range().

> So I proposed the fix in here:
> 
> https://lore.kernel.org/lkml/20220323184915.1335049-2-mizhang@google.com/T/#me78d50ffac33f4f418432f7b171c50630414ef28
> 
> If we see memory corruptions, I bet it could only be that we miss some
> TLB flushes, since this patch series is basically trying to avoid
> immediate TLB flushing by simply changing ASID (assigning new root).

Ya, it was a lost TLB flush goof.  My apologaies for not cc'ing you on the patch.

https://lore.kernel.org/all/20220325230348.2587437-1-seanjc@google.com

> To debug, maybe force the TLB flushes after zap_gfn_range and see if the
> problem still exist?
> 
>