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

[Edgecombe, Rick P]

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?

> 
> 
> >                  }
> >                  handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
> >                                      old_spte, REMOVED_SPTE, sp->role,
> > shared);
> 
> 
> We should call it here after processing lower level.
> 
> 
> 
> > @@ -667,9 +670,6 @@ static void handle_changed_spte(struct kvm *kvm, int
> > as_id,
> > gfn_t gfn,
> >                  handle_removed_pt(kvm, spte_to_child_pt(old_spte, level),
> > shared);
> >          }
> >   
> > -       if (is_mirror && !is_present)
> > -               reflect_removed_spte(kvm, gfn, old_spte, new_spte,
> > role.level);
> > -
> >          if (was_leaf && is_accessed_spte(old_spte) &&
> >              (!is_present || !is_accessed_spte(new_spte) || pfn_changed))
> >                  kvm_set_pfn_accessed(spte_to_pfn(old_spte));
> > @@ -839,6 +839,9 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id,
> > tdp_ptep_t sptep,
> >                                                        new_spte, level),
> > kvm);
> >          }
> >   
> > +       if (is_mirror_sptep(sptep))
> > +               reflect_removed_spte(kvm, gfn, old_spte, REMOVED_SPTE,
> > level);
> > +
> 
> Ditto.
> 
> 
> >          role = sptep_to_sp(sptep)->role;
> >          role.level = level;
> >          handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, role,
> > false);
> 
> The callback should be here.  It should be after handling the lower level.

Ok, let me try.

> 
> 
> 
> > 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?

>   Anyway we can pick from two outcomes based on which is
> easy to understand/maintain.

I guess I can try to generate a diff of the other one and we can compare. It's a
matter of opinion, but I think splitting it between the two methods is the most
confusing.