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>
