KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

While soaking unrelated KVM changes on a PREEMPT_RT + lockdep kernel
I hit this, which I don't believe has been reported before:

  WARNING: arch/x86/kvm/mmu/mmu.c:920 at __kvm_rmap_lock+0x1a7/0x1e0 [kvm], CPU#16: vmx_apic_update/3708
  CPU: 16 UID: 0 PID: 3708 Comm: vmx_apic_update Not tainted 7.2.0-rc7 #52 PREEMPT_{RT,LAZY}
  RIP: 0010:__kvm_rmap_lock+0x1a7/0x1e0 [kvm]
  Call Trace:
   pte_list_add+0x67/0x4d0 [kvm]
   __link_shadow_page+0x249/0x480 [kvm]
   ept_fetch+0x4d5/0x1220 [kvm]
   ept_page_fault+0x60b/0x850 [kvm]
   kvm_mmu_do_page_fault+0x252/0x690 [kvm]

That's the lockdep_assert_preemption_disabled() in __kvm_rmap_lock(),
from commit 4834eaded91e ("KVM: x86/mmu: Add infrastructure to allow
walking rmaps outside of mmu_lock").

I don't think this one is just lockdep vs. PREEMPT_RT causing false
positives — the rmap lock is a hand-crafted bit-spinlock, and if a lock
holder is preempted that leaves every other walker of that rmap
spinning and waiting for it (with no tracked owner for PI to boost).

On PREEMPT_RT we genuinely get here without preemption disabled,
because kvm->mmu_lock is a sleeping lock now. Any shadow-MMU fault
on an RT kernel should trip it — this one is the !TDP nested EPT path
(ept_fetch()), and it fires within seconds of running a nested guest
with lockdep enabled.

I guess we fix it by turning the assertion into a preempt_disable() of
its own? Not sufficiently confident in that conclusion to send it in
'diff -up' form though...

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]

Re: KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[Sean Christopherson]

On Thu, Aug 27, 2026, David Woodhouse wrote:
> While soaking unrelated KVM changes on a PREEMPT_RT + lockdep kernel
> I hit this, which I don't believe has been reported before:

Heh, not on-list.  I've hit it the few times I've run PREEMPT_RT, but I (obviously)
haven't cared enough to fix it, especially given the lack of external reports.
Which makes sense; I know people run KVM with PREEMPT_RT, but running nested,
memory-overcommitted VMs with PREEMPT_RT seems beyond crazy.

>   WARNING: arch/x86/kvm/mmu/mmu.c:920 at __kvm_rmap_lock+0x1a7/0x1e0 [kvm], CPU#16: vmx_apic_update/3708
>   CPU: 16 UID: 0 PID: 3708 Comm: vmx_apic_update Not tainted 7.2.0-rc7 #52 PREEMPT_{RT,LAZY}
>   RIP: 0010:__kvm_rmap_lock+0x1a7/0x1e0 [kvm]
>   Call Trace:
>    pte_list_add+0x67/0x4d0 [kvm]
>    __link_shadow_page+0x249/0x480 [kvm]
>    ept_fetch+0x4d5/0x1220 [kvm]
>    ept_page_fault+0x60b/0x850 [kvm]
>    kvm_mmu_do_page_fault+0x252/0x690 [kvm]
> 
> That's the lockdep_assert_preemption_disabled() in __kvm_rmap_lock(),
> from commit 4834eaded91e ("KVM: x86/mmu: Add infrastructure to allow
> walking rmaps outside of mmu_lock").
> 
> I don't think this one is just lockdep vs. PREEMPT_RT causing false
> positives — the rmap lock is a hand-crafted bit-spinlock, and if a lock
> holder is preempted that leaves every other walker of that rmap
> spinning and waiting for it (with no tracked owner for PI to boost).
>
> On PREEMPT_RT we genuinely get here without preemption disabled,
> because kvm->mmu_lock is a sleeping lock now. Any shadow-MMU fault
> on an RT kernel should trip it — this one is the !TDP nested EPT path
> (ept_fetch()), and it fires within seconds of running a nested guest
> with lockdep enabled.

Yep.

> I guess we fix it by turning the assertion into a preempt_disable() of
> its own? Not sufficiently confident in that conclusion to send it in
> 'diff -up' form though...

Or in the spirit of PREEMPT_RT, make the aging code acquire mmu_lock?  As above,
I have a hard time believing anyone cares about aging throughput of nested VMs
when running PREEMPT_RT, certainly not enough to want to disable preemption for
any amount of time.

Not the prettiest code, but it does seem like the right thing to do for PREEMPT_RT.
Compile-tested only at this point.  I'll take it for a spin, unless someone has a
better idea.

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 81c30e2c74f3..31a372d0697c 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -913,11 +913,15 @@ static struct kvm_memory_slot *gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu
  */
 #define KVM_RMAP_LOCKED	BIT(1)
 
-static unsigned long __kvm_rmap_lock(struct kvm_rmap_head *rmap_head)
+static unsigned long __kvm_rmap_lock(struct kvm *kvm,
+				     struct kvm_rmap_head *rmap_head)
 {
 	unsigned long old_val, new_val;
 
-	lockdep_assert_preemption_disabled();
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		lockdep_assert_preemption_disabled();
+	else
+		lockdep_assert_held(&kvm->mmu_lock);
 
 	/*
 	 * Elide the lock if the rmap is empty, as lockless walkers (read-only
@@ -973,7 +977,7 @@ static unsigned long kvm_rmap_lock(struct kvm *kvm,
 {
 	lockdep_assert_held_write(&kvm->mmu_lock);
 
-	return __kvm_rmap_lock(rmap_head);
+	return __kvm_rmap_lock(kvm, rmap_head);
 }
 
 static void __kvm_rmap_unlock(struct kvm_rmap_head *rmap_head,
@@ -1008,14 +1012,17 @@ static unsigned long kvm_rmap_get(struct kvm_rmap_head *rmap_head)
  * actual locking is the same, but the caller is disallowed from modifying the
  * rmap, and so the unlock flow is a nop if the rmap is/was empty.
  */
-static unsigned long kvm_rmap_lock_readonly(struct kvm_rmap_head *rmap_head)
+static unsigned long kvm_rmap_lock_readonly(struct kvm *kvm,
+					    struct kvm_rmap_head *rmap_head)
 {
 	unsigned long rmap_val;
 
-	preempt_disable();
-	rmap_val = __kvm_rmap_lock(rmap_head);
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_disable();
 
-	if (!rmap_val)
+	rmap_val = __kvm_rmap_lock(kvm, rmap_head);
+
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT) && !rmap_val)
 		preempt_enable();
 
 	return rmap_val;
@@ -1030,7 +1037,9 @@ static void kvm_rmap_unlock_readonly(struct kvm_rmap_head *rmap_head,
 	KVM_MMU_WARN_ON(old_val != kvm_rmap_get(rmap_head));
 
 	__kvm_rmap_unlock(rmap_head, old_val);
-	preempt_enable();
+
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		preempt_enable();
 }
 
 /*
@@ -1745,11 +1754,15 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
 	gfn_t gfn;
 	int level;
 
+#ifdef CONFIG_PREEMPT_RT
+	guard(read_lock)(&kvm->mmu_lock);
+#endif
+
 	for (level = PG_LEVEL_4K; level <= KVM_MAX_HUGEPAGE_LEVEL; level++) {
 		for (gfn = range->start; gfn < range->end;
 		     gfn += KVM_PAGES_PER_HPAGE(level)) {
 			rmap_head = gfn_to_rmap(gfn, level, range->slot);
-			rmap_val = kvm_rmap_lock_readonly(rmap_head);
+			rmap_val = kvm_rmap_lock_readonly(kvm, rmap_head);
 
 			for_each_rmap_spte_lockless(rmap_val, &iter, sptep, old_spte) {
 				if (!is_accessed_spte(old_spte))

Re: KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Thu, 2026-08-27 at 14:47 -0700, Sean Christopherson wrote:
> I'll take it for a spin, unless someone has a better idea.

Heh, no pun intended?

> -	lockdep_assert_preemption_disabled();
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +		lockdep_assert_preemption_disabled();
> +	else
> +		lockdep_assert_held(&kvm->mmu_lock);

Other way round?

I'll fix that and throw it on top of my GPC/RT/RCU torture tests.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]

Re: KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[Sean Christopherson]

On Thu, Aug 27, 2026, Sean Christopherson wrote:
> On Thu, Aug 27, 2026, David Woodhouse wrote:
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 81c30e2c74f3..31a372d0697c 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -913,11 +913,15 @@ static struct kvm_memory_slot *gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu
>   */
>  #define KVM_RMAP_LOCKED	BIT(1)
>  
> -static unsigned long __kvm_rmap_lock(struct kvm_rmap_head *rmap_head)
> +static unsigned long __kvm_rmap_lock(struct kvm *kvm,
> +				     struct kvm_rmap_head *rmap_head)
>  {
>  	unsigned long old_val, new_val;
>  
> -	lockdep_assert_preemption_disabled();
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +		lockdep_assert_preemption_disabled();
> +	else
> +		lockdep_assert_held(&kvm->mmu_lock);

...

> @@ -1745,11 +1754,15 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
>  	gfn_t gfn;
>  	int level;
>  
> +#ifdef CONFIG_PREEMPT_RT
> +	guard(read_lock)(&kvm->mmu_lock);
> +#endif

And now I remember why I swept this under the rug.  This really should take
mmu_lock for write, otherwise concurrent aging threads could theoretically get
stuck competing for KVM_RMAP_LOCKED.  Which is silly, because they don't actually
need to take a lock of any kind.  I.e. it's not super trivial?

Wait, duh.  It is trivial if mmu_lock is held, because then KVM can operate on
rmaps without any KVM_RMAP_LOCKED shenanigans.  I wouldn't test this because it
might break horribly, but I think this?

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 81c30e2c74f3..2b157b390a40 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -895,6 +895,7 @@ static struct kvm_memory_slot *gfn_to_memslot_dirty_bitmap(struct kvm_vcpu *vcpu
  */
 #define KVM_RMAP_MANY	BIT(0)
 
+#ifndef CONFIG_PREEMPT_RT
 /*
  * rmaps and PTE lists are mostly protected by mmu_lock (the shadow MMU always
  * operates with mmu_lock held for write), but rmaps can be walked without
@@ -1008,7 +1009,8 @@ static unsigned long kvm_rmap_get(struct kvm_rmap_head *rmap_head)
  * actual locking is the same, but the caller is disallowed from modifying the
  * rmap, and so the unlock flow is a nop if the rmap is/was empty.
  */
-static unsigned long kvm_rmap_lock_readonly(struct kvm_rmap_head *rmap_head)
+static unsigned long kvm_rmap_lock_readonly(struct kvm *kvm,
+					    struct kvm_rmap_head *rmap_head)
 {
 	unsigned long rmap_val;
 
@@ -1032,6 +1034,35 @@ static void kvm_rmap_unlock_readonly(struct kvm_rmap_head *rmap_head,
 	__kvm_rmap_unlock(rmap_head, old_val);
 	preempt_enable();
 }
+#else
+static unsigned long kvm_rmap_get(struct kvm_rmap_head *rmap_head)
+{
+	return atomic_long_read(&rmap_head->val);
+}
+static unsigned long kvm_rmap_lock(struct kvm *kvm,
+				   struct kvm_rmap_head *rmap_head)
+{
+	lockdep_assert_held_write(&kvm->mmu_lock);
+	return kvm_rmap_get(rmap_head);
+}
+
+static void kvm_rmap_unlock(struct kvm *kvm,
+			    struct kvm_rmap_head *rmap_head,
+			    unsigned long new_val)
+{
+	atomic_long_set_release(&rmap_head->val, new_val);
+}
+
+static unsigned long kvm_rmap_lock_readonly(struct kvm *kvm,
+					    struct kvm_rmap_head *rmap_head)
+{
+	lockdep_assert_held_read(&kvm->mmu_lock);
+	return kvm_rmap_get(rmap_head);
+}
+
+static void kvm_rmap_unlock_readonly(struct kvm_rmap_head *rmap_head,
+				     unsigned long old_val) { }
+#endif
 
 /*
  * Returns the number of pointers in the rmap chain, not counting the new one.
@@ -1745,11 +1776,15 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
 	gfn_t gfn;
 	int level;
 
+#ifdef CONFIG_PREEMPT_RT
+	guard(read_lock)(&kvm->mmu_lock);
+#endif
+
 	for (level = PG_LEVEL_4K; level <= KVM_MAX_HUGEPAGE_LEVEL; level++) {
 		for (gfn = range->start; gfn < range->end;
 		     gfn += KVM_PAGES_PER_HPAGE(level)) {
 			rmap_head = gfn_to_rmap(gfn, level, range->slot);
-			rmap_val = kvm_rmap_lock_readonly(rmap_head);
+			rmap_val = kvm_rmap_lock_readonly(kvm, rmap_head);
 
 			for_each_rmap_spte_lockless(rmap_val, &iter, sptep, old_spte) {
 				if (!is_accessed_spte(old_spte))

Re: KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2026-08-27 15:17:57 [-0700], Sean Christopherson wrote:
> > @@ -1745,11 +1754,15 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
> >  	gfn_t gfn;
> >  	int level;
> >  
> > +#ifdef CONFIG_PREEMPT_RT
> > +	guard(read_lock)(&kvm->mmu_lock);
> > +#endif
> 
> And now I remember why I swept this under the rug.  This really should take
> mmu_lock for write, otherwise concurrent aging threads could theoretically get
> stuck competing for KVM_RMAP_LOCKED.  Which is silly, because they don't actually
> need to take a lock of any kind.  I.e. it's not super trivial?
> 
> Wait, duh.  It is trivial if mmu_lock is held, because then KVM can operate on
> rmaps without any KVM_RMAP_LOCKED shenanigans.  I wouldn't test this because it
> might break horribly, but I think this?

So you drop the bit spinlock and rely on kvm->mmu_lock instead? No sure
why it should break horribly but it looks reasonable.

Sebastian

Re: KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

On Fri, 2026-08-28 at 12:45 +0200, Sebastian Andrzej Siewior wrote:
> On 2026-08-27 15:17:57 [-0700], Sean Christopherson wrote:
> > > @@ -1745,11 +1754,15 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
> > >  	gfn_t gfn;
> > >  	int level;
> > >  
> > > +#ifdef CONFIG_PREEMPT_RT
> > > +	guard(read_lock)(&kvm->mmu_lock);
> > > +#endif
> > 
> > And now I remember why I swept this under the rug.  This really should take
> > mmu_lock for write, otherwise concurrent aging threads could theoretically get
> > stuck competing for KVM_RMAP_LOCKED.  Which is silly, because they don't actually
> > need to take a lock of any kind.  I.e. it's not super trivial?
> > 
> > Wait, duh.  It is trivial if mmu_lock is held, because then KVM can operate on
> > rmaps without any KVM_RMAP_LOCKED shenanigans.  I wouldn't test this because it
> > might break horribly, but I think this?
> 
> So you drop the bit spinlock and rely on kvm->mmu_lock instead? No sure
> why it should break horribly but it looks reasonable.

FWIW it survived the night in my GPC invalidation torture tests.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]

Re: KVM: x86/mmu: __kvm_rmap_lock() preemption assert trips on PREEMPT_RT

[Sean Christopherson]

On Fri, Aug 28, 2026, David Woodhouse wrote:
> On Fri, 2026-08-28 at 12:45 +0200, Sebastian Andrzej Siewior wrote:
> > On 2026-08-27 15:17:57 [-0700], Sean Christopherson wrote:
> > > > @@ -1745,11 +1754,15 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
> > > >  	gfn_t gfn;
> > > >  	int level;
> > > >  
> > > > +#ifdef CONFIG_PREEMPT_RT
> > > > +	guard(read_lock)(&kvm->mmu_lock);
> > > > +#endif
> > > 
> > > And now I remember why I swept this under the rug.  This really should take
> > > mmu_lock for write, otherwise concurrent aging threads could theoretically get
> > > stuck competing for KVM_RMAP_LOCKED.  Which is silly, because they don't actually
> > > need to take a lock of any kind.  I.e. it's not super trivial?
> > > 
> > > Wait, duh.  It is trivial if mmu_lock is held, because then KVM can operate on
> > > rmaps without any KVM_RMAP_LOCKED shenanigans.  I wouldn't test this because it
> > > might break horribly, but I think this?
> > 
> > So you drop the bit spinlock and rely on kvm->mmu_lock instead?

Ya, it's basically a revert of the per-rmap locking, but just for PREEMPT_RT.

> > No sure why it should break horribly but it looks reasonable.

I wasn't worried about the locking, I was worried I was forgetting an rmap detail,
e.g. I almost forgot to update rmap_head->val in the "unlock" path.

> FWIW it survived the night in my GPC invalidation torture tests.

Awesome!  I'll throw a PREEMPT_RT kernel through the normal testing and hopefully
get a patch posted today.