Re: [PATCH] KVM: x86/mmu: Always guard rmaps with mmu_lock on PREEMPT_RT=y kernels

[James Houghton]

On Wed, Sep 2, 2026 at 2:29 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> For all intents and purposes, revert KVM's ability to walk rmaps outside of
> mmu_lock when running on a realtime (PREEMPT_RT=y) kernel.  I.e. don't use
> a non-sleepable bit-spinlock to protect rmap entries, as realtime kernels
> are highly unlikely to benefit from increased aging throughput and reduced
> jitter for memory-overcommitted nested VMs, whereas using a non-sleepable
> lock is currently buggy and goes against the spirit of realtime kernels.
>
> Because KVM's rmap locks are hand-crafted bit-spinlocks, preemption must be
> disabled before acquiring the lock, otherwise a preempted lock holder will
> result in all other walkers of the locked rmap to spin and wait, with no
> tracked owner for PI to boost.  For non-RT kernels, acquiring mmu_lock
> suffices, as mmu_lock is a non-sleepable rwlock.  But on RT, where mmu_lock
> becomes sleepable, preemption is left enabled for rmap writers:
>
>   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]
>
> Alternatively, KVM could manually disable preemption when grabbing an rmap
> lock, but as above, that isn't what RT kernels generally want, and it's
> actually more complex to implement (cleanly).
>
> To not completely lose the scaling advantage of per-rmap locks, take
> mmu_lock for read in the aging path, i.e. allow multiple concurrent aging
> tasks, as the aging code needs to use atomic SPTE accesses no matter what,
> i.e. no extra code/work is required to guard against concurrent aging of
> SPTEs.
>
> Reported-by: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/8d47b43e1829ac92703723e6a1a4afc7a2eaacb5.camel@xxxxxxxxxxxxx
> Fixes: 4834eaded91e ("KVM: x86/mmu: Add infrastructure to allow walking rmaps outside of mmu_lock")
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>

Thanks, Sean. A small comment below. Feel free to add:

Reviewed-by: James Houghton <jthoughton@xxxxxxxxxx>

> ---
>  arch/x86/kvm/mmu/mmu.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 064ecc33b926..5bf833550f84 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

Future me would be happier if there were a comment here. :)

Also, how about BUILD_BUG_ON(!CONFIG_KVM_MMU_LOCKLESS_AGING)?

> +
>         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))
>
> base-commit: 76671054f9a1ff6abb976583cd8da37650acdc97
> --
> 2.55.0.970.g62bdec98f9-goog
>
>