On Wed, May 14, 2025 at 06:56:26AM +0800, Edgecombe, Rick P wrote:
> On Thu, 2025-04-24 at 11:08 +0800, Yan Zhao wrote:
> > Introduce kvm_split_boundary_leafs() to manage the splitting of boundary
> > leafs within the mirror root.
> >
> > Before zapping a specific GFN range in the mirror root, split any huge leaf
> > that intersects with the boundary of the GFN range to ensure that the
> > subsequent zap operation does not impact any GFN outside the specified
> > range. This is crucial for the mirror root as the private page table
> > requires the guest's ACCEPT operation after faulting back a GFN.
> >
> > This function should be called while kvm->mmu_lock is held for writing. The
> > kvm->mmu_lock is temporarily released to allocate memory for sp for split.
> > The only expected error is -ENOMEM.
> >
> > Opportunistically, WARN in tdp_mmu_zap_leafs() if zapping a huge leaf in
> > the mirror root affects a GFN outside the specified range.
> >
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > Signed-off-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 21 +++++++
> > arch/x86/kvm/mmu/tdp_mmu.c | 116 ++++++++++++++++++++++++++++++++++++-
> > arch/x86/kvm/mmu/tdp_mmu.h | 1 +
> > include/linux/kvm_host.h | 1 +
> > 4 files changed, 136 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 0e227199d73e..0d49c69b6b55 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1640,6 +1640,27 @@ static bool __kvm_rmap_zap_gfn_range(struct kvm *kvm,
> > start, end - 1, can_yield, true, flush);
> > }
> >
> > +/*
> > + * Split large leafs at the boundary of the specified range for the mirror root
> > + *
> > + * Return value:
> > + * 0 : success, no flush is required;
> > + * 1 : success, flush is required;
> > + * <0: failure.
> > + */
> > +int kvm_split_boundary_leafs(struct kvm *kvm, struct kvm_gfn_range *range)
> > +{
> > + bool ret = 0;
> > +
> > + lockdep_assert_once(kvm->mmu_invalidate_in_progress ||
> > + lockdep_is_held(&kvm->slots_lock));
> > +
> > + if (tdp_mmu_enabled)
> > + ret = kvm_tdp_mmu_gfn_range_split_boundary(kvm, range);
> > +
> > + return ret;
> > +}
> > +
> > bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range)
> > {
> > bool flush = false;
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 0f683753a7bb..d3fba5d11ea2 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -324,6 +324,8 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> > u64 old_spte, u64 new_spte, int level,
> > bool shared);
> >
> > +static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
> > + struct kvm_mmu_page *sp, bool shared);
> > static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(bool mirror);
> > static void *get_external_spt(gfn_t gfn, u64 new_spte, int level);
> >
> > @@ -962,6 +964,19 @@ bool kvm_tdp_mmu_zap_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> > return true;
> > }
> >
> > +static inline bool iter_split_required(struct kvm *kvm, struct kvm_mmu_page *root,
> > + struct tdp_iter *iter, gfn_t start, gfn_t end)
> > +{
> > + if (!is_mirror_sp(root) || !is_large_pte(iter->old_spte))
> > + return false;
> > +
> > + /* Fully contained, no need to split */
> > + if (iter->gfn >= start && iter->gfn + KVM_PAGES_PER_HPAGE(iter->level) <= end)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > /*
> > * If can_yield is true, will release the MMU lock and reschedule if the
> > * scheduler needs the CPU or there is contention on the MMU lock. If this
> > @@ -991,6 +1006,8 @@ static bool tdp_mmu_zap_leafs(struct kvm *kvm, struct kvm_mmu_page *root,
> > !is_last_spte(iter.old_spte, iter.level))
> > continue;
> >
> > + WARN_ON_ONCE(iter_split_required(kvm, root, &iter, start, end));
> > +
>
> Kind of unrelated change? But good idea. Maybe for another patch.
Yes, will move it to a separate patch in a formal version.
As initial RFC, I hoped to show related changes in one patch to allow a whole
picture.
> > tdp_mmu_iter_set_spte(kvm, &iter, SHADOW_NONPRESENT_VALUE);
> >
> > /*
> > @@ -1246,9 +1263,6 @@ static int tdp_mmu_link_sp(struct kvm *kvm, struct tdp_iter *iter,
> > return 0;
> > }
> >
> > -static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
> > - struct kvm_mmu_page *sp, bool shared);
> > -
> > /*
> > * Handle a TDP page fault (NPT/EPT violation/misconfiguration) by installing
> > * page tables and SPTEs to translate the faulting guest physical address.
> > @@ -1341,6 +1355,102 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> > return ret;
> > }
> >
> > +/*
> > + * Split large leafs at the boundary of the specified range for the mirror root
> > + */
> > +static int tdp_mmu_split_boundary_leafs(struct kvm *kvm, struct kvm_mmu_page *root,
> > + gfn_t start, gfn_t end, bool can_yield, bool *flush)
> > +{
> > + struct kvm_mmu_page *sp = NULL;
> > + struct tdp_iter iter;
> > +
> > + WARN_ON_ONCE(!can_yield);
>
> Why pass this in then?
Right, can move the warning up to the caller.
Currently callers of kvm_split_boundary_leafs() are only
kvm_arch_pre_set_memory_attributes() and kvm_gmem_punch_hole(), so can_yield is
always false.
> > +
> > + if (!is_mirror_sp(root))
> > + return 0;
>
> What is special about mirror roots here?
Hmm, I thought only the mirror root needs splitting before zapping of the
S-EPT, which requires guest's acceptance. Other roots could tolerate zapping of
a larger range than required.
Maybe AMD guys can shout out if I'm wrong.
> > + end = min(end, tdp_mmu_max_gfn_exclusive());
> > +
> > + lockdep_assert_held_write(&kvm->mmu_lock);
> > +
> > + rcu_read_lock();
> > +
> > + for_each_tdp_pte_min_level(iter, kvm, root, PG_LEVEL_4K, start, end) {
> > +retry:
> > + if (can_yield &&
>
> Do we need this part of the conditional based on the above?
No need if we don't pass in can_yield.
> > + tdp_mmu_iter_cond_resched(kvm, &iter, *flush, false)) {
> > + *flush = false;
> > + continue;
> > + }
> > +
> > + if (!is_shadow_present_pte(iter.old_spte) ||
> > + !is_last_spte(iter.old_spte, iter.level) ||
> > + !iter_split_required(kvm, root, &iter, start, end))
> > + continue;
> > +
> > + if (!sp) {
> > + rcu_read_unlock();
> > +
> > + write_unlock(&kvm->mmu_lock);
> > +
> > + sp = tdp_mmu_alloc_sp_for_split(true);
> > +
> > + write_lock(&kvm->mmu_lock);
> > +
> > + if (!sp) {
> > + trace_kvm_mmu_split_huge_page(iter.gfn, iter.old_spte,
> > + iter.level, -ENOMEM);
> > + return -ENOMEM;
> > + }
> > + rcu_read_lock();
> > +
> > + iter.yielded = true;
> > + continue;
> > + }
> > + tdp_mmu_init_child_sp(sp, &iter);
> > +
> > + if (tdp_mmu_split_huge_page(kvm, &iter, sp, false))
>
> I think it can't fail when you hold mmu write lock.
You are right!
Thanks for catching it.
> > + goto retry;
> > +
> > + sp = NULL;
> > + /*
> > + * Set yielded in case after splitting to a lower level,
> > + * the new iter requires furter splitting.
> > + */
> > + iter.yielded = true;
> > + *flush = true;
> > + }
> > +
> > + rcu_read_unlock();
> > +
> > + /* Leave it here though it should be impossible for the mirror root */
> > + if (sp)
> > + tdp_mmu_free_sp(sp);
>
> What do you think about relying on tdp_mmu_split_huge_pages_root() and moving
> this to an optimization patch at the end?
>
> Or what about just two calls to tdp_mmu_split_huge_pages_root() at the
> boundaries?
Though the two generally look like the same, relying on
tdp_mmu_split_huge_pages_root() will create several minor changes scattering
in tdp_mmu_split_huge_pages_root().
e.g. update flush after tdp_mmu_iter_cond_resched(), check
iter_split_required(), set "iter.yielded = true".
So, it may be hard to review as a initial RFC.
I prefer to do that after Paolo and Sean have taken a look of it :)
> > + return 0;
> > +}
> > +
> > +int kvm_tdp_mmu_gfn_range_split_boundary(struct kvm *kvm, struct kvm_gfn_range *range)
> > +{
> > + enum kvm_tdp_mmu_root_types types;
> > + struct kvm_mmu_page *root;
> > + bool flush = false;
> > + int ret;
> > +
> > + types = kvm_gfn_range_filter_to_root_types(kvm, range->attr_filter) | KVM_INVALID_ROOTS;
>
> What is the reason for KVM_INVALID_ROOTS in this case?
I wanted to keep consistent with that in kvm_tdp_mmu_unmap_gfn_range().
Yes, we can remove the KVM_INVALID_ROOTS.
> > +
> > + __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, types) {
>
> It would be better to check for mirror roots here, instead of inside
> tdp_mmu_split_boundary_leafs().
Ok.
>
> > + ret = tdp_mmu_split_boundary_leafs(kvm, root, range->start, range->end,
> > + range->may_block, &flush);
> > + if (ret < 0) {
> > + if (flush)
> > + kvm_flush_remote_tlbs(kvm);
> > +
> > + return ret;
> > + }
> > + }
> > + return flush;
> > +}
> > +
> > /* Used by mmu notifier via kvm_unmap_gfn_range() */
> > bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range,
> > bool flush)
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> > index 52acf99d40a0..806a21d4f0e3 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.h
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> > @@ -69,6 +69,7 @@ void kvm_tdp_mmu_zap_all(struct kvm *kvm);
> > void kvm_tdp_mmu_invalidate_roots(struct kvm *kvm,
> > enum kvm_tdp_mmu_root_types root_types);
> > void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm, bool shared);
> > +int kvm_tdp_mmu_gfn_range_split_boundary(struct kvm *kvm, struct kvm_gfn_range *range);
> >
> > int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 655d36e1f4db..19d7a577e7ed 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -272,6 +272,7 @@ struct kvm_gfn_range {
> > bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range);
> > bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range);
> > bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range);
> > +int kvm_split_boundary_leafs(struct kvm *kvm, struct kvm_gfn_range *range);
> > #endif
> >
> > enum {
>
