Received-SPF: pass (google.com: domain of linux-kernel+bounces-193291-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 28 May 2024 18:24:43 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"sagis@google.com" <sagis@google.com>,
	"dmatlack@google.com" <dmatlack@google.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"Aktas, Erdem" <erdemaktas@google.com>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU
Message-ID: <20240529012443.GE386318@ls.amr.corp.intel.com>
References: <20240515005952.3410568-1-rick.p.edgecombe@intel.com>
 <20240515005952.3410568-11-rick.p.edgecombe@intel.com>
 <36a1b5d239bdbca588625a75660406c1b5ea952a.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <36a1b5d239bdbca588625a75660406c1b5ea952a.camel@intel.com>

On Tue, May 28, 2024 at 08:54:31PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:

> On Tue, 2024-05-14 at 17:59 -0700, Rick Edgecombe wrote:
> > +static inline int __tdp_mmu_set_spte_atomic(struct kvm *kvm, struct tdp_iter
> > *iter, u64 new_spte)
> >  {
> >         u64 *sptep = rcu_dereference(iter->sptep);
> >  
> > @@ -542,15 +671,42 @@ static inline int __tdp_mmu_set_spte_atomic(struct
> > tdp_iter *iter, u64 new_spte)
> >          */
> >         WARN_ON_ONCE(iter->yielded || is_removed_spte(iter->old_spte));
> >  
> > -       /*
> > -        * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
> > -        * does not hold the mmu_lock.  On failure, i.e. if a different
> > logical
> > -        * CPU modified the SPTE, try_cmpxchg64() updates iter->old_spte with
> > -        * the current value, so the caller operates on fresh data, e.g. if it
> > -        * retries tdp_mmu_set_spte_atomic()
> > -        */
> > -       if (!try_cmpxchg64(sptep, &iter->old_spte, new_spte))
> > -               return -EBUSY;
> > +       if (is_private_sptep(iter->sptep) && !is_removed_spte(new_spte)) {
> > +               int ret;
> > +
> > +               if (is_shadow_present_pte(new_spte)) {
> > +                       /*
> > +                        * Populating case.
> > +                        * - set_private_spte_present() implements
> > +                        *   1) Freeze SPTE
> > +                        *   2) call hooks to update private page table,
> > +                        *   3) update SPTE to new_spte
> > +                        * - handle_changed_spte() only updates stats.
> > +                        */
> > +                       ret = set_private_spte_present(kvm, iter->sptep, iter-
> > >gfn,
> > +                                                      iter->old_spte,
> > new_spte, iter->level);
> > +                       if (ret)
> > +                               return ret;
> > +               } else {
> > +                       /*
> > +                        * Zapping case.
> > +                        * Zap is only allowed when write lock is held
> > +                        */
> > +                       if (WARN_ON_ONCE(!is_shadow_present_pte(new_spte)))
> 
> This inside an else block for (is_shadow_present_pte(new_spte)), so it will
> always be true if it gets here. But it can't because TDX doesn't do any atomic
> zapping.
> 
> We can remove the conditional, but in regards to the WARN, any recollection of
> what was might have been going on here originally?

We had an optimization so that there are other state in addition to present,
non-present.  When I dropped it, I should've dropped else-sentence.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>