[PATCH 0/2] KVM: x86/mmu: Use LOCK CMPXCHG to clear Accessed bits

[PATCH 0/2] KVM: x86/mmu: Use LOCK CMPXCHG to clear Accessed bits

[Sean Christopherson]

Use LOCK CMPXCHG instead of LOCK AND or LOCK BTR to clear Accessed bits when
aging SPTEs, as LOCK AND could clobber a !PRESENT SPTE.  Specificaly, as
flagged by AI of some kind, a LOCK AND in the TDP MMU could clobber a
FROZEN_SPTE and ultimately result in a false negative since the Accessed bit
is included in FROZEN_SPTE.

I initially was planning on tweaking the FROZEN_SPTE value to make it ok to
use LOCK AND on Accessed bits, but AFAICT there's no meaningful downside to
using CMPXCHG.

Sean Christopherson (2):
  KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in TDP MMU
  KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in the shadow MMU

 arch/x86/kvm/mmu/mmu.c      | 27 ++++++++++++++-------------
 arch/x86/kvm/mmu/tdp_iter.h |  7 +++++++
 arch/x86/kvm/mmu/tdp_mmu.c  | 20 +++++++++-----------
 3 files changed, 30 insertions(+), 24 deletions(-)


base-commit: 271255273d5ff348fe29d89fe4712b2f7f7907c3
-- 
2.55.0.229.g6434b31f56-goog

[PATCH 1/2] KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in TDP MMU

[Sean Christopherson]

Use LOCK CMPXCHG instead of LOCK AND to clear the Accessed bit when aging
SPTEs in the TDP MMU, as doing a LOCK AND can corrupt a FROZEN SPTE and
allow a third CPU to effectively overwrite the FROZEN SPTE.  As pointed
out by AI of some kind, because the magic FROZEN_SPTE value is a "full"
SPTE, not a single bit, and includes the Accessed bit, clearing the
Accessed bit in a FROZEN SPTE will result in is_frozen_spte() getting a
false negative.

E.g. if CPU0 freezes an SPTE, and CPU1 clears the Accessed bitin the frozen
SPTE, then CPU2 could come along and overwrite the frozen SPTE with a
shadow-present SPTE.

Thankfully, the false negative is largely benign, because outside of TDX,
which doesn't support aging, KVM only freezes leaf SPTEs when removing an
upper level shadow page.  So while KVM could clobber a frozen SPTE back to
a shadow-present SPTE, and could even use the new SPTE, the subsequent TLB
flush will make the orphaned, shadow-present SPTE unreachable.  Failure to
ever zap the orphaned leaf SPTE would show up in KVM's stats, but otherwise
is benign (because KVM no longer keeps an elevated refcount for leaf SPTEs).

Opportunistically add a comment to warn future developers away from using
kvm_tdp_mmu_write_spte_atomic() and tdp_mmu_clear_spte_bits_atomic(), as
they are generally unsafe.  Keep the helpers, e.g. instead of open-coding
the atomic64_fetch_and() in tdp_mmu_clear_spte_bits(), as scary warnings
usually are more effective deterrent against recidivism than removal of the
dangerous code.

Alternatively, KVM could use different bits for the magic FROZEN_SPTE value,
e.g. setting the Dirty bits (with effective IPAT and Global aliases) would
likely be "ok", as IPAT/Global are extremely unlikely to be cleared without
doing a full SPTE write, and KVM's clearing of Dirty bits shares logic with
Write-Protection, which must do a full SPTE write (via cmpxchg64() in the
TDP MMU) to ensure KVM isn't clobbering state.  But there is zero reason to
carry that risk (beyond stubbornness in wanting to preserve a "cute" idea),
as the cost of LOCK CMPXCHG and LOCK AND are within 1-2 uops of each other
on modern hardware.

Fixes: b146a9b34aed ("KVM: x86/mmu: Age TDP MMU SPTEs without holding mmu_lock")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/tdp_iter.h |  7 +++++++
 arch/x86/kvm/mmu/tdp_mmu.c  | 20 +++++++++-----------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h
index 364c5da6c499..f898d8d0d93c 100644
--- a/arch/x86/kvm/mmu/tdp_iter.h
+++ b/arch/x86/kvm/mmu/tdp_iter.h
@@ -19,6 +19,13 @@ static inline u64 kvm_tdp_mmu_read_spte(tdp_ptep_t sptep)
 	return READ_ONCE(*rcu_dereference(sptep));
 }
 
+/*
+ * WARNING!  mmu_lock must be held for write when using the "write atomic" or
+ * "clear bits atomic" APIs, otherwise KVM could overwrite the "wrong" old SPTE
+ * value, i.e. clobber an update from a different CPU.  The only exception is
+ * when KVM is freezing a leaf SPTE for removal, in which case KVM doesn't care
+ * about the exact old SPTE value (KVM will react to the actual old value).
+ */
 static inline u64 kvm_tdp_mmu_write_spte_atomic(tdp_ptep_t sptep, u64 new_spte)
 {
 	KVM_MMU_WARN_ON(is_ept_ve_possible(new_spte));
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index ce3f2efadb05..44dad106fad1 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1335,19 +1335,17 @@ static void kvm_tdp_mmu_age_spte(struct kvm *kvm, struct tdp_iter *iter)
 	if (WARN_ON_ONCE(is_mirror_sptep(iter->sptep)))
 		return;
 
-	if (spte_ad_enabled(iter->old_spte)) {
-		iter->old_spte = tdp_mmu_clear_spte_bits_atomic(iter->sptep,
-								shadow_accessed_mask);
+	if (spte_ad_enabled(iter->old_spte))
 		new_spte = iter->old_spte & ~shadow_accessed_mask;
-	} else {
+	else
 		new_spte = mark_spte_for_access_track(iter->old_spte);
-		/*
-		 * It is safe for the following cmpxchg to fail. Leave the
-		 * Accessed bit set, as the spte is most likely young anyway.
-		 */
-		if (__tdp_mmu_set_spte_atomic(kvm, iter, new_spte))
-			return;
-	}
+
+	/*
+	 * Don't bother retrying if another CPU modified the SPTE, the SPTE is
+	 * either being zapped or is likely still in-use, i.e. is still young.
+	 */
+	if (__tdp_mmu_set_spte_atomic(kvm, iter, new_spte))
+		return;
 
 	trace_kvm_tdp_mmu_spte_changed(iter->as_id, iter->gfn, iter->level,
 				       iter->old_spte, new_spte);
-- 
2.55.0.229.g6434b31f56-goog

Re: [PATCH 1/2] KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in TDP MMU

[James Houghton]

On Mon, Jul 27, 2026 at 5:23 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Use LOCK CMPXCHG instead of LOCK AND to clear the Accessed bit when aging
> SPTEs in the TDP MMU, as doing a LOCK AND can corrupt a FROZEN SPTE and
> allow a third CPU to effectively overwrite the FROZEN SPTE.  As pointed
> out by AI of some kind, because the magic FROZEN_SPTE value is a "full"
> SPTE, not a single bit, and includes the Accessed bit, clearing the
> Accessed bit in a FROZEN SPTE will result in is_frozen_spte() getting a
> false negative.
>
> E.g. if CPU0 freezes an SPTE, and CPU1 clears the Accessed bitin the frozen
> SPTE, then CPU2 could come along and overwrite the frozen SPTE with a
> shadow-present SPTE.
>
> Thankfully, the false negative is largely benign, because outside of TDX,
> which doesn't support aging, KVM only freezes leaf SPTEs when removing an
> upper level shadow page.  So while KVM could clobber a frozen SPTE back to
> a shadow-present SPTE, and could even use the new SPTE, the subsequent TLB
> flush will make the orphaned, shadow-present SPTE unreachable.  Failure to
> ever zap the orphaned leaf SPTE would show up in KVM's stats, but otherwise
> is benign (because KVM no longer keeps an elevated refcount for leaf SPTEs).
>
> Opportunistically add a comment to warn future developers away from using
> kvm_tdp_mmu_write_spte_atomic() and tdp_mmu_clear_spte_bits_atomic(), as
> they are generally unsafe.  Keep the helpers, e.g. instead of open-coding
> the atomic64_fetch_and() in tdp_mmu_clear_spte_bits(), as scary warnings
> usually are more effective deterrent against recidivism than removal of the
> dangerous code.
>
> Alternatively, KVM could use different bits for the magic FROZEN_SPTE value,
> e.g. setting the Dirty bits (with effective IPAT and Global aliases) would
> likely be "ok", as IPAT/Global are extremely unlikely to be cleared without
> doing a full SPTE write, and KVM's clearing of Dirty bits shares logic with
> Write-Protection, which must do a full SPTE write (via cmpxchg64() in the
> TDP MMU) to ensure KVM isn't clobbering state.  But there is zero reason to
> carry that risk (beyond stubbornness in wanting to preserve a "cute" idea),
> as the cost of LOCK CMPXCHG and LOCK AND are within 1-2 uops of each other
> on modern hardware.
>
> Fixes: b146a9b34aed ("KVM: x86/mmu: Age TDP MMU SPTEs without holding mmu_lock")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Thanks for this. Feel free to add:

Reviewed-by: James Houghton <jthoughton@google.com>

Re: [PATCH 1/2] KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in TDP MMU

[Huang, Kai]

On Mon, 2026-07-27 at 17:22 -0700, Sean Christopherson wrote:
> Use LOCK CMPXCHG instead of LOCK AND to clear the Accessed bit when aging
> SPTEs in the TDP MMU, as doing a LOCK AND can corrupt a FROZEN SPTE and
> allow a third CPU to effectively overwrite the FROZEN SPTE.  As pointed
> out by AI of some kind, because the magic FROZEN_SPTE value is a "full"
> SPTE, not a single bit, and includes the Accessed bit, clearing the
> Accessed bit in a FROZEN SPTE will result in is_frozen_spte() getting a
> false negative.
> 
> E.g. if CPU0 freezes an SPTE, and CPU1 clears the Accessed bitin the frozen
							     ^
							     bit in

> SPTE, then CPU2 could come along and overwrite the frozen SPTE with a
> shadow-present SPTE.
> 
> Thankfully, the false negative is largely benign, because outside of TDX,
> which doesn't support aging, KVM only freezes leaf SPTEs when removing an
> upper level shadow page.  So while KVM could clobber a frozen SPTE back to
> a shadow-present SPTE, and could even use the new SPTE, the subsequent TLB
> flush will make the orphaned, shadow-present SPTE unreachable.  Failure to
> ever zap the orphaned leaf SPTE would show up in KVM's stats, but otherwise
> is benign (because KVM no longer keeps an elevated refcount for leaf SPTEs).
> 
> Opportunistically add a comment to warn future developers away from using
> kvm_tdp_mmu_write_spte_atomic() and tdp_mmu_clear_spte_bits_atomic(), as
> they are generally unsafe.  Keep the helpers, e.g. instead of open-coding
> the atomic64_fetch_and() in tdp_mmu_clear_spte_bits(), as scary warnings
> usually are more effective deterrent against recidivism than removal of the
> dangerous code.
> 
> Alternatively, KVM could use different bits for the magic FROZEN_SPTE value,
> e.g. setting the Dirty bits (with effective IPAT and Global aliases) would
> likely be "ok", as IPAT/Global are extremely unlikely to be cleared without
> doing a full SPTE write, and KVM's clearing of Dirty bits shares logic with
> Write-Protection, which must do a full SPTE write (via cmpxchg64() in the
> TDP MMU) to ensure KVM isn't clobbering state.  But there is zero reason to
> carry that risk (beyond stubbornness in wanting to preserve a "cute" idea),
> as the cost of LOCK CMPXCHG and LOCK AND are within 1-2 uops of each other
> on modern hardware.
> 
> Fixes: b146a9b34aed ("KVM: x86/mmu: Age TDP MMU SPTEs without holding mmu_lock")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

[PATCH 2/2] KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in the shadow MMU

[Sean Christopherson]

Use CMPXCHG instead of clear_bit(), which currently emits a LOCK BTR since
the to-be-cleared bit isn't a compiled-time constant, when aging SPTEs in
the shadow MMU to align with the approach taken by the TDP MMU, and because
using CMPXCHG is far more robust against bugs in KVM.  E.g. if the SPTE is
somehow no longer an SPTE due to a KVM bug, CMPXCHG will fail gracefully,
whereas clear_bit() would potentially corrupt/clobber memory.

Clearing the Accessed bit without atomically ensuring the SPTE is still the
old SPTE is "fine", as holding the rmap's lock ensures zapping the old SPTE
can't fully complete, which in turn ensures a new, different SPTE can't be
installed.  But that chain of logic isn't exactly obvious, and there's zero
reason to avoid CMPXCHG as its cost on modern hardware is within ~1-2 uops
of LOCK BTR (and may even be cheaper on some microarchitectures).  Doing a
64-bit CMPXCHG on 32-bit kernels does requires a more expensive CMPXCHG8B,
but 32-bit KVM is all but dead at this point.

Cc: James Houghton <jthoughton@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index ecf9e39aed5a..c519e8e8d646 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1719,11 +1719,11 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
 	struct kvm_rmap_head *rmap_head;
 	struct rmap_iterator iter;
 	unsigned long rmap_val;
+	u64 old_spte, new_spte;
 	bool young = false;
 	u64 *sptep;
 	gfn_t gfn;
 	int level;
-	u64 spte;
 
 	for (level = PG_LEVEL_4K; level <= KVM_MAX_HUGEPAGE_LEVEL; level++) {
 		for (gfn = range->start; gfn < range->end;
@@ -1731,8 +1731,8 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
 			rmap_head = gfn_to_rmap(gfn, level, range->slot);
 			rmap_val = kvm_rmap_lock_readonly(rmap_head);
 
-			for_each_rmap_spte_lockless(rmap_val, &iter, sptep, spte) {
-				if (!is_accessed_spte(spte))
+			for_each_rmap_spte_lockless(rmap_val, &iter, sptep, old_spte) {
+				if (!is_accessed_spte(old_spte))
 					continue;
 
 				if (test_only) {
@@ -1740,17 +1740,18 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
 					return true;
 				}
 
-				if (spte_ad_enabled(spte))
-					clear_bit((ffs(shadow_accessed_mask) - 1),
-						  (unsigned long *)sptep);
+				if (spte_ad_enabled(old_spte))
+					new_spte = old_spte & ~shadow_accessed_mask;
 				else
-					/*
-					 * If the following cmpxchg fails, the
-					 * spte is being concurrently modified
-					 * and should most likely stay young.
-					 */
-					cmpxchg64(sptep, spte,
-					      mark_spte_for_access_track(spte));
+					new_spte = mark_spte_for_access_track(old_spte);
+
+				/*
+				 * Don't bother retrying if the CMPXCHG fails,
+				 * i.e. if another CPU modified the SPTE.  The
+				 * SPTE is either being zapped or is likely
+				 * still in-use, i.e. is still young.
+				 */
+				cmpxchg64(sptep, old_spte, new_spte);
 				young = true;
 			}
 
-- 
2.55.0.229.g6434b31f56-goog

Re: [PATCH 2/2] KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in the shadow MMU

[James Houghton]

On Mon, Jul 27, 2026 at 5:23 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Use CMPXCHG instead of clear_bit(), which currently emits a LOCK BTR since
> the to-be-cleared bit isn't a compiled-time constant, when aging SPTEs in
> the shadow MMU to align with the approach taken by the TDP MMU, and because
> using CMPXCHG is far more robust against bugs in KVM.  E.g. if the SPTE is
> somehow no longer an SPTE due to a KVM bug, CMPXCHG will fail gracefully,
> whereas clear_bit() would potentially corrupt/clobber memory.
>
> Clearing the Accessed bit without atomically ensuring the SPTE is still the
> old SPTE is "fine", as holding the rmap's lock ensures zapping the old SPTE
> can't fully complete, which in turn ensures a new, different SPTE can't be
> installed.  But that chain of logic isn't exactly obvious, and there's zero
> reason to avoid CMPXCHG as its cost on modern hardware is within ~1-2 uops
> of LOCK BTR (and may even be cheaper on some microarchitectures).  Doing a
> 64-bit CMPXCHG on 32-bit kernels does requires a more expensive CMPXCHG8B,
> but 32-bit KVM is all but dead at this point.
>
> Cc: James Houghton <jthoughton@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: James Houghton <jthoughton@google.com>

Re: [PATCH 2/2] KVM: x86/mmu: Use CMPXCHG when clearing Accessed bit in the shadow MMU

[Huang, Kai]

On Mon, 2026-07-27 at 17:22 -0700, Sean Christopherson wrote:
> Use CMPXCHG instead of clear_bit(), which currently emits a LOCK BTR since
> the to-be-cleared bit isn't a compiled-time constant, when aging SPTEs in
				^
				compile-time ?

> the shadow MMU to align with the approach taken by the TDP MMU, and because
> using CMPXCHG is far more robust against bugs in KVM.  E.g. if the SPTE is
> somehow no longer an SPTE due to a KVM bug, CMPXCHG will fail gracefully,
> whereas clear_bit() would potentially corrupt/clobber memory.
> 
> Clearing the Accessed bit without atomically ensuring the SPTE is still the
> old SPTE is "fine", as holding the rmap's lock ensures zapping the old SPTE
> can't fully complete, which in turn ensures a new, different SPTE can't be
> installed.  But that chain of logic isn't exactly obvious, and there's zero
> reason to avoid CMPXCHG as its cost on modern hardware is within ~1-2 uops
> of LOCK BTR (and may even be cheaper on some microarchitectures).  Doing a
> 64-bit CMPXCHG on 32-bit kernels does requires a more expensive CMPXCHG8B,
					^
					require

> but 32-bit KVM is all but dead at this point.
> 
> Cc: James Houghton <jthoughton@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 0/2] KVM: x86/mmu: Use LOCK CMPXCHG to clear Accessed bits

[Huang, Kai]

On Mon, 2026-07-27 at 17:22 -0700, Sean Christopherson wrote:
> I initially was planning on tweaking the FROZEN_SPTE value to make it ok to
> use LOCK AND on Accessed bits, but AFAICT there's no meaningful downside to
> using CMPXCHG.

Maybe do both, e.g., still remove the A-bit from FROZEN_SPTE?  AFAICT there's no
good reason to keep the A-bit in FROZEN_SPTE.

Re: [PATCH 0/2] KVM: x86/mmu: Use LOCK CMPXCHG to clear Accessed bits

[Sean Christopherson]

On Wed, Jul 29, 2026, Kai Huang wrote:
> On Mon, 2026-07-27 at 17:22 -0700, Sean Christopherson wrote:
> > I initially was planning on tweaking the FROZEN_SPTE value to make it ok to
> > use LOCK AND on Accessed bits, but AFAICT there's no meaningful downside to
> > using CMPXCHG.
> 
> Maybe do both, e.g., still remove the A-bit from FROZEN_SPTE?  AFAICT there's no
> good reason to keep the A-bit in FROZEN_SPTE.

Hmm, I'm leaning "no".

Once aging uses CMPXCHG, the A-bit isn't any more or less likely to be corrupted
than any other bits.  And arguably, using the A-bit is "safer", because (a) it's
got the same bit position on EPT and everything else, and (b) spuriously setting
or clearing the bit is more likely to be benign.

Re: [PATCH 0/2] KVM: x86/mmu: Use LOCK CMPXCHG to clear Accessed bits

[Huang, Kai]

On Wed, 2026-07-29 at 06:33 -0700, Sean Christopherson wrote:
> On Wed, Jul 29, 2026, Kai Huang wrote:
> > On Mon, 2026-07-27 at 17:22 -0700, Sean Christopherson wrote:
> > > I initially was planning on tweaking the FROZEN_SPTE value to make it ok to
> > > use LOCK AND on Accessed bits, but AFAICT there's no meaningful downside to
> > > using CMPXCHG.
> > 
> > Maybe do both, e.g., still remove the A-bit from FROZEN_SPTE?  AFAICT there's no
> > good reason to keep the A-bit in FROZEN_SPTE.
> 
> Hmm, I'm leaning "no".
> 
> Once aging uses CMPXCHG, the A-bit isn't any more or less likely to be corrupted
> than any other bits.  And arguably, using the A-bit is "safer", because (a) it's
> got the same bit position on EPT and everything else, and (b) spuriously setting
> or clearing the bit is more likely to be benign.

OK makes sense :-)

My concern was if some new code comes up in the future to manipulate the A bit
then it may not be obvious the FROZEN_SPTE contains the A bit, but like you said
it's more likely to be benign.