Re: [RFC PATCH v5 048/104] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

[Sean Christopherson]

On Thu, Apr 28, 2022, Sagi Shahar wrote:
> > @@ -468,23 +503,49 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> >
> >         if (was_leaf && is_dirty_spte(old_spte) &&
> >             (!is_present || !is_dirty_spte(new_spte) || pfn_changed))
> > -               kvm_set_pfn_dirty(spte_to_pfn(old_spte));
> > +               kvm_set_pfn_dirty(old_pfn);
> > +
> > +       /*
> > +        * Special handling for the private mapping.  We are either
> > +        * setting up new mapping at middle level page table, or leaf,
> > +        * or tearing down existing mapping.
> > +        */
> > +       if (private_spte) {
> > +               void *sept_page = NULL;
> > +
> > +               if (is_present && !is_leaf) {
> > +                       struct kvm_mmu_page *sp = to_shadow_page(pfn_to_hpa(new_pfn));
> > +
> > +                       sept_page = kvm_mmu_private_sp(sp);
> > +                       WARN_ON(!sept_page);
> > +                       WARN_ON(sp->role.level + 1 != level);
> > +                       WARN_ON(sp->gfn != gfn);
> > +               }
> > +
> > +               static_call(kvm_x86_handle_changed_private_spte)(
> > +                       kvm, gfn, level,
> > +                       old_pfn, was_present, was_leaf,
> > +                       new_pfn, is_present, is_leaf, sept_page);
> > +       }
> >
> >         /*
> >          * Recursively handle child PTs if the change removed a subtree from
> >          * the paging structure.
> >          */
> > -       if (was_present && !was_leaf && (pfn_changed || !is_present))
> > +       if (was_present && !was_leaf && (pfn_changed || !is_present)) {
> > +               WARN_ON(private_spte !=
> > +                       is_private_spte(spte_to_child_pt(old_spte, level)));

This sanity check is pointless.  The private flag comes from the parent shadow
page role, and that's not changing.

> > @@ -1015,6 +1137,12 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> >                     is_large_pte(iter.old_spte)) {
> >                         if (!tdp_mmu_zap_spte_atomic(vcpu->kvm, &iter))
> >                                 break;
> > +                       /*
> > +                        * TODO: large page support.
> > +                        * Doesn't support large page for TDX now
> > +                        */
> > +                       WARN_ON(is_private_spte(&iter.old_spte));
> 
> The above line is causing a null ptr dereferencing when running the
> KVM unit tests.
> It should be is_private_spte(iter.sptep) instead of
> is_private_spte(&iter.old_spte)
> While old_spte holds a snapshot of the value pointed to by sptep,
> &old_spte is not equivalent to sptep.

Bug aside, the name is really, really bad.  All of the existing helpers with an
"is_blah_spte()" name take an SPTE value, not a pointer to an SPTE.

is_private_sptep() is the obvious choice.  That makes me a bit nervous too, and
I don't love having to go back to the parent to query private vs shared.

That said, I think it's worth waiting to see the next version of this series before
going behind the bikeshed, I suspect many/most of the calls will go away, i.e. we
might find a better option presents itself.