On Mon, Oct 11, 2021 at 2:25 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Mon, Oct 11, 2021 at 2:07 PM Ben Gardon <bgardon@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 11, 2021 at 1:44 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > >
> > > slot_handle_leaf is a misnomer because it only operates on 4K SPTEs
> > > whereas "leaf" is used to describe any valid terminal SPTE (4K or
> > > large page). Rename slot_handle_leaf to slot_handle_level_4k to
> > > avoid confusion.
> > >
> > > Making this change makes it more obvious there is a benign discrepency
> > > between the legacy MMU and the TDP MMU when it comes to dirty logging.
> > > The legacy MMU only operates on 4K SPTEs when zapping for collapsing
> > > and when clearing D-bits. The TDP MMU, on the other hand, operates on
> > > SPTEs on all levels. The TDP MMU behavior is technically overkill but
> > > not incorrect. So opportunistically add comments to explain the
> > > difference.
> >
> > Note that at least in the zapping case when disabling dirty logging,
> > the TDP MMU will still only zap pages if they're mapped smaller than
> > the highest granularity they could be. As a result it uses a slower
> > check, but shouldn't be doing many (if any) extra zaps.
>
> Agreed. The legacy MMU implementation relies on the fact that
> collapsible 2M SPTEs are never generated by dirty logging so it only
> needs to check 4K SPTEs.
>
> The TDP MMU implementation is actually more robust, since it checks
> every SPTE for collapsibility. The only reason it would be doing extra
> zaps if there is something other than dirty logging can cause an SPTE
> to be collapsible. (HugePage NX comes to mind.)
Ah but HugePage NX does not create 2M SPTEs so this wouldn't actually
result in extra zaps.
>
> >
> > >
> > > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> >
> > Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx>
> >
> > > ---
> > > arch/x86/kvm/mmu/mmu.c | 18 +++++++++++++-----
> > > 1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > index 24a9f4c3f5e7..f00644e79ef5 100644
> > > --- a/arch/x86/kvm/mmu/mmu.c
> > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > @@ -5382,8 +5382,8 @@ slot_handle_level(struct kvm *kvm, const struct kvm_memory_slot *memslot,
> > > }
> > >
> > > static __always_inline bool
> > > -slot_handle_leaf(struct kvm *kvm, const struct kvm_memory_slot *memslot,
> > > - slot_level_handler fn, bool flush_on_yield)
> > > +slot_handle_level_4k(struct kvm *kvm, const struct kvm_memory_slot *memslot,
> > > + slot_level_handler fn, bool flush_on_yield)
> > > {
> > > return slot_handle_level(kvm, memslot, fn, PG_LEVEL_4K,
> > > PG_LEVEL_4K, flush_on_yield);
> > > @@ -5772,7 +5772,12 @@ void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
> > >
> > > if (kvm_memslots_have_rmaps(kvm)) {
> > > write_lock(&kvm->mmu_lock);
> > > - flush = slot_handle_leaf(kvm, slot, kvm_mmu_zap_collapsible_spte, true);
> > > + /*
> > > + * Strictly speaking only 4k SPTEs need to be zapped because
> > > + * KVM never creates intermediate 2m mappings when performing
> > > + * dirty logging.
> > > + */
> > > + flush = slot_handle_level_4k(kvm, slot, kvm_mmu_zap_collapsible_spte, true);
> > > if (flush)
> > > kvm_arch_flush_remote_tlbs_memslot(kvm, slot);
> > > write_unlock(&kvm->mmu_lock);
> > > @@ -5809,8 +5814,11 @@ void kvm_mmu_slot_leaf_clear_dirty(struct kvm *kvm,
> > >
> > > if (kvm_memslots_have_rmaps(kvm)) {
> > > write_lock(&kvm->mmu_lock);
> > > - flush = slot_handle_leaf(kvm, memslot, __rmap_clear_dirty,
> > > - false);
> > > + /*
> > > + * Strictly speaking only 4k SPTEs need to be cleared because
> > > + * KVM always performs dirty logging at a 4k granularity.
> > > + */
> > > + flush = slot_handle_level_4k(kvm, memslot, __rmap_clear_dirty, false);
> > > write_unlock(&kvm->mmu_lock);
> > > }
> > >
> > > --
> > > 2.33.0.882.g93a45727a2-goog
> > >
