Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 00:46:08 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Sagi Shahar <sagis@google.com>
Cc:     "Yamahata, Isaku" <isaku.yamahata@intel.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Erdem Aktas <erdemaktas@google.com>,
        Connor Kuehl <ckuehl@redhat.com>
Subject: Re: [RFC PATCH v5 048/104] KVM: x86/tdp_mmu: Support TDX private
 mapping for TDP MMU
Message-ID: <Yms1UPapk9jVGHrf@google.com>
References: <cover.1646422845.git.isaku.yamahata@intel.com>
 <7a5246c54427952728bd702bd7f2c6963eefa712.1646422845.git.isaku.yamahata@intel.com>
 <CAAhR5DE4-Z6JYLHbWyEyraqqWCx5bD+gZADAnisDmSJ4bkgV8g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAhR5DE4-Z6JYLHbWyEyraqqWCx5bD+gZADAnisDmSJ4bkgV8g@mail.gmail.com>
Precedence: bulk

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.