Re: [PATCH] x86/kvm/mmu: Remove unnecessary ‘NULL’ values from sptep

[Huang, Kai]

On Tue, 2023-09-05 at 14:09 -0700, Sean Christopherson wrote:
> On Mon, Sep 04, 2023, Kai Huang wrote:
> > On Fri, 2023-09-01 at 09:48 -0700, Sean Christopherson wrote:
> > > @@ -3447,6 +3447,14 @@ static int fast_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> > >                 else
> > >                         sptep = fast_pf_get_last_sptep(vcpu, fault->addr, &spte);
> > >  
> > > +               /*
> > > +                * It's entirely possible for the mapping to have been zapped
> > > +                * by a different task, but the root page is should always be
> > > +                * available as the vCPU holds a reference to its root(s).
> > > +                */
> > > +               if (WARN_ON_ONCE(!sptep))
> > > +                       spte = REMOVED_SPTE;
> > 
> > If I recall correctly, REMOVED_SPTE is only used by TDP MMU code.  Should we use
> > 0 (or initial SPTE value for case like TDX) instead of REMOVED_SPTE?
> 
> I deliberately suggested REMOVED_SPTE in part because of TDX introducing "initial
> SPTE"; finding/remembering '0' initialization of SPTEs is hard.  Though FWIW, '0'
> would be totally fine for TDX because the value is never exposed to hardware.
> 
> I think it's totally fine to use REMOVED_SPTE like this in common code, I would
> be quite surprised if it causes confusion.  Even though REMOVED_SPTE was introduced
> by the TDP MMU, its value/semantics are generic.

Yeah certainly no harm here. :-)

My thinking was REMOVED_SPTE is supposed to be an intermediate state for one
SPTE, which is actually "in the page table", while multiple threads can operate
on the page table entry concurrently.  Slightly mismatch the case here IIUC. 
But I guess it also depends on how we view this case here.