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.
