Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

[Isaku Yamahata]

On Wed, May 29, 2024 at 02:13:24AM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@xxxxxxxxx> wrote:

> On Tue, 2024-05-28 at 18:57 -0700, Isaku Yamahata wrote:
> > > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > > @@ -438,6 +438,9 @@ static void handle_removed_pt(struct kvm *kvm,
> > > tdp_ptep_t
> > > pt, bool shared)
> > >                           */
> > >                          old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte,
> > >                                                            REMOVED_SPTE,
> > > level);
> > > +
> > > +                       if (is_mirror_sp(sp))
> > > +                               reflect_removed_spte(kvm, gfn, old_spte,
> > > REMOVED_SPTE, level);
> > 
> > The callback before handling lower level will result in error.
> 
> Hmm, yea the order is changed. It didn't result in an error for some reason
> though. Can you elaborate?

TDH.MEM.{PAGE, SEPT}.REMOVE() needs to be issued from the leaf.  I guess
zapping is done at only leaf by tdp_mmu_zap_leafs(). Subtree zapping case wasn't
exercised.


> > > Otherwise, we could move the "set present" mirroring operations into
> > > handle_changed_spte(), and have some earlier conditional logic do the
> > > REMOVED_SPTE parts. It starts to become more scattered.
> > > Anyway, it's just a code clarity thing arising from having hard time
> > > explaining
> > > the design in the log. Any opinions?
> > 
> > Originally I tried to consolidate the callbacks by following TDP MMU using
> > handle_changed_spte().
> 
> How did it handle the REMOVED_SPTE part of the set_present() path?

is_removed_pt() was used. It was ugly.
-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>