[PATCH v2] KVM: x86/mmu: Rename slot_handle_leaf to slot_handle_level_4k

[PATCH v2] KVM: x86/mmu: Rename slot_handle_leaf to slot_handle_level_4k

[David Matlack]

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 iterates through 4K SPTEs when zapping for
collapsing and when clearing D-bits. The TDP MMU, on the other hand,
iterates through SPTEs on all levels.

The TDP MMU behavior of zapping SPTEs at all levels is technically
overkill for its current dirty logging implementation, which always
demotes to 4k SPTES, but both the TDP MMU and legacy MMU zap if and only
if the SPTE can be replaced by a larger page, i.e. will not spuriously
zap 2m (or larger) SPTEs. Opportunistically add comments to explain this
discrepency in the code.

Signed-off-by: David Matlack <dmatlack@google.com>
---
v1: https://lore.kernel.org/kvm/20211011204418.162846-1-dmatlack@google.com/
- Clarified that the TDP MMU does not perform spurious zaps in commit
  message [Sean, Ben]
- Use "legacy MMU" instead of "KVM" in comments to avoid comments
  becoming stale in the future if the TDP MMU gets support for 2m dirty
  logging [Sean]

 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..fa918289c9e0 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);
+		/*
+		 * Zap only 4k SPTEs since the legacy MMU only supports dirty
+		 * logging at a 4k granularity and never creates collapsible
+		 * 2m SPTEs during 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);
+		/*
+		 * Clear dirty bits only on 4k SPTEs since the legacy MMU only
+		 * support 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.1079.g6e70778dc9-goog

Re: [PATCH v2] KVM: x86/mmu: Rename slot_handle_leaf to slot_handle_level_4k

[Ben Gardon]

On Tue, Oct 19, 2021 at 9:22 AM David Matlack <dmatlack@google.com> 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 iterates through 4K SPTEs when zapping for
> collapsing and when clearing D-bits. The TDP MMU, on the other hand,
> iterates through SPTEs on all levels.
>
> The TDP MMU behavior of zapping SPTEs at all levels is technically
> overkill for its current dirty logging implementation, which always
> demotes to 4k SPTES, but both the TDP MMU and legacy MMU zap if and only
> if the SPTE can be replaced by a larger page, i.e. will not spuriously
> zap 2m (or larger) SPTEs. Opportunistically add comments to explain this
> discrepency in the code.
>
> Signed-off-by: David Matlack <dmatlack@google.com>

Reviewed-by: Ben Gardon <bgardon@google.com>

> ---
> v1: https://lore.kernel.org/kvm/20211011204418.162846-1-dmatlack@google.com/
> - Clarified that the TDP MMU does not perform spurious zaps in commit
>   message [Sean, Ben]
> - Use "legacy MMU" instead of "KVM" in comments to avoid comments
>   becoming stale in the future if the TDP MMU gets support for 2m dirty
>   logging [Sean]
>
>  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..fa918289c9e0 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);
> +               /*
> +                * Zap only 4k SPTEs since the legacy MMU only supports dirty
> +                * logging at a 4k granularity and never creates collapsible
> +                * 2m SPTEs during 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);
> +               /*
> +                * Clear dirty bits only on 4k SPTEs since the legacy MMU only
> +                * support 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.1079.g6e70778dc9-goog
>

Re: [PATCH v2] KVM: x86/mmu: Rename slot_handle_leaf to slot_handle_level_4k

[Sean Christopherson]

On Tue, Oct 19, 2021, David Matlack 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 iterates through 4K SPTEs when zapping for
> collapsing and when clearing D-bits. The TDP MMU, on the other hand,
> iterates through SPTEs on all levels.
> 
> The TDP MMU behavior of zapping SPTEs at all levels is technically
> overkill for its current dirty logging implementation, which always
> demotes to 4k SPTES, but both the TDP MMU and legacy MMU zap if and only
> if the SPTE can be replaced by a larger page, i.e. will not spuriously
> zap 2m (or larger) SPTEs. Opportunistically add comments to explain this
> discrepency in the code.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>