[PATCH v2 0/6] KVM: x86/mmu: Optimize TDP MMU huge page recovery during disable-dirty-log

[PATCH v2 0/6] KVM: x86/mmu: Optimize TDP MMU huge page recovery during disable-dirty-log

[David Matlack]

Rework the TDP MMU disable-dirty-log path to batch TLB flushes and
recover huge page mappings, rather than zapping and flushing for every
potential huge page mapping.

With this series, dirty_log_perf_test shows a decrease in the time it takes to
disable dirty logging, as well as a decrease in the number of vCPU faults:

 $ ./dirty_log_perf_test -s anonymous_hugetlb_2mb -v 64 -e -b 4g

 Before: Disabling dirty logging time: 14.334453428s (131072 flushes)
 After:  Disabling dirty logging time: 4.794969689s  (76 flushes)

 Before: 393,599      kvm:kvm_page_fault
 After:  262,575      kvm:kvm_page_fault

v2:
 - Use a separate iterator to walk down to child SPTEs during huge page
   recovery [Sean]
 - Return SHADOW_NONPRESENT_VALUE in error conditions in
   make_huge_spte() [Vipin][off-list]

v1: https://lore.kernel.org/kvm/20240805233114.4060019-8-dmatlack@google.com/

David Matlack (6):
  KVM: x86/mmu: Drop @max_level from kvm_mmu_max_mapping_level()
  KVM: x86/mmu: Batch TLB flushes when zapping collapsible TDP MMU SPTEs
  KVM: x86/mmu: Refactor TDP MMU iter need resched check
  KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of
    zapping
  KVM: x86/mmu: Rename make_huge_page_split_spte() to make_small_spte()
  KVM: x86/mmu: WARN if huge page recovery triggered during dirty
    logging

 arch/x86/include/asm/kvm_host.h |   4 +-
 arch/x86/kvm/mmu/mmu.c          |  16 ++--
 arch/x86/kvm/mmu/mmu_internal.h |   3 +-
 arch/x86/kvm/mmu/spte.c         |  43 +++++++++--
 arch/x86/kvm/mmu/spte.h         |   5 +-
 arch/x86/kvm/mmu/tdp_mmu.c      | 129 +++++++++++++++++---------------
 arch/x86/kvm/mmu/tdp_mmu.h      |   4 +-
 arch/x86/kvm/x86.c              |  18 ++---
 8 files changed, 128 insertions(+), 94 deletions(-)


base-commit: 728d17c2cb8cc5f9ac899173d0e9a67fb8887622
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v2 1/6] KVM: x86/mmu: Drop @max_level from kvm_mmu_max_mapping_level()

[David Matlack]

Drop the @max_level parameter from kvm_mmu_max_mapping_level(). All
callers pass in PG_LEVEL_NUM, so @max_level can be replaced with
PG_LEVEL_NUM in the function body.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/mmu.c          | 8 +++-----
 arch/x86/kvm/mmu/mmu_internal.h | 3 +--
 arch/x86/kvm/mmu/tdp_mmu.c      | 3 +--
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index d25c2b395116..2e92d9e9b311 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3109,13 +3109,12 @@ static int __kvm_mmu_max_mapping_level(struct kvm *kvm,
 }
 
 int kvm_mmu_max_mapping_level(struct kvm *kvm,
-			      const struct kvm_memory_slot *slot, gfn_t gfn,
-			      int max_level)
+			      const struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	bool is_private = kvm_slot_can_be_private(slot) &&
 			  kvm_mem_is_private(kvm, gfn);
 
-	return __kvm_mmu_max_mapping_level(kvm, slot, gfn, max_level, is_private);
+	return __kvm_mmu_max_mapping_level(kvm, slot, gfn, PG_LEVEL_NUM, is_private);
 }
 
 void kvm_mmu_hugepage_adjust(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
@@ -6877,8 +6876,7 @@ static bool kvm_mmu_zap_collapsible_spte(struct kvm *kvm,
 		 * mapping if the indirect sp has level = 1.
 		 */
 		if (sp->role.direct &&
-		    sp->role.level < kvm_mmu_max_mapping_level(kvm, slot, sp->gfn,
-							       PG_LEVEL_NUM)) {
+		    sp->role.level < kvm_mmu_max_mapping_level(kvm, slot, sp->gfn)) {
 			kvm_zap_one_rmap_spte(kvm, rmap_head, sptep);
 
 			if (kvm_available_flush_remote_tlbs_range())
diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
index 1721d97743e9..fee385e75405 100644
--- a/arch/x86/kvm/mmu/mmu_internal.h
+++ b/arch/x86/kvm/mmu/mmu_internal.h
@@ -344,8 +344,7 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 }
 
 int kvm_mmu_max_mapping_level(struct kvm *kvm,
-			      const struct kvm_memory_slot *slot, gfn_t gfn,
-			      int max_level);
+			      const struct kvm_memory_slot *slot, gfn_t gfn);
 void kvm_mmu_hugepage_adjust(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 void disallowed_hugepage_adjust(struct kvm_page_fault *fault, u64 spte, int cur_level);
 
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 3c55955bcaf8..2a843b9c8d81 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1655,8 +1655,7 @@ static void zap_collapsible_spte_range(struct kvm *kvm,
 		if (iter.gfn < start || iter.gfn >= end)
 			continue;
 
-		max_mapping_level = kvm_mmu_max_mapping_level(kvm, slot,
-							      iter.gfn, PG_LEVEL_NUM);
+		max_mapping_level = kvm_mmu_max_mapping_level(kvm, slot, iter.gfn);
 		if (max_mapping_level < iter.level)
 			continue;
 
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v2 2/6] KVM: x86/mmu: Batch TLB flushes when zapping collapsible TDP MMU SPTEs

[David Matlack]

Set SPTEs directly to SHADOW_NONPRESENT_VALUE and batch up TLB flushes
when zapping collapsible SPTEs, rather than freezing them first.

Freezing the SPTE first is not required. It is fine for another thread
holding mmu_lock for read to immediately install a present entry before
TLBs are flushed because the underlying mapping is not changing. vCPUs
that translate through the stale 4K mappings or a new huge page mapping
will still observe the same GPA->HPA translations.

KVM must only flush TLBs before dropping RCU (to avoid use-after-free of
the zapped page tables) and before dropping mmu_lock (to synchronize
with mmu_notifiers invalidating mappings).

In VMs backed with 2MiB pages, batching TLB flushes improves the time it
takes to zap collapsible SPTEs to disable dirty logging:

 $ ./dirty_log_perf_test -s anonymous_hugetlb_2mb -v 64 -e -b 4g

 Before: Disabling dirty logging time: 14.334453428s (131072 flushes)
 After:  Disabling dirty logging time: 4.794969689s  (76 flushes)

Skipping freezing SPTEs also avoids stalling vCPU threads on the frozen
SPTE for the time it takes to perform a remote TLB flush. vCPUs faulting
on the zapped mapping can now immediately install a new huge mapping and
proceed with guest execution.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 55 +++++++-------------------------------
 1 file changed, 10 insertions(+), 45 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 2a843b9c8d81..27adbb3ecb02 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -591,48 +591,6 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
 	return 0;
 }
 
-static inline int __must_check tdp_mmu_zap_spte_atomic(struct kvm *kvm,
-						       struct tdp_iter *iter)
-{
-	int ret;
-
-	lockdep_assert_held_read(&kvm->mmu_lock);
-
-	/*
-	 * Freeze the SPTE by setting it to a special, non-present value. This
-	 * will stop other threads from immediately installing a present entry
-	 * in its place before the TLBs are flushed.
-	 *
-	 * Delay processing of the zapped SPTE until after TLBs are flushed and
-	 * the FROZEN_SPTE is replaced (see below).
-	 */
-	ret = __tdp_mmu_set_spte_atomic(iter, FROZEN_SPTE);
-	if (ret)
-		return ret;
-
-	kvm_flush_remote_tlbs_gfn(kvm, iter->gfn, iter->level);
-
-	/*
-	 * No other thread can overwrite the frozen SPTE as they must either
-	 * wait on the MMU lock or use tdp_mmu_set_spte_atomic() which will not
-	 * overwrite the special frozen SPTE value. Use the raw write helper to
-	 * avoid an unnecessary check on volatile bits.
-	 */
-	__kvm_tdp_mmu_write_spte(iter->sptep, SHADOW_NONPRESENT_VALUE);
-
-	/*
-	 * Process the zapped SPTE after flushing TLBs, and after replacing
-	 * FROZEN_SPTE with 0. This minimizes the amount of time vCPUs are
-	 * blocked by the FROZEN_SPTE and reduces contention on the child
-	 * SPTEs.
-	 */
-	handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
-			    SHADOW_NONPRESENT_VALUE, iter->level, true);
-
-	return 0;
-}
-
-
 /*
  * tdp_mmu_set_spte - Set a TDP MMU SPTE and handle the associated bookkeeping
  * @kvm:	      KVM instance
@@ -1625,13 +1583,16 @@ static void zap_collapsible_spte_range(struct kvm *kvm,
 	gfn_t end = start + slot->npages;
 	struct tdp_iter iter;
 	int max_mapping_level;
+	bool flush = false;
 
 	rcu_read_lock();
 
 	for_each_tdp_pte_min_level(iter, root, PG_LEVEL_2M, start, end) {
 retry:
-		if (tdp_mmu_iter_cond_resched(kvm, &iter, false, true))
+		if (tdp_mmu_iter_cond_resched(kvm, &iter, flush, true)) {
+			flush = false;
 			continue;
+		}
 
 		if (iter.level > KVM_MAX_HUGEPAGE_LEVEL ||
 		    !is_shadow_present_pte(iter.old_spte))
@@ -1659,11 +1620,15 @@ static void zap_collapsible_spte_range(struct kvm *kvm,
 		if (max_mapping_level < iter.level)
 			continue;
 
-		/* Note, a successful atomic zap also does a remote TLB flush. */
-		if (tdp_mmu_zap_spte_atomic(kvm, &iter))
+		if (tdp_mmu_set_spte_atomic(kvm, &iter, SHADOW_NONPRESENT_VALUE))
 			goto retry;
+
+		flush = true;
 	}
 
+	if (flush)
+		kvm_flush_remote_tlbs_memslot(kvm, slot);
+
 	rcu_read_unlock();
 }
 
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v2 3/6] KVM: x86/mmu: Refactor TDP MMU iter need resched check

[David Matlack]

Refactor the TDP MMU iterator "need resched" checks into a helper
function so they can be called from a different code path in a
subsequent commit.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 27adbb3ecb02..9b8299ee4abb 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -646,6 +646,16 @@ static inline void tdp_mmu_iter_set_spte(struct kvm *kvm, struct tdp_iter *iter,
 #define tdp_mmu_for_each_pte(_iter, _mmu, _start, _end)		\
 	for_each_tdp_pte(_iter, root_to_sp(_mmu->root.hpa), _start, _end)
 
+static inline bool __must_check tdp_mmu_iter_need_resched(struct kvm *kvm,
+							  struct tdp_iter *iter)
+{
+	/* Ensure forward progress has been made before yielding. */
+	if (iter->next_last_level_gfn == iter->yielded_gfn)
+		return false;
+
+	return need_resched() || rwlock_needbreak(&kvm->mmu_lock);
+}
+
 /*
  * Yield if the MMU lock is contended or this thread needs to return control
  * to the scheduler.
@@ -666,11 +676,7 @@ static inline bool __must_check tdp_mmu_iter_cond_resched(struct kvm *kvm,
 {
 	WARN_ON_ONCE(iter->yielded);
 
-	/* Ensure forward progress has been made before yielding. */
-	if (iter->next_last_level_gfn == iter->yielded_gfn)
-		return false;
-
-	if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) {
+	if (tdp_mmu_iter_need_resched(kvm, iter)) {
 		if (flush)
 			kvm_flush_remote_tlbs(kvm);
 
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH v2 3/6] KVM: x86/mmu: Refactor TDP MMU iter need resched check

[Sean Christopherson]

On Fri, Aug 23, 2024, David Matlack wrote:
> Refactor the TDP MMU iterator "need resched" checks into a helper
> function so they can be called from a different code path in a
> subsequent commit.
> 
> No functional change intended.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 27adbb3ecb02..9b8299ee4abb 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -646,6 +646,16 @@ static inline void tdp_mmu_iter_set_spte(struct kvm *kvm, struct tdp_iter *iter,
>  #define tdp_mmu_for_each_pte(_iter, _mmu, _start, _end)		\
>  	for_each_tdp_pte(_iter, root_to_sp(_mmu->root.hpa), _start, _end)
>  
> +static inline bool __must_check tdp_mmu_iter_need_resched(struct kvm *kvm,
> +							  struct tdp_iter *iter)
> +{
> +	/* Ensure forward progress has been made before yielding. */
> +	if (iter->next_last_level_gfn == iter->yielded_gfn)
> +		return false;
> +
> +	return need_resched() || rwlock_needbreak(&kvm->mmu_lock);
> +}
> +
>  /*
>   * Yield if the MMU lock is contended or this thread needs to return control
>   * to the scheduler.
> @@ -666,11 +676,7 @@ static inline bool __must_check tdp_mmu_iter_cond_resched(struct kvm *kvm,
>  {
>  	WARN_ON_ONCE(iter->yielded);
>  
> -	/* Ensure forward progress has been made before yielding. */
> -	if (iter->next_last_level_gfn == iter->yielded_gfn)
> -		return false;
> -
> -	if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) {
> +	if (tdp_mmu_iter_need_resched(kvm, iter)) {

Huh.  There's a subtle behavioral change here, that I would not have noticed had
I not _just_ looked at this code[*].  Falling through means the "don't yield to
ensure forward progress case" would return iter->yielded, not %false.  Per the
WARN above, iter->yielded _should_ be false, but if KVM had a bug that caused it
to get stuck, then that bug would escalate into an even worse bug of putting KVM
into a potentially unbreakable infinite loop.

Which is extremely unlikely, but it's a good excuse to clean this up :-)  I'll
test and post the below, and plan on slotting it in before this patch (you might
even see it show up in kvm-x86 before it gets posted).

[*] https://lore.kernel.org/all/Zx-_cmV8ps7Y2fTe@google.com


From: Sean Christopherson <seanjc@google.com>
Date: Wed, 30 Oct 2024 16:28:31 -0700
Subject: [PATCH] KVM: x86/mmu: Check yielded_gfn for forward progress iff
 resched is needed

Swap the order of the checks in tdp_mmu_iter_cond_resched() so that KVM
checks to see if a resched is needed _before_ checking to see if yielding
must be disallowed to guarantee forward progress.  Iterating over TDP MMU
SPTEs is a hot path, e.g. tearing down a root can touch millions of SPTEs,
and not needing to reschedule is by far the common case.  On the other
handle, disallowing yielding because forward progress has not been made is
a very rare case.

Returning early for the common case (no resched), effectively reduces the
number of checks from 2 to 1 for the common case, and should make the code
slightly more predictable for the CPU.

To resolve a weird conundrum where the forward progress check currently
returns false, but the need resched check subtly returns iter->yielded,
which _should_ be false (enforced by a WARN), return false unconditionally
(which might also help make the sequence more predicatble).  If KVM has a
bug where iter->yielded is left danging, continuing to yield is neither
right nor wrong, it was simply an artifact of how the original code was
written.

Unconditionally returning false when yielding is unnecessary or unwanted
will also allow extracting the "should resched" logic to a separate helper
in a future patch.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 076343c3c8a7..8170b16b91c3 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -658,29 +658,29 @@ static inline bool __must_check tdp_mmu_iter_cond_resched(struct kvm *kvm,
 {
 	WARN_ON_ONCE(iter->yielded);
 
+	if (!need_resched() && !rwlock_needbreak(&kvm->mmu_lock))
+		return false;
+
 	/* Ensure forward progress has been made before yielding. */
 	if (iter->next_last_level_gfn == iter->yielded_gfn)
 		return false;
 
-	if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) {
-		if (flush)
-			kvm_flush_remote_tlbs(kvm);
+	if (flush)
+		kvm_flush_remote_tlbs(kvm);
 
-		rcu_read_unlock();
+	rcu_read_unlock();
 
-		if (shared)
-			cond_resched_rwlock_read(&kvm->mmu_lock);
-		else
-			cond_resched_rwlock_write(&kvm->mmu_lock);
+	if (shared)
+		cond_resched_rwlock_read(&kvm->mmu_lock);
+	else
+		cond_resched_rwlock_write(&kvm->mmu_lock);
 
-		rcu_read_lock();
+	rcu_read_lock();
 
-		WARN_ON_ONCE(iter->gfn > iter->next_last_level_gfn);
+	WARN_ON_ONCE(iter->gfn > iter->next_last_level_gfn);
 
-		iter->yielded = true;
-	}
-
-	return iter->yielded;
+	iter->yielded = true;
+	return true;
 }
 
 static inline gfn_t tdp_mmu_max_gfn_exclusive(void)

base-commit: 35ef80eb29ab5f7b7c7264c7f21a64b3aa046921
-- 

[PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[David Matlack]

Recover TDP MMU huge page mappings in-place instead of zapping them when
dirty logging is disabled, and rename functions that recover huge page
mappings when dirty logging is disabled to move away from the "zap
collapsible spte" terminology.

Before KVM flushes TLBs, guest accesses may be translated through either
the (stale) small SPTE or the (new) huge SPTE. This is already possible
when KVM is doing eager page splitting (where TLB flushes are also
batched), and when vCPUs are faulting in huge mappings (where TLBs are
flushed after the new huge SPTE is installed).

Recovering huge pages reduces the number of page faults when dirty
logging is disabled:

 $ perf stat -e kvm:kvm_page_fault -- ./dirty_log_perf_test -s anonymous_hugetlb_2mb -v 64 -e -b 4g

 Before: 393,599      kvm:kvm_page_fault
 After:  262,575      kvm:kvm_page_fault

vCPU throughput and the latency of disabling dirty-logging are about
equal compared to zapping, but avoiding faults can be beneficial to
remove vCPU jitter in extreme scenarios.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/include/asm/kvm_host.h |  4 +--
 arch/x86/kvm/mmu/mmu.c          |  6 ++--
 arch/x86/kvm/mmu/spte.c         | 39 +++++++++++++++++++++++--
 arch/x86/kvm/mmu/spte.h         |  1 +
 arch/x86/kvm/mmu/tdp_mmu.c      | 52 +++++++++++++++++++++++++++------
 arch/x86/kvm/mmu/tdp_mmu.h      |  4 +--
 arch/x86/kvm/x86.c              | 18 +++++-------
 7 files changed, 94 insertions(+), 30 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1811a42fa093..8f9bd7c0e139 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1953,8 +1953,8 @@ void kvm_mmu_try_split_huge_pages(struct kvm *kvm,
 				  const struct kvm_memory_slot *memslot,
 				  u64 start, u64 end,
 				  int target_level);
-void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
-				   const struct kvm_memory_slot *memslot);
+void kvm_mmu_recover_huge_pages(struct kvm *kvm,
+				const struct kvm_memory_slot *memslot);
 void kvm_mmu_slot_leaf_clear_dirty(struct kvm *kvm,
 				   const struct kvm_memory_slot *memslot);
 void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm, u64 gen);
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 2e92d9e9b311..2f8b1ebcbe9c 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6904,8 +6904,8 @@ static void kvm_rmap_zap_collapsible_sptes(struct kvm *kvm,
 		kvm_flush_remote_tlbs_memslot(kvm, slot);
 }
 
-void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
-				   const struct kvm_memory_slot *slot)
+void kvm_mmu_recover_huge_pages(struct kvm *kvm,
+				const struct kvm_memory_slot *slot)
 {
 	if (kvm_memslots_have_rmaps(kvm)) {
 		write_lock(&kvm->mmu_lock);
@@ -6915,7 +6915,7 @@ void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm,
 
 	if (tdp_mmu_enabled) {
 		read_lock(&kvm->mmu_lock);
-		kvm_tdp_mmu_zap_collapsible_sptes(kvm, slot);
+		kvm_tdp_mmu_recover_huge_pages(kvm, slot);
 		read_unlock(&kvm->mmu_lock);
 	}
 }
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 8f7eb3ad88fc..a12437bf6e0c 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -268,15 +268,14 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	return wrprot;
 }
 
-static u64 make_spte_executable(u64 spte)
+static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
 {
 	bool is_access_track = is_access_track_spte(spte);
 
 	if (is_access_track)
 		spte = restore_acc_track_spte(spte);
 
-	spte &= ~shadow_nx_mask;
-	spte |= shadow_x_mask;
+	spte = (spte | set) & ~clear;
 
 	if (is_access_track)
 		spte = mark_spte_for_access_track(spte);
@@ -284,6 +283,16 @@ static u64 make_spte_executable(u64 spte)
 	return spte;
 }
 
+static u64 make_spte_executable(u64 spte)
+{
+	return modify_spte_protections(spte, shadow_x_mask, shadow_nx_mask);
+}
+
+static u64 make_spte_nonexecutable(u64 spte)
+{
+	return modify_spte_protections(spte, shadow_nx_mask, shadow_x_mask);
+}
+
 /*
  * Construct an SPTE that maps a sub-page of the given huge page SPTE where
  * `index` identifies which sub-page.
@@ -320,6 +329,30 @@ u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte,
 	return child_spte;
 }
 
+u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
+{
+	u64 huge_spte;
+
+	if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
+		return SHADOW_NONPRESENT_VALUE;
+
+	if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
+		return SHADOW_NONPRESENT_VALUE;
+
+	huge_spte = small_spte | PT_PAGE_SIZE_MASK;
+
+	/*
+	 * huge_spte already has the address of the sub-page being collapsed
+	 * from small_spte, so just clear the lower address bits to create the
+	 * huge page address.
+	 */
+	huge_spte &= KVM_HPAGE_MASK(level) | ~PAGE_MASK;
+
+	if (is_nx_huge_page_enabled(kvm))
+		huge_spte = make_spte_nonexecutable(huge_spte);
+
+	return huge_spte;
+}
 
 u64 make_nonleaf_spte(u64 *child_pt, bool ad_disabled)
 {
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 2cb816ea2430..990d599eb827 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -503,6 +503,7 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	       bool host_writable, u64 *new_spte);
 u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte,
 		      	      union kvm_mmu_page_role role, int index);
+u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level);
 u64 make_nonleaf_spte(u64 *child_pt, bool ad_disabled);
 u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access);
 u64 mark_spte_for_access_track(u64 spte);
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 9b8299ee4abb..be70f0f22550 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1581,15 +1581,43 @@ void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm,
 		clear_dirty_pt_masked(kvm, root, gfn, mask, wrprot);
 }
 
-static void zap_collapsible_spte_range(struct kvm *kvm,
-				       struct kvm_mmu_page *root,
-				       const struct kvm_memory_slot *slot)
+static int tdp_mmu_make_huge_spte(struct kvm *kvm,
+				  struct tdp_iter *parent,
+				  u64 *huge_spte)
+{
+	struct kvm_mmu_page *root = spte_to_child_sp(parent->old_spte);
+	gfn_t start = parent->gfn;
+	gfn_t end = start + KVM_PAGES_PER_HPAGE(parent->level);
+	struct tdp_iter iter;
+
+	tdp_root_for_each_leaf_pte(iter, root, start, end) {
+		/*
+		 * Use the parent iterator when checking for forward progress so
+		 * that KVM doesn't get stuck continuously trying to yield (i.e.
+		 * returning -EAGAIN here and then failing the forward progress
+		 * check in the caller ad nauseam).
+		 */
+		if (tdp_mmu_iter_need_resched(kvm, parent))
+			return -EAGAIN;
+
+		*huge_spte = make_huge_spte(kvm, iter.old_spte, parent->level);
+		return 0;
+	}
+
+	return -ENOENT;
+}
+
+static void recover_huge_pages_range(struct kvm *kvm,
+				     struct kvm_mmu_page *root,
+				     const struct kvm_memory_slot *slot)
 {
 	gfn_t start = slot->base_gfn;
 	gfn_t end = start + slot->npages;
 	struct tdp_iter iter;
 	int max_mapping_level;
 	bool flush = false;
+	u64 huge_spte;
+	int r;
 
 	rcu_read_lock();
 
@@ -1626,7 +1654,13 @@ static void zap_collapsible_spte_range(struct kvm *kvm,
 		if (max_mapping_level < iter.level)
 			continue;
 
-		if (tdp_mmu_set_spte_atomic(kvm, &iter, SHADOW_NONPRESENT_VALUE))
+		r = tdp_mmu_make_huge_spte(kvm, &iter, &huge_spte);
+		if (r == -EAGAIN)
+			goto retry;
+		else if (r)
+			continue;
+
+		if (tdp_mmu_set_spte_atomic(kvm, &iter, huge_spte))
 			goto retry;
 
 		flush = true;
@@ -1639,17 +1673,17 @@ static void zap_collapsible_spte_range(struct kvm *kvm,
 }
 
 /*
- * Zap non-leaf SPTEs (and free their associated page tables) which could
- * be replaced by huge pages, for GFNs within the slot.
+ * Recover huge page mappings within the slot by replacing non-leaf SPTEs with
+ * huge SPTEs where possible.
  */
-void kvm_tdp_mmu_zap_collapsible_sptes(struct kvm *kvm,
-				       const struct kvm_memory_slot *slot)
+void kvm_tdp_mmu_recover_huge_pages(struct kvm *kvm,
+				    const struct kvm_memory_slot *slot)
 {
 	struct kvm_mmu_page *root;
 
 	lockdep_assert_held_read(&kvm->mmu_lock);
 	for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id)
-		zap_collapsible_spte_range(kvm, root, slot);
+		recover_huge_pages_range(kvm, root, slot);
 }
 
 /*
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index 1b74e058a81c..ddea2827d1ad 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -40,8 +40,8 @@ void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm,
 				       struct kvm_memory_slot *slot,
 				       gfn_t gfn, unsigned long mask,
 				       bool wrprot);
-void kvm_tdp_mmu_zap_collapsible_sptes(struct kvm *kvm,
-				       const struct kvm_memory_slot *slot);
+void kvm_tdp_mmu_recover_huge_pages(struct kvm *kvm,
+				    const struct kvm_memory_slot *slot);
 
 bool kvm_tdp_mmu_write_protect_gfn(struct kvm *kvm,
 				   struct kvm_memory_slot *slot, gfn_t gfn,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 966fb301d44b..3d09c12847d5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -13053,19 +13053,15 @@ static void kvm_mmu_slot_apply_flags(struct kvm *kvm,
 
 	if (!log_dirty_pages) {
 		/*
-		 * Dirty logging tracks sptes in 4k granularity, meaning that
-		 * large sptes have to be split.  If live migration succeeds,
-		 * the guest in the source machine will be destroyed and large
-		 * sptes will be created in the destination.  However, if the
-		 * guest continues to run in the source machine (for example if
-		 * live migration fails), small sptes will remain around and
-		 * cause bad performance.
+		 * Recover huge page mappings in the slot now that dirty logging
+		 * is disabled, i.e. now that KVM does not have to track guest
+		 * writes at 4KiB granularity.
 		 *
-		 * Scan sptes if dirty logging has been stopped, dropping those
-		 * which can be collapsed into a single large-page spte.  Later
-		 * page faults will create the large-page sptes.
+		 * Dirty logging might be disabled by userspace if an ongoing VM
+		 * live migration is cancelled and the VM must continue running
+		 * on the source.
 		 */
-		kvm_mmu_zap_collapsible_sptes(kvm, new);
+		kvm_mmu_recover_huge_pages(kvm, new);
 	} else {
 		/*
 		 * Initially-all-set does not require write protecting any page,
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Vipin Sharma]

On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@google.com> wrote:
> +static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
>  {
>         bool is_access_track = is_access_track_spte(spte);
>
>         if (is_access_track)
>                 spte = restore_acc_track_spte(spte);
>
> -       spte &= ~shadow_nx_mask;
> -       spte |= shadow_x_mask;
> +       spte = (spte | set) & ~clear;

We should add a check here WARN_ON_ONCE(set & clear) because if both
have a common bit set to 1 then the result  will be different between:
1. spte = (spt | set) & ~clear
2. spte = (spt | ~clear) & set

In the current form, 'clear' has more authority in the final value of spte.

>
> +u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
> +{
> +       u64 huge_spte;
> +
> +       if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
> +               return SHADOW_NONPRESENT_VALUE;
> +
> +       if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
> +               return SHADOW_NONPRESENT_VALUE;
> +

KVM_BUG_ON() is very aggressive. We should replace it with WARN_ON_ONCE()

Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Sean Christopherson]

On Wed, Oct 09, 2024, Vipin Sharma wrote:
> On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@google.com> wrote:
> > +static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
> >  {
> >         bool is_access_track = is_access_track_spte(spte);
> >
> >         if (is_access_track)
> >                 spte = restore_acc_track_spte(spte);
> >
> > -       spte &= ~shadow_nx_mask;
> > -       spte |= shadow_x_mask;
> > +       spte = (spte | set) & ~clear;
> 
> We should add a check here WARN_ON_ONCE(set & clear) because if both
> have a common bit set to 1 then the result  will be different between:
> 1. spte = (spt | set) & ~clear
> 2. spte = (spt | ~clear) & set
> 
> In the current form, 'clear' has more authority in the final value of spte.

KVM_MMU_WARN_ON(), overlapping @set and @clear is definitely something that should
be caught during development, i.e. we don't need to carry the WARN_ON_ONCE() in
production kernels

> > +u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
> > +{
> > +       u64 huge_spte;
> > +
> > +       if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
> > +               return SHADOW_NONPRESENT_VALUE;
> > +
> > +       if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
> > +               return SHADOW_NONPRESENT_VALUE;
> > +
> 
> KVM_BUG_ON() is very aggressive. We should replace it with WARN_ON_ONCE()

I'm tempted to say KVM_MMU_WARN_ON() here too.

Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Vipin Sharma]

On Wed, Oct 9, 2024 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Oct 09, 2024, Vipin Sharma wrote:
> > On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@google.com> wrote:
> > > +static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
> > >  {
> > >         bool is_access_track = is_access_track_spte(spte);
> > >
> > >         if (is_access_track)
> > >                 spte = restore_acc_track_spte(spte);
> > >
> > > -       spte &= ~shadow_nx_mask;
> > > -       spte |= shadow_x_mask;
> > > +       spte = (spte | set) & ~clear;
> >
> > We should add a check here WARN_ON_ONCE(set & clear) because if both
> > have a common bit set to 1 then the result  will be different between:
> > 1. spte = (spt | set) & ~clear
> > 2. spte = (spt | ~clear) & set
> >
> > In the current form, 'clear' has more authority in the final value of spte.
>
> KVM_MMU_WARN_ON(), overlapping @set and @clear is definitely something that should
> be caught during development, i.e. we don't need to carry the WARN_ON_ONCE() in
> production kernels
>
> > > +u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
> > > +{
> > > +       u64 huge_spte;
> > > +
> > > +       if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
> > > +               return SHADOW_NONPRESENT_VALUE;
> > > +
> > > +       if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
> > > +               return SHADOW_NONPRESENT_VALUE;
> > > +
> >
> > KVM_BUG_ON() is very aggressive. We should replace it with WARN_ON_ONCE()
>
> I'm tempted to say KVM_MMU_WARN_ON() here too.

I am fine with KVM_MMU_WARN_ON() here. Callers should check for the
value they provided and returned from this API and if it's important
to them in Production then decide on next steps accordingly.

Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Sean Christopherson]

On Wed, Oct 09, 2024, Vipin Sharma wrote:
> On Wed, Oct 9, 2024 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Oct 09, 2024, Vipin Sharma wrote:
> > > On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@google.com> wrote:
> > > > +static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
> > > >  {
> > > >         bool is_access_track = is_access_track_spte(spte);
> > > >
> > > >         if (is_access_track)
> > > >                 spte = restore_acc_track_spte(spte);
> > > >
> > > > -       spte &= ~shadow_nx_mask;
> > > > -       spte |= shadow_x_mask;
> > > > +       spte = (spte | set) & ~clear;
> > >
> > > We should add a check here WARN_ON_ONCE(set & clear) because if both
> > > have a common bit set to 1 then the result  will be different between:
> > > 1. spte = (spt | set) & ~clear
> > > 2. spte = (spt | ~clear) & set
> > >
> > > In the current form, 'clear' has more authority in the final value of spte.
> >
> > KVM_MMU_WARN_ON(), overlapping @set and @clear is definitely something that should
> > be caught during development, i.e. we don't need to carry the WARN_ON_ONCE() in
> > production kernels
> >
> > > > +u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
> > > > +{
> > > > +       u64 huge_spte;
> > > > +
> > > > +       if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
> > > > +               return SHADOW_NONPRESENT_VALUE;
> > > > +
> > > > +       if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
> > > > +               return SHADOW_NONPRESENT_VALUE;
> > > > +
> > >
> > > KVM_BUG_ON() is very aggressive. We should replace it with WARN_ON_ONCE()
> >
> > I'm tempted to say KVM_MMU_WARN_ON() here too.
> 
> I am fine with KVM_MMU_WARN_ON() here. Callers should check for the
> value they provided and returned from this API and if it's important
> to them in Production then decide on next steps accordingly.

Coming back to this, I opted to match the behavior of make_small_spte() and do:

	KVM_BUG_ON(!is_shadow_present_pte(small_spte) || level == PG_LEVEL_4K, kvm);

As explained in commit 3d4415ed75a57, the scenario is meant to be impossible.
If the check fails in production, odds are good there's SPTE memory corruption
and we _want_ to kill the VM.

    KVM: x86/mmu: Bug the VM if KVM tries to split a !hugepage SPTE
    
    Bug the VM instead of simply warning if KVM tries to split a SPTE that is
    non-present or not-huge.  KVM is guaranteed to end up in a broken state as
    the callers fully expect a valid SPTE, e.g. the shadow MMU will add an
    rmap entry, and all MMUs will account the expected small page.  Returning
    '0' is also technically wrong now that SHADOW_NONPRESENT_VALUE exists,
    i.e. would cause KVM to create a potential #VE SPTE.
    
    While it would be possible to have the callers gracefully handle failure,
    doing so would provide no practical value as the scenario really should be
    impossible, while the error handling would add a non-trivial amount of
    noise.

There's also no need to return SHADOW_NONPRESENT_VALUE.  KVM_BUG_ON() ensures
all vCPUs are kicked out of the guest, so while the return SPTE may be a bit
nonsensical, it will never be consumed by hardware.  Theoretically, KVM could
wander down a weird path in the future, but again, the most likely scenario is
that there was host memory corruption, so potential weird paths are the least of
KVM's worries at that point.

More importantly, in the _current_ code, returning SHADOW_NONPRESENT_VALUE happens
to be benign, but that's 100% due to make_huge_spte() only being used by the TDP
MMU.  If the shaduw MMU ever started using make_huge_spte(), returning a !present
SPTE would be all but guaranteed to cause fatal problems.

Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Vipin Sharma]

On Wed, Oct 30, 2024 at 4:42 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Oct 09, 2024, Vipin Sharma wrote:
>
> Coming back to this, I opted to match the behavior of make_small_spte() and do:
>
>         KVM_BUG_ON(!is_shadow_present_pte(small_spte) || level == PG_LEVEL_4K, kvm);
>

Should these be two separate KVM_BUG_ON(), to aid in debugging?

> As explained in commit 3d4415ed75a57, the scenario is meant to be impossible.
> If the check fails in production, odds are good there's SPTE memory corruption
> and we _want_ to kill the VM.
>
>     KVM: x86/mmu: Bug the VM if KVM tries to split a !hugepage SPTE
>
>     Bug the VM instead of simply warning if KVM tries to split a SPTE that is
>     non-present or not-huge.  KVM is guaranteed to end up in a broken state as
>     the callers fully expect a valid SPTE, e.g. the shadow MMU will add an
>     rmap entry, and all MMUs will account the expected small page.  Returning
>     '0' is also technically wrong now that SHADOW_NONPRESENT_VALUE exists,
>     i.e. would cause KVM to create a potential #VE SPTE.
>
>     While it would be possible to have the callers gracefully handle failure,
>     doing so would provide no practical value as the scenario really should be
>     impossible, while the error handling would add a non-trivial amount of
>     noise.
>
> There's also no need to return SHADOW_NONPRESENT_VALUE.  KVM_BUG_ON() ensures
> all vCPUs are kicked out of the guest, so while the return SPTE may be a bit
> nonsensical, it will never be consumed by hardware.  Theoretically, KVM could
> wander down a weird path in the future, but again, the most likely scenario is
> that there was host memory corruption, so potential weird paths are the least of
> KVM's worries at that point.
>
> More importantly, in the _current_ code, returning SHADOW_NONPRESENT_VALUE happens
> to be benign, but that's 100% due to make_huge_spte() only being used by the TDP
> MMU.  If the shaduw MMU ever started using make_huge_spte(), returning a !present
> SPTE would be all but guaranteed to cause fatal problems.

I think the caller should be given the opportunity to handle a
failure. In the current code, TDP is able to handle the error
condition, so penalizing a VM seems wrong. We have gone from a state
of reduced performance to either very good performance or VM being
killed.

If shadow MMU starts using make_huge_spte() and doesn't add logic to
handle this scenario (killing vm or something else) then that is a
coding bug of that feature which should be fixed.

Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Sean Christopherson]

On Fri, Nov 01, 2024, Vipin Sharma wrote:
> On Wed, Oct 30, 2024 at 4:42 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Oct 09, 2024, Vipin Sharma wrote:
> >
> > Coming back to this, I opted to match the behavior of make_small_spte() and do:
> >
> >         KVM_BUG_ON(!is_shadow_present_pte(small_spte) || level == PG_LEVEL_4K, kvm);
> >
> 
> Should these be two separate KVM_BUG_ON(), to aid in debugging?

I smushed them together to may make_huge_page_split_spte()'s style, and because
practically speaking the assertion will never fail.  And if it does fail, figuring
out what failed wil be quite easy as the two variables being checked are function
parameters, i.e. are all but guaranteed to be in RSI and RDX.

> > As explained in commit 3d4415ed75a57, the scenario is meant to be impossible.
> > If the check fails in production, odds are good there's SPTE memory corruption
> > and we _want_ to kill the VM.
> >
> >     KVM: x86/mmu: Bug the VM if KVM tries to split a !hugepage SPTE
> >
> >     Bug the VM instead of simply warning if KVM tries to split a SPTE that is
> >     non-present or not-huge.  KVM is guaranteed to end up in a broken state as
> >     the callers fully expect a valid SPTE, e.g. the shadow MMU will add an
> >     rmap entry, and all MMUs will account the expected small page.  Returning
> >     '0' is also technically wrong now that SHADOW_NONPRESENT_VALUE exists,
> >     i.e. would cause KVM to create a potential #VE SPTE.
> >
> >     While it would be possible to have the callers gracefully handle failure,
> >     doing so would provide no practical value as the scenario really should be
> >     impossible, while the error handling would add a non-trivial amount of
> >     noise.
> >
> > There's also no need to return SHADOW_NONPRESENT_VALUE.  KVM_BUG_ON() ensures
> > all vCPUs are kicked out of the guest, so while the return SPTE may be a bit
> > nonsensical, it will never be consumed by hardware.  Theoretically, KVM could
> > wander down a weird path in the future, but again, the most likely scenario is
> > that there was host memory corruption, so potential weird paths are the least of
> > KVM's worries at that point.
> >
> > More importantly, in the _current_ code, returning SHADOW_NONPRESENT_VALUE happens
> > to be benign, but that's 100% due to make_huge_spte() only being used by the TDP
> > MMU.  If the shaduw MMU ever started using make_huge_spte(), returning a !present
> > SPTE would be all but guaranteed to cause fatal problems.
> 
> I think the caller should be given the opportunity to handle a
> failure. In the current code, TDP is able to handle the error
> condition, so penalizing a VM seems wrong. We have gone from a state
> of reduced performance to either very good performance or VM being
> killed.

The context of how the failure can happens matters.  The only way this helper can
fail is if there is a blatant bug in the caller, or if there is data corruption
of some form.  The use of KVM_BUG_ON() is a very clear signal to developers and
readers that the caller _must_ ensure the SPTE is shadow-present SPTE, and that
the new SPTE will be a huge SPTE.  Critically, treating bad input as fatal to the
VM also allow the caller to assume success.

> If shadow MMU starts using make_huge_spte() and doesn't add logic to
> handle this scenario (killing vm or something else) then that is a
> coding bug of that feature which should be fixed.

No, because allowing make_huge_spte() to fail, even if there is a WARN, adds
non-trivial complexity with zero real-world benefit.  At some point, success must
be assumed/guaranteed.  Forcing a future developer to think about the best way to
handle a failure that "can't" happen is a waste of their time.

E.g. as an example where allowing failure is both more absurd and more painful,
imagine if kvm_mmu_prepare_zap_page() return an error if mmu_lock weren't held.
Trying to gracefully handle error would be madness, and so it simply asserts that
mmu_lock is held.

[PATCH v2 5/6] KVM: x86/mmu: Rename make_huge_page_split_spte() to make_small_spte()

[David Matlack]

Rename make_huge_page_split_spte() to make_small_spte(). This ensures
that the usage of "small_spte" and "huge_spte" are consistent between
make_huge_spte() and make_small_spte().

This should also reduce some confusion as make_huge_page_split_spte()
almost reads like it will create a huge SPTE, when in fact it is
creating a small SPTE to split the huge SPTE.

No functional change intended.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/mmu.c     | 2 +-
 arch/x86/kvm/mmu/spte.c    | 4 ++--
 arch/x86/kvm/mmu/spte.h    | 4 ++--
 arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 2f8b1ebcbe9c..8967508b63f9 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6693,7 +6693,7 @@ static void shadow_mmu_split_huge_page(struct kvm *kvm,
 			continue;
 		}
 
-		spte = make_huge_page_split_spte(kvm, huge_spte, sp->role, index);
+		spte = make_small_spte(kvm, huge_spte, sp->role, index);
 		mmu_spte_set(sptep, spte);
 		__rmap_add(kvm, cache, slot, sptep, gfn, sp->role.access);
 	}
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index a12437bf6e0c..fe010e3404b1 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -300,8 +300,8 @@ static u64 make_spte_nonexecutable(u64 spte)
  * This is used during huge page splitting to build the SPTEs that make up the
  * new page table.
  */
-u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte,
-			      union kvm_mmu_page_role role, int index)
+u64 make_small_spte(struct kvm *kvm, u64 huge_spte,
+		    union kvm_mmu_page_role role, int index)
 {
 	u64 child_spte = huge_spte;
 
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 990d599eb827..3aee16e0a575 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -501,8 +501,8 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	       unsigned int pte_access, gfn_t gfn, kvm_pfn_t pfn,
 	       u64 old_spte, bool prefetch, bool can_unsync,
 	       bool host_writable, u64 *new_spte);
-u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte,
-		      	      union kvm_mmu_page_role role, int index);
+u64 make_small_spte(struct kvm *kvm, u64 huge_spte,
+		    union kvm_mmu_page_role role, int index);
 u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level);
 u64 make_nonleaf_spte(u64 *child_pt, bool ad_disabled);
 u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access);
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index be70f0f22550..4c1cd41750ad 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1334,7 +1334,7 @@ static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
 	 * not been linked in yet and thus is not reachable from any other CPU.
 	 */
 	for (i = 0; i < SPTE_ENT_PER_PAGE; i++)
-		sp->spt[i] = make_huge_page_split_spte(kvm, huge_spte, sp->role, i);
+		sp->spt[i] = make_small_spte(kvm, huge_spte, sp->role, i);
 
 	/*
 	 * Replace the huge spte with a pointer to the populated lower level
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v2 6/6] KVM: x86/mmu: WARN if huge page recovery triggered during dirty logging

[David Matlack]

WARN and bail out of recover_huge_pages_range() if dirty logging is
enabled. KVM shouldn't be recovering huge pages during dirty logging
anyway, since KVM needs to track writes at 4KiB. However its not out of
the possibility that that changes in the future.

If KVM wants to recover huge pages during dirty logging,
make_huge_spte() must be updated to write-protect the new huge page
mapping. Otherwise, writes through the newly recovered huge page mapping
will not be tracked.

Note that this potential risk did not exist back when KVM zapped to
recover huge page mappings, since subsequent accesses would just be
faulted in at PG_LEVEL_4K if dirty logging was enabled.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 4c1cd41750ad..301a2c19bfe9 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1619,6 +1619,9 @@ static void recover_huge_pages_range(struct kvm *kvm,
 	u64 huge_spte;
 	int r;
 
+	if (WARN_ON_ONCE(kvm_slot_dirty_track_enabled(slot)))
+		return;
+
 	rcu_read_lock();
 
 	for_each_tdp_pte_min_level(iter, root, PG_LEVEL_2M, start, end) {
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH v2 0/6] KVM: x86/mmu: Optimize TDP MMU huge page recovery during disable-dirty-log

[Sean Christopherson]

On Fri, Aug 23, 2024, David Matlack wrote:
> Rework the TDP MMU disable-dirty-log path to batch TLB flushes and
> recover huge page mappings, rather than zapping and flushing for every
> potential huge page mapping.
> 
> With this series, dirty_log_perf_test shows a decrease in the time it takes to
> disable dirty logging, as well as a decrease in the number of vCPU faults:
> 
>  $ ./dirty_log_perf_test -s anonymous_hugetlb_2mb -v 64 -e -b 4g
> 
>  Before: Disabling dirty logging time: 14.334453428s (131072 flushes)
>  After:  Disabling dirty logging time: 4.794969689s  (76 flushes)
> 
>  Before: 393,599      kvm:kvm_page_fault
>  After:  262,575      kvm:kvm_page_fault
> 
> v2:
>  - Use a separate iterator to walk down to child SPTEs during huge page
>    recovery [Sean]
>  - Return SHADOW_NONPRESENT_VALUE in error conditions in
>    make_huge_spte() [Vipin][off-list]
> 
> v1: https://lore.kernel.org/kvm/20240805233114.4060019-8-dmatlack@google.com/
> 
> David Matlack (6):
>   KVM: x86/mmu: Drop @max_level from kvm_mmu_max_mapping_level()
>   KVM: x86/mmu: Batch TLB flushes when zapping collapsible TDP MMU SPTEs
>   KVM: x86/mmu: Refactor TDP MMU iter need resched check
>   KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of
>     zapping
>   KVM: x86/mmu: Rename make_huge_page_split_spte() to make_small_spte()
>   KVM: x86/mmu: WARN if huge page recovery triggered during dirty
>     logging
> 
>  arch/x86/include/asm/kvm_host.h |   4 +-
>  arch/x86/kvm/mmu/mmu.c          |  16 ++--
>  arch/x86/kvm/mmu/mmu_internal.h |   3 +-
>  arch/x86/kvm/mmu/spte.c         |  43 +++++++++--
>  arch/x86/kvm/mmu/spte.h         |   5 +-
>  arch/x86/kvm/mmu/tdp_mmu.c      | 129 +++++++++++++++++---------------
>  arch/x86/kvm/mmu/tdp_mmu.h      |   4 +-
>  arch/x86/kvm/x86.c              |  18 ++---
>  8 files changed, 128 insertions(+), 94 deletions(-)

FYI, these are sitting in kvm-x86 mmu, but will be rebased next week, at which
point I'll send the "official" thank yous.

[1/8] KVM: x86/mmu: Drop @max_level from kvm_mmu_max_mapping_level()
      https://github.com/kvm-x86/linux/commit/8ccd51cb5911
[2/8] KVM: x86/mmu: Batch TLB flushes when zapping collapsible TDP MMU SPTEs
      https://github.com/kvm-x86/linux/commit/35ef80eb29ab
[3/8] KVM: x86/mmu: Check yielded_gfn for forward progress iff resched is needed
      https://github.com/kvm-x86/linux/commit/d400ce271d9c
[4/8] KVM: x86/mmu: Demote the WARN on yielded in xxx_cond_resched() to KVM_MMU_WARN_ON
      https://github.com/kvm-x86/linux/commit/012a5c17cba4
[5/8] KVM: x86/mmu: Refactor TDP MMU iter need resched check
      https://github.com/kvm-x86/linux/commit/cb059b9e2432
[6/8] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping
      https://github.com/kvm-x86/linux/commit/13237fb40c74
[7/8] KVM: x86/mmu: Rename make_huge_page_split_spte() to make_small_spte()
      https://github.com/kvm-x86/linux/commit/1d2a6d0b6438
[8/8] KVM: x86/mmu: WARN if huge page recovery triggered during dirty logging
      https://github.com/kvm-x86/linux/commit/ed5ca61d995f

Re: [PATCH v2 0/6] KVM: x86/mmu: Optimize TDP MMU huge page recovery during disable-dirty-log

[Sean Christopherson]

On Fri, 23 Aug 2024 16:56:42 -0700, David Matlack wrote:
> Rework the TDP MMU disable-dirty-log path to batch TLB flushes and
> recover huge page mappings, rather than zapping and flushing for every
> potential huge page mapping.
> 
> With this series, dirty_log_perf_test shows a decrease in the time it takes to
> disable dirty logging, as well as a decrease in the number of vCPU faults:
> 
> [...]

Applied to kvm-x86 mmu, thanks!  Note, these won't be contiguous due to the
insertion of the other patches, but the hashes should be correct (knock wood).

[1/6] KVM: x86/mmu: Drop @max_level from kvm_mmu_max_mapping_level()
      https://github.com/kvm-x86/linux/commit/8ccd51cb5911
[2/6] KVM: x86/mmu: Batch TLB flushes when zapping collapsible TDP MMU SPTEs
      https://github.com/kvm-x86/linux/commit/35ef80eb29ab
[3/6] KVM: x86/mmu: Refactor TDP MMU iter need resched check
      https://github.com/kvm-x86/linux/commit/dd2e7dbc4ae2
[4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping
      https://github.com/kvm-x86/linux/commit/13e2e4f62a4b
[5/6] KVM: x86/mmu: Rename make_huge_page_split_spte() to make_small_spte()
      https://github.com/kvm-x86/linux/commit/430e264b7653
[6/6] KVM: x86/mmu: WARN if huge page recovery triggered during dirty logging
      https://github.com/kvm-x86/linux/commit/06c4cd957b5c

--
https://github.com/kvm-x86/linux/tree/next