Re: [PATCH] KVM: x86/mmu: simplify kvm_tdp_mmu_map flow when guest has to retry

[Robert Hoo]

On Thu, 2022-11-17 at 10:43 -0800, David Matlack wrote:
> On Thu, Nov 17, 2022 at 8:14 AM Paolo Bonzini <pbonzini@xxxxxxxxxx>
> wrote:
> > 
> > A removed SPTE is never present, hence the "if" in kvm_tdp_mmu_map
> > only fails in the exact same conditions that the earlier loop
> > tested in order to issue a  "break". So, instead of checking twice
> > the
> > condition (upper level SPTEs could not be created or was frozen),
> > just
> > exit the loop with a goto---the usual poor-man C replacement for
> > RAII
> > early returns.
> > 
> > While at it, do not use the "ret" variable for return values of
> > functions that do not return a RET_PF_* enum.  This is clearer
> > and also makes it possible to initialize ret to RET_PF_RETRY.
> > 
> > Suggested-by: Robert Hoo <robert.hu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> >  arch/x86/kvm/mmu/tdp_mmu.c | 40 ++++++++++++++++++--------------
> > ------
> >  1 file changed, 19 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c
> > b/arch/x86/kvm/mmu/tdp_mmu.c
> > index e08596775427..771210ce5181 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -1159,7 +1159,7 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu,
> > struct kvm_page_fault *fault)
> >         struct kvm *kvm = vcpu->kvm;
> >         struct tdp_iter iter;
> >         struct kvm_mmu_page *sp;
> > -       int ret;
> > +       int ret = RET_PF_RETRY;
> > 
> >         kvm_mmu_hugepage_adjust(vcpu, fault);
> > 
> > @@ -1168,23 +1168,25 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu,
> > struct kvm_page_fault *fault)
> >         rcu_read_lock();
> > 
> >         tdp_mmu_for_each_pte(iter, mmu, fault->gfn, fault->gfn + 1)
> > {
> > +               int r;
> > +
> >                 if (fault->nx_huge_page_workaround_enabled)
> >                         disallowed_hugepage_adjust(fault,
> > iter.old_spte, iter.level);
> > 
> >                 if (iter.level == fault->goal_level)
> >                         break;
> > 
> > -               /* Step down into the lower level page table if it
> > exists. */
> > -               if (is_shadow_present_pte(iter.old_spte) &&
> > -                   !is_large_pte(iter.old_spte))
> > -                       continue;
> > -
> >                 /*
> >                  * If SPTE has been frozen by another thread, just
> > give up and
> >                  * retry, avoiding unnecessary page table
> > allocation and free.
> >                  */
> >                 if (is_removed_spte(iter.old_spte))
> > -                       break;
> > +                       goto retry;
> > +
> > +               /* Step down into the lower level page table if it
> > exists. */
> > +               if (is_shadow_present_pte(iter.old_spte) &&
> > +                   !is_large_pte(iter.old_spte))
> > +                       continue;
> > 
> >                 /*
> >                  * The SPTE is either non-present or points to a
> > huge page that
> > @@ -1196,13 +1198,17 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu,
> > struct kvm_page_fault *fault)
> >                 sp->nx_huge_page_disallowed = fault-
> > >huge_page_disallowed;
> > 
> >                 if (is_shadow_present_pte(iter.old_spte))
> > -                       ret = tdp_mmu_split_huge_page(kvm, &iter,
> > sp, true);
> > +                       r = tdp_mmu_split_huge_page(kvm, &iter, sp,
> > true);
> >                 else
> > -                       ret = tdp_mmu_link_sp(kvm, &iter, sp,
> > true);
> > +                       r = tdp_mmu_link_sp(kvm, &iter, sp, true);
> 
> Can this fix be squashed into [1]? It seems like a serious enough
> bug.
> If 2 threads race to update the same PTE, KVM will return -EBUSY out
> to userspace from KVM_RUN, I think. I'm not sure about QEMU, but that
> would be fatal for the VM in Vanadium.
> 
> [1] 
> https://lore.kernel.org/kvm/20221109185905.486172-3-dmatlack@xxxxxxxxxx/
> 
I think in you patch it's all right, since then before
kvm_tdp_mmu_map() returns, it must go through
tdp_mmu_map_handle_target_level(), it returns RET_PF_* enum.

> > 
> > -               if (ret) {
> > +               /*
> > +                * Also force the guest to retry the access if the
> > upper level SPTEs
> > +                * aren't in place.
> > +                */
> > +               if (r) {
> >                         tdp_mmu_free_sp(sp);
> > -                       break;
> > +                       goto retry;
> >                 }
> > 
> >                 if (fault->huge_page_disallowed &&
> > @@ -1213,18 +1219,10 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu,
> > struct kvm_page_fault *fault)
> >                 }
> >         }
> > 
> > -       /*
> > -        * Force the guest to retry the access if the upper level
> > SPTEs aren't
> > -        * in place, or if the target leaf SPTE is frozen by
> > another CPU.
> > -        */
> > -       if (iter.level != fault->goal_level ||
> > is_removed_spte(iter.old_spte)) {
> > -               rcu_read_unlock();
> > -               return RET_PF_RETRY;
> > -       }
> > -
> >         ret = tdp_mmu_map_handle_target_level(vcpu, fault, &iter);
> > -       rcu_read_unlock();
> > 
> > +retry:
> > +       rcu_read_unlock();
> >         return ret;
> >  }
> > 
> > --
> > 2.31.1
> >