[PATCH v3 0/7] KVM: x86/xen: Fix Xen/GPC/PREEMPT_RT issues with rwlock_t

[PATCH v3 0/7] KVM: x86/xen: Fix Xen/GPC/PREEMPT_RT issues with rwlock_t

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 7222 bytes --]

The gfn_to_pfn_cache read side is currently protected by a per-GPC
rwlock_t. That is a problem on PREEMPT_RT, which turns rwlock_t into a
sleeping lock, when the GPC is read from hardirq context (the Xen timer
callback and kvm_arch_set_irq_inatomic()) and from the scheduler path
(kvm_xen_runstate_set_preempted() via kvm_sched_out()).

v1 and v2 worked around that with read_trylock() and a pile of related
cleanups: dropping the now-unnecessary IRQ disabling, and adding CLASS()
constructs to make the lock-and-check dance less error-prone. That series
stalled, partly on the observation that a trylock which callers must be
prepared to fail is an awkward contract to build on.

This version takes the locking away instead. Readers run under
rcu_read_lock() alone, which is legal in every context a GPC is read
from, never fails, and never spins. Mutators clear gpc->valid, wait for
a grace period, and only then touch anything a reader might be looking
at — exactly the pattern of a TLB shootdown, and for the same reason.
That deletes the trylock contortions, the double-lock dance with its
lockdep subclass hack in the runstate update, and the CLASS() machinery
along with them: 15 of the 20 patches in v2 simply become unnecessary
once there is no lock object left to manage.

The only complexity is that the OOM reaper path currently calls
non_block_start(), which is a little overzealous and makes the RCU
grace period sad too. So we rip that out in patch 2.

Patch 1 is the standalone fix for two syzbot reports, already posted
separately¹ and verified by syzbot against the published branch. It
is Cc: stable and stands alone; the rest of the series is cleanup and
consolidation on top.

Patch 2 removes the non_block_start()/end() annotation from the
!blockable mmu_notifier walk. This is a prerequisite, because the
invalidation path now waits for an RCU grace period. That wait is safe
even for the OOM reaper: the actual requirement, per the commit which
added non_block_start() in the first place, is that the caller must not
"have an indirect dependency upon the page allocator and hence close the
loop with the oom reaper", and a grace period has no such dependency —
GPC readers never allocate, never take mmap_lock and never sleep. The
annotation is coarser than the requirement, though, and forbids *any*
voluntary schedule, so it would splat regardless. It is already known to
fire spuriously on PREEMPT_RT, where a plain spinlock becomes sleepable;
all of this was gone over at some length on v2². Cc'ing everyone
involved in the original code.

Patches 4-6 are Sean's from v2, unchanged in substance. Patch 7 is
Carsten's steal-time conversion, which Sean explicitly did not want to
take in v2 without more confidence that it would not regress setups
running NUMA autobalancing and KSM, on the grounds that steal time is
updated on every vCPU load rather than rarely like KVM_REQ_CLOCK_UPDATE.
That concern was well founded, and measuring it turned up a real problem
(now fixed) in this series — see below.

Numbers
=======

Booting a Xen HVM guest (QEMU -accel kvm,xen-version=0x4000a, 4 vCPUs)
on a KASAN + PROVE_LOCKING kernel, timed to the login prompt:

  rwlock (patch 1 only, pre-RCU)  45.562 s
  RCU, this series               45.551 s
  RCU, without the fast path     51.73  s   (1400+ grace periods, 5.9 s)

The middle row is the point: the RCU conversion is performance-neutral
against the code it replaces. Getting there required keeping the
optimisation the rwlock version already had — a refresh which resolves
to the same uHVA, in the same memslot, for the same gPA does not need to
invalidate anything, because the gPA => uHVA translation has not changed
and gpc->valid already asserts the uHVA => PFN mapping is good.

Testing
=======

 - syzbot's C reproducer for the shared_info UAF: fires on the first run
   without patch 1; clean over 12 runs with the series. syzbot has also
   tested patch 1 directly and issued a Tested-by.
 - Targeted reproducers for both syzbot reports (xen_shinfo_race,
   vcpu_info_race) and one for the steal-time cache, soaked 10 minutes
   each under KASAN: clean. Instrumentation showed vcpu_info_race only
   refreshing the cache 16 times over the whole soak, so treat that one
   as weak evidence; the other two drove 4.2M and 147k refreshes
   respectively.
 - xen_shinfo_test, steal_time: pass.
 - The reproducers cover both sides of the fast path: syzbot's alternates
   between two HVAs, so the fast path correctly declines 2506/2506 times
   and takes a grace period each time; a single-HVA reproducer takes it
   4.07M times out of 4.2M refreshes.

Not addressed
=============

Range-based invalidation, and the needs_invalidation flag from earlier
drafts, are both still absent. With the fast path in place the
motivation largely evaporates: a cache which is invalidated but whose
page returns at the same address costs nothing to "refresh", so making
the invalidation itself more precise buys little. I would rather not add
that complexity without a workload which demonstrates it is needed.

  ¹ https://lore.kernel.org/all/6a734239.d1b4d54b.5552d.001c.GAE@google.com/
  ² https://lore.kernel.org/all/20260601134008.rllwJtn-@linutronix.de/

v3:
 - Use RCU for the read side instead of read_trylock(), dropping the
   CLASS() APIs, the irqsave removal patches and the locking/rt patch
   (15 of v2's 20 patches) as no longer applicable.
 - Remove non_block_start()/end() from the !blockable notifier walk.
 - Keep the same-uHVA refresh shortcut, without which the conversion
   costs a grace period per mmu_notifier invalidation.
 - Restore the KVM_REQ_STEAL_UPDATE early-out on the MSR disable path,
   which v2's steal time patch had inadvertently made unconditional.
 - Rebase onto kvm-x86/next, where the MSR handling has moved to msrs.c.

v2: https://lore.kernel.org/all/20260529165114.748639-1-seanjc@google.com
 - Add the CLASS() APIs.
 - Move the steal time change to the very end.
 - "Fix" a dirty logging inconsistency with the Xen vCPU info page.

v1: https://lore.kernel.org/all/20260508181717.3230988-1-dwmw2@infradead.org


Carsten Stollmaier (1):
      KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

David Woodhouse (3):
      KVM: pfncache: use a dedicated invalidation sequence for cache refresh
      mm/mmu_notifier: Remove non_block_start/end() from notifier invocation
      KVM: pfncache: Use RCU for readers instead of a rwlock

Sean Christopherson (3):
      KVM: x86/xen: Extract delivery of event to vCPU into a separate helper
      KVM: x86/xen: Explicitly tag "shared info" page as never being dirty tracked
      KVM: x86/xen: Don't dirty track "vCPU info" page

 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/msrs.c             |   7 +-
 arch/x86/kvm/x86.c              | 128 +++++++++---------
 arch/x86/kvm/xen.c              | 200 ++++++++++++++--------------
 include/linux/kvm_host.h        |  28 ++--
 include/linux/kvm_types.h       |   2 +-
 mm/mmu_notifier.c               |  11 +-
 virt/kvm/kvm_main.c             |  10 ++
 virt/kvm/pfncache.c             | 282 +++++++++++++++++++++++++---------------
 9 files changed, 369 insertions(+), 301 deletions(-)


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 1/7] KVM: pfncache: use a dedicated invalidation sequence for cache refresh

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 6888 bytes --]

The gfn_to_pfn_cache refresh path guards against mmu notifier
invalidations which complete while it has dropped gpc->lock for the
HVA->PFN lookup: hva_to_pfn_retry() samples kvm->mmu_invalidate_seq
and retries if it changed, or if mn_active_invalidate_count is still
elevated.

That is insufficient for HVA-based caches. mmu_invalidate_seq is only
advanced by kvm_mmu_invalidate_end() when the invalidated range
overlaps a memslot, and an HVA-based cache (e.g. the Xen shared_info
page mapped with KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA) need not be backed
by any memslot at all. An invalidation of the cached HVA which starts
and ends entirely within the lookup window is thus invisible to the
retry check: mn_active_invalidate_count is back to zero and the
sequence never moved. The refresh then publishes a mapping of a page
which has already been freed, and the next reader dereferences it:

  BUG: KASAN: use-after-free in kvm_xen_shared_info_init+0x3c6/0x440
  Read of size 4 at addr ffff8880599c2900 by task syz.2.383/7257

Since gfn_to_pfn_cache_invalidate_start() deliberately skips caches
which are not currently valid (including one whose refresh is in
progress, as the refresh clears the valid flag before dropping the
lock), the retry check is the only line of defence, and it must fire
for *any* invalidation, not just those hitting a memslot.

Add a dedicated kvm->gpc_invalidate_seq, incremented by every
kvm_mmu_notifier_invalidate_range_end() under mn_invalidate_lock
before mn_active_invalidate_count is decremented, and check it in
hva_to_pfn_retry() instead of mmu_invalidate_seq. Incrementing in
range_end() in the same critical section as the in-progress count
also closes the variant where the cache is activated with the
contested HVA only after invalidate_range_start() has run.

The same bug is also reachable through the per-vCPU vcpu_info cache
(KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA), where the stale mapping is
then dereferenced by kvm_setup_guest_pvclock() on the next KVM_RUN:

  BUG: KASAN: use-after-free in kvm_setup_guest_pvclock+0x5bf/0x660

This intentionally makes refresh retry on *unrelated* mmu notifier
events; restoring precision (and reworking the GPC locking more
generally) is left for a subsequent series.

Reproducers: https://david.woodhou.se/xen_shinfo_race.c
             https://david.woodhou.se/vcpu_info_race.c

Suggested-by: Sean Christopherson <seanjc@google.com>
Reported-by: syzbot+0948c82180d475ad24e2@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6a0c5f2c.a00a0220.2c7954.0000.GAE@google.com/
Tested-by: syzbot+0948c82180d475ad24e2@syzkaller.appspotmail.com
Reported-by: syzbot+fb7c2dd166d3ea63df2a@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6a426dd2.854d4ab9.360e1d.0008.GAE@google.com/
Fixes: b9220d32799a ("KVM: x86/xen: allow shared_info to be mapped by fixed HVA")
Cc: stable@vger.kernel.org
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 include/linux/kvm_host.h |  2 ++
 virt/kvm/kvm_main.c      | 10 ++++++++++
 virt/kvm/pfncache.c      | 18 +++++++++---------
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 03bfc92864b6..3dd04605f2e5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -855,6 +855,8 @@ struct kvm {
 	gfn_t mmu_invalidate_range_start;
 	gfn_t mmu_invalidate_range_end;
 
+	unsigned long gpc_invalidate_seq;
+
 	struct list_head devices;
 	u64 manual_dirty_log_protect;
 	struct dentry *debugfs_dentry;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 345d56a15fa4..41c88a8ade95 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -812,6 +812,16 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
 
 	/* Pairs with the increment in range_start(). */
 	spin_lock(&kvm->mn_invalidate_lock);
+	kvm->gpc_invalidate_seq++;
+
+	/*
+	 * As with the MMU sequence counter and mmu_invalidate_in_progress, the
+	 * GPC sequence increase must be visible before the invalidate count
+	 * goes to zero.  Pairs with the smp_rmb() in
+	 * mmu_notifier_retry_cache().
+	 */
+	smp_wmb();
+
 	if (!WARN_ON_ONCE(!kvm->mn_active_invalidate_count))
 		--kvm->mn_active_invalidate_count;
 	wake = !kvm->mn_active_invalidate_count;
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 728d2c1b488a..3659686b97c2 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -124,7 +124,7 @@ static void gpc_unmap(kvm_pfn_t pfn, void *khva)
 #endif
 }
 
-static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_seq)
+static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long gpc_seq)
 {
 	/*
 	 * mn_active_invalidate_count acts for all intents and purposes
@@ -136,20 +136,20 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_s
 	 * Note, it does not matter that mn_active_invalidate_count
 	 * is not protected by gpc->lock.  It is guaranteed to
 	 * be elevated before the mmu_notifier acquires gpc->lock, and
-	 * isn't dropped until after mmu_invalidate_seq is updated.
+	 * isn't dropped until after gpc_invalidate_seq is updated.
 	 */
 	if (kvm->mn_active_invalidate_count)
 		return true;
 
 	/*
 	 * Ensure mn_active_invalidate_count is read before
-	 * mmu_invalidate_seq.  This pairs with the smp_wmb() in
-	 * mmu_notifier_invalidate_range_end() to guarantee either the
+	 * gpc_invalidate_seq.  This pairs with the smp_wmb() in
+	 * kvm_mmu_notifier_invalidate_range_end() to guarantee either the
 	 * old (non-zero) value of mn_active_invalidate_count or the
-	 * new (incremented) value of mmu_invalidate_seq is observed.
+	 * new (incremented) value of gpc_invalidate_seq is observed.
 	 */
 	smp_rmb();
-	return kvm->mmu_invalidate_seq != mmu_seq;
+	return kvm->gpc_invalidate_seq != gpc_seq;
 }
 
 static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
@@ -158,7 +158,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 	void *old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva);
 	kvm_pfn_t new_pfn = KVM_PFN_ERR_FAULT;
 	void *new_khva = NULL;
-	unsigned long mmu_seq;
+	unsigned long gpc_seq;
 	struct page *page;
 
 	struct kvm_follow_pfn kfp = {
@@ -181,7 +181,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 	gpc->valid = false;
 
 	do {
-		mmu_seq = gpc->kvm->mmu_invalidate_seq;
+		gpc_seq = gpc->kvm->gpc_invalidate_seq;
 		smp_rmb();
 
 		write_unlock_irq(&gpc->lock);
@@ -232,7 +232,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 		 * attempting to refresh.
 		 */
 		WARN_ON_ONCE(gpc->valid);
-	} while (mmu_notifier_retry_cache(gpc->kvm, mmu_seq));
+	} while (mmu_notifier_retry_cache(gpc->kvm, gpc_seq));
 
 	gpc->valid = true;
 	gpc->pfn = new_pfn;

base-commit: 51ba04112e93ac6e04c627eddcea5caf9cbc0134
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 2/7] mm/mmu_notifier: Remove non_block_start/end() from notifier invocation

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 3130 bytes --]

This effectively reverts commit ba170f76b69d ("mm, notifier: Catch
sleeping/blocking for !blockable") for the mmu_notifier call sites.

The non_block_start/end() annotation causes the scheduler to complain
about *any* voluntary sleep in a non-blockable notifier. But that was
never the actual constraint. As Michal Hocko put it when the
annotation was first proposed, the OOM reaper "shouldn't depend on any
locks or sleepable conditionals" and checking for sleepable context
was "the best thing we could come up with that would describe these
demands at least partially". The real requirement is that the reaper
must not block on anything which may itself depend on memory
allocation (or on the dying mm) to make progress — which is why
spinning locks were always considered fine.

That distinction now matters in both directions:

 - On PREEMPT_RT, spinning locks become sleeping locks, and perfectly
   legitimate spinlock/rwlock usage in notifier implementations (e.g.
   KVM's mn_invalidate_lock and gfn_to_pfn_cache locks) triggers the
   splat despite having no allocator dependency whatsoever.

 - A notifier implementation may legitimately need to wait for an RCU
   grace period before allowing the caller to proceed with unmapping
   (in the manner of a TLB shootdown, waiting for readers of a cached
   translation to drain). A grace period completes without any memory
   allocation and cannot deadlock against the reaper, but the
   annotation forbids it.

Checking for genuinely forbidden dependencies mechanically would
require tracking *what* is being waited on, which this annotation
never did. Remove it from the notifier invocation and leave the
constraint where it always really lived: in review and documentation
of the notifier implementations.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 mm/mmu_notifier.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 245b74f39f91..cd5d15cd646a 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -520,11 +520,7 @@ static int mn_hlist_invalidate_range_start(
 		if (ops->invalidate_range_start) {
 			int _ret;
 
-			if (!mmu_notifier_range_blockable(range))
-				non_block_start();
 			_ret = ops->invalidate_range_start(subscription, range);
-			if (!mmu_notifier_range_blockable(range))
-				non_block_end();
 			if (_ret) {
 				pr_info("%pS callback failed with %d in %sblockable context.\n",
 					ops->invalidate_range_start, _ret,
@@ -591,14 +587,9 @@ mn_hlist_invalidate_end(struct mmu_notifier_subscriptions *subscriptions,
 	id = srcu_read_lock(&srcu);
 	hlist_for_each_entry_srcu(subscription, &subscriptions->list, hlist,
 				 srcu_read_lock_held(&srcu)) {
-		if (subscription->ops->invalidate_range_end) {
-			if (!mmu_notifier_range_blockable(range))
-				non_block_start();
+		if (subscription->ops->invalidate_range_end)
 			subscription->ops->invalidate_range_end(subscription,
 								range);
-			if (!mmu_notifier_range_blockable(range))
-				non_block_end();
-		}
 	}
 	srcu_read_unlock(&srcu, id);
 }
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 29321 bytes --]

Replace the per-cache rwlock with RCU for the read side. Readers now
run under rcu_read_lock() alone, which works in any context (including
hardirq and sched-out paths) and never fails or spins — eliminating the
read_trylock() contortions in the atomic paths, the double-lock dance
with the lockdep subclass hack in the runstate update, and the
PREEMPT_RT problems inherent to taking an rwlock in those contexts.
In particular, kvm_xen_set_evtchn_fast() is called from hardirq
context (timer callback, kvm_arch_set_irq_inatomic()), where taking
gpc->lock is a sleeping-lock-in-atomic-context bug on PREEMPT_RT.

The invariant is that a cache's fields (pfn, khva, uhva, gpa) are only
ever mutated after clearing gpc->valid and waiting for a full grace
period, so any reader which observed ->valid == true (with an acquire
load, paired with the release store publishing the fields) has stable
values for its entire RCU read-side critical section — including
writes through khva, which are guaranteed to land before the backing
page can be unmapped, exactly as with a TLB shootdown.

The mmu_notifier invalidation path clears ->valid on any overlapping
cache and then does synchronize_rcu() before returning, so the primary
MMU cannot proceed to zap the page tables until all readers of the
stale mapping have drained. This wait happens even on unblockable
(OOM reaper) ranges: the actual constraint on those is not "no
sleeping" but "no blocking on anything which may itself depend on
memory allocation to make progress", and an RCU grace period has no
such dependency — GPC readers never allocate, never take mmap_lock,
and never sleep. The previous patch removed the over-broad
non_block_start() debug annotation which would have splatted on any
voluntary schedule regardless.

As with the rwlock version, a refresh which resolves to the same uHVA,
in the same memslot, for the same gPA does not need to invalidate the
cache at all: the gPA => uHVA translation has not changed, and
gpc->valid already asserts that the uHVA => PFN mapping is good. Such a
refresh updates only the memslot generation and returns, leaving
concurrent readers undisturbed and skipping the grace period entirely.
This matters because the mmu_notifier invalidates caches on any host
memory management activity (page migration, NUMA balancing, KSM), and
the affected pages usually come straight back at the same uHVA; making
each of those cost a full RCU grace period in the reader's refresh path
would add seconds to a guest boot.

Refreshes are serialized by the existing refresh_lock mutex, and the
gpc_invalidate_seq mechanism continues to catch invalidations which
race with the (lockless) HVA->PFN lookup.

Fixes: 14243b387137 ("KVM: x86/xen: Add KVM_IRQ_ROUTING_XEN_EVTCHN and event channel delivery")
Reported-by: syzbot+208f7f3e5f59c11aeb90@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=208f7f3e5f59c11aeb90
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 arch/x86/kvm/x86.c        |   9 +-
 arch/x86/kvm/xen.c        |  91 +++++--------
 include/linux/kvm_host.h  |  16 +--
 include/linux/kvm_types.h |   1 -
 virt/kvm/pfncache.c       | 260 ++++++++++++++++++++++++--------------
 5 files changed, 206 insertions(+), 171 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d94b59140c45..09425f72f4a5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1719,18 +1719,17 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
 {
 	struct pvclock_vcpu_time_info *guest_hv_clock;
 	struct pvclock_vcpu_time_info hv_clock;
-	unsigned long flags;
 
 	memcpy(&hv_clock, ref_hv_clock, sizeof(hv_clock));
 
-	read_lock_irqsave(&gpc->lock, flags);
+	rcu_read_lock();
 	while (!kvm_gpc_check(gpc, offset + sizeof(*guest_hv_clock))) {
-		read_unlock_irqrestore(&gpc->lock, flags);
+		rcu_read_unlock();
 
 		if (kvm_gpc_refresh(gpc, offset + sizeof(*guest_hv_clock)))
 			return;
 
-		read_lock_irqsave(&gpc->lock, flags);
+		rcu_read_lock();
 	}
 
 	guest_hv_clock = (void *)(gpc->khva + offset);
@@ -1755,7 +1754,7 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
 	guest_hv_clock->version = ++hv_clock.version;
 
 	kvm_gpc_mark_dirty_in_slot(gpc);
-	read_unlock_irqrestore(&gpc->lock, flags);
+	rcu_read_unlock();
 
 	trace_kvm_pvclock_update(vcpu->vcpu_id, &hv_clock);
 }
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index f2c6757fc1fa..6bcece2439f5 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -46,15 +46,15 @@ static int kvm_xen_shared_info_init(struct kvm *kvm)
 	int ret = 0;
 	int idx = srcu_read_lock(&kvm->srcu);
 
-	read_lock_irq(&gpc->lock);
+	rcu_read_lock();
 	while (!kvm_gpc_check(gpc, PAGE_SIZE)) {
-		read_unlock_irq(&gpc->lock);
+		rcu_read_unlock();
 
 		ret = kvm_gpc_refresh(gpc, PAGE_SIZE);
 		if (ret)
 			goto out;
 
-		read_lock_irq(&gpc->lock);
+		rcu_read_lock();
 	}
 
 	/*
@@ -97,7 +97,7 @@ static int kvm_xen_shared_info_init(struct kvm *kvm)
 	smp_wmb();
 
 	wc->version = wc_version + 1;
-	read_unlock_irq(&gpc->lock);
+	rcu_read_unlock();
 
 out:
 	srcu_read_unlock(&kvm->srcu, idx);
@@ -154,22 +154,21 @@ static int xen_get_guest_pvclock(struct kvm_vcpu *vcpu,
 				 struct gfn_to_pfn_cache *gpc,
 				 unsigned int offset)
 {
-	unsigned long flags;
 	int r;
 
-	read_lock_irqsave(&gpc->lock, flags);
+	rcu_read_lock();
 	while (!kvm_gpc_check(gpc, offset + sizeof(*hv_clock))) {
-		read_unlock_irqrestore(&gpc->lock, flags);
+		rcu_read_unlock();
 
 		r = kvm_gpc_refresh(gpc, offset + sizeof(*hv_clock));
 		if (r)
 			return r;
 
-		read_lock_irqsave(&gpc->lock, flags);
+		rcu_read_lock();
 	}
 
 	memcpy(hv_clock, gpc->khva + offset, sizeof(*hv_clock));
-	read_unlock_irqrestore(&gpc->lock, flags);
+	rcu_read_unlock();
 
 	/*
 	 * Sanity check TSC shift+multiplier to verify the guest's view of time
@@ -324,7 +323,6 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 	struct gfn_to_pfn_cache *gpc2 = &vx->runstate2_cache;
 	size_t user_len, user_len1, user_len2;
 	struct vcpu_runstate_info rs;
-	unsigned long flags;
 	size_t times_ofs;
 	uint8_t *update_bit = NULL;
 	uint64_t entry_time;
@@ -416,20 +414,12 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 
  retry:
 	/*
-	 * Attempt to obtain the GPC lock on *both* (if there are two)
-	 * gfn_to_pfn caches that cover the region.
+	 * Check *both* (if there are two) gfn_to_pfn caches that cover
+	 * the region, under a single RCU read-side critical section.
 	 */
-	if (atomic) {
-		local_irq_save(flags);
-		if (!read_trylock(&gpc1->lock)) {
-			local_irq_restore(flags);
-			return;
-		}
-	} else {
-		read_lock_irqsave(&gpc1->lock, flags);
-	}
+	rcu_read_lock();
 	while (!kvm_gpc_check(gpc1, user_len1)) {
-		read_unlock_irqrestore(&gpc1->lock, flags);
+		rcu_read_unlock();
 
 		/* When invoked from kvm_sched_out() we cannot sleep */
 		if (atomic)
@@ -438,7 +428,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 		if (kvm_gpc_refresh(gpc1, user_len1))
 			return;
 
-		read_lock_irqsave(&gpc1->lock, flags);
+		rcu_read_lock();
 	}
 
 	if (likely(!user_len2)) {
@@ -458,24 +448,11 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 	} else {
 		/*
 		 * The guest's runstate_info is split across two pages and we
-		 * need to hold and validate both GPCs simultaneously. We can
-		 * declare a lock ordering GPC1 > GPC2 because nothing else
-		 * takes them more than one at a time. Set a subclass on the
-		 * gpc1 lock to make lockdep shut up about it.
+		 * need to validate both GPCs simultaneously. They are both
+		 * covered by the single RCU read-side critical section above.
 		 */
-		lock_set_subclass(&gpc1->lock.dep_map, 1, _THIS_IP_);
-		if (atomic) {
-			if (!read_trylock(&gpc2->lock)) {
-				read_unlock_irqrestore(&gpc1->lock, flags);
-				return;
-			}
-		} else {
-			read_lock(&gpc2->lock);
-		}
-
 		if (!kvm_gpc_check(gpc2, user_len2)) {
-			read_unlock(&gpc2->lock);
-			read_unlock_irqrestore(&gpc1->lock, flags);
+			rcu_read_unlock();
 
 			/* When invoked from kvm_sched_out() we cannot sleep */
 			if (atomic)
@@ -574,13 +551,11 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 		smp_wmb();
 	}
 
-	if (user_len2) {
+	if (user_len2)
 		kvm_gpc_mark_dirty_in_slot(gpc2);
-		read_unlock(&gpc2->lock);
-	}
 
 	kvm_gpc_mark_dirty_in_slot(gpc1);
-	read_unlock_irqrestore(&gpc1->lock, flags);
+	rcu_read_unlock();
 }
 
 void kvm_xen_update_runstate(struct kvm_vcpu *v, int state)
@@ -639,7 +614,6 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 {
 	unsigned long evtchn_pending_sel = READ_ONCE(v->arch.xen.evtchn_pending_sel);
 	struct gfn_to_pfn_cache *gpc = &v->arch.xen.vcpu_info_cache;
-	unsigned long flags;
 
 	if (!evtchn_pending_sel)
 		return;
@@ -649,14 +623,14 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 	 * does anyway. Page it in and retry the instruction. We're just a
 	 * little more honest about it.
 	 */
-	read_lock_irqsave(&gpc->lock, flags);
+	rcu_read_lock();
 	while (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
-		read_unlock_irqrestore(&gpc->lock, flags);
+		rcu_read_unlock();
 
 		if (kvm_gpc_refresh(gpc, sizeof(struct vcpu_info)))
 			return;
 
-		read_lock_irqsave(&gpc->lock, flags);
+		rcu_read_lock();
 	}
 
 	/* Now gpc->khva is a valid kernel address for the vcpu_info */
@@ -686,7 +660,7 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 	}
 
 	kvm_gpc_mark_dirty_in_slot(gpc);
-	read_unlock_irqrestore(&gpc->lock, flags);
+	rcu_read_unlock();
 
 	/* For the per-vCPU lapic vector, deliver it as MSI. */
 	if (v->arch.xen.upcall_vector)
@@ -696,7 +670,6 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
 {
 	struct gfn_to_pfn_cache *gpc = &v->arch.xen.vcpu_info_cache;
-	unsigned long flags;
 	u8 rc = 0;
 
 	/*
@@ -712,9 +685,9 @@ int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
 	BUILD_BUG_ON(sizeof(rc) !=
 		     sizeof_field(struct compat_vcpu_info, evtchn_upcall_pending));
 
-	read_lock_irqsave(&gpc->lock, flags);
+	rcu_read_lock();
 	while (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
-		read_unlock_irqrestore(&gpc->lock, flags);
+		rcu_read_unlock();
 
 		/*
 		 * This function gets called from kvm_vcpu_block() after setting the
@@ -734,11 +707,11 @@ int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
 			 */
 			return 0;
 		}
-		read_lock_irqsave(&gpc->lock, flags);
+		rcu_read_lock();
 	}
 
 	rc = ((struct vcpu_info *)gpc->khva)->evtchn_upcall_pending;
-	read_unlock_irqrestore(&gpc->lock, flags);
+	rcu_read_unlock();
 	return rc;
 }
 
@@ -1437,12 +1410,11 @@ static bool wait_pending_event(struct kvm_vcpu *vcpu, int nr_ports,
 	struct kvm *kvm = vcpu->kvm;
 	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
 	unsigned long *pending_bits;
-	unsigned long flags;
 	bool ret = true;
 	int idx, i;
 
 	idx = srcu_read_lock(&kvm->srcu);
-	read_lock_irqsave(&gpc->lock, flags);
+	rcu_read_lock();
 	if (!kvm_gpc_check(gpc, PAGE_SIZE))
 		goto out_rcu;
 
@@ -1463,7 +1435,7 @@ static bool wait_pending_event(struct kvm_vcpu *vcpu, int nr_ports,
 	}
 
  out_rcu:
-	read_unlock_irqrestore(&gpc->lock, flags);
+	rcu_read_unlock();
 	srcu_read_unlock(&kvm->srcu, idx);
 
 	return ret;
@@ -1802,7 +1774,6 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
 	struct kvm_vcpu *vcpu;
 	unsigned long *pending_bits, *mask_bits;
-	unsigned long flags;
 	int port_word_bit;
 	bool kick_vcpu = false;
 	int vcpu_idx, idx, rc;
@@ -1824,7 +1795,7 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 
 	idx = srcu_read_lock(&kvm->srcu);
 
-	read_lock_irqsave(&gpc->lock, flags);
+	rcu_read_lock();
 	if (!kvm_gpc_check(gpc, PAGE_SIZE))
 		goto out_rcu;
 
@@ -1855,10 +1826,8 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 	} else {
 		rc = 1; /* Delivered to the bitmap in shared_info. */
 		/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
-		read_unlock_irqrestore(&gpc->lock, flags);
 		gpc = &vcpu->arch.xen.vcpu_info_cache;
 
-		read_lock_irqsave(&gpc->lock, flags);
 		if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
 			/*
 			 * Could not access the vcpu_info. Set the bit in-kernel
@@ -1892,7 +1861,7 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 	}
 
  out_rcu:
-	read_unlock_irqrestore(&gpc->lock, flags);
+	rcu_read_unlock();
 	srcu_read_unlock(&kvm->srcu, idx);
 
 	if (kick_vcpu) {
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3dd04605f2e5..da0b3669f1a3 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1510,12 +1510,9 @@ int kvm_gpc_activate_hva(struct gfn_to_pfn_cache *gpc, unsigned long hva, unsign
  * @return:	   %true if the cache is still valid and the address matches.
  *		   %false if the cache is not valid.
  *
- * Callers outside IN_GUEST_MODE context should hold a read lock on @gpc->lock
- * while calling this function, and then continue to hold the lock until the
- * access is complete.
- *
- * Callers in IN_GUEST_MODE may do so without locking, although they should
- * still hold a read lock on kvm->scru for the memslot checks.
+ * Callers must hold the RCU read lock across this function and any
+ * subsequent access to the target page, and must hold a read lock on
+ * kvm->srcu for the memslot checks.
  */
 bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len);
 
@@ -1532,8 +1529,8 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len);
  * This will attempt to refresh a gfn_to_pfn_cache. Note that a successful
  * return from this function does not mean the page can be immediately
  * accessed because it may have raced with an invalidation. Callers must
- * still lock and check the cache status, as this function does not return
- * with the lock still held to permit access.
+ * still check the cache status under the RCU read lock, via
+ * kvm_gpc_check(), before accessing the target page.
  */
 int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
 
@@ -1974,7 +1971,8 @@ static inline bool kvm_is_gpa_in_memslot(struct kvm *kvm, gpa_t gpa)
 
 static inline void kvm_gpc_mark_dirty_in_slot(struct gfn_to_pfn_cache *gpc)
 {
-	lockdep_assert_held(&gpc->lock);
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
+			 "kvm_gpc_mark_dirty_in_slot() without RCU read lock");
 
 	if (!gpc->memslot)
 		return;
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index a568d8e6f4e8..fdf7d2273298 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -88,7 +88,6 @@ struct gfn_to_pfn_cache {
 	struct kvm_memory_slot *memslot;
 	struct kvm *kvm;
 	struct list_head list;
-	rwlock_t lock;
 	struct mutex refresh_lock;
 	void *khva;
 	kvm_pfn_t pfn;
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 3659686b97c2..3bae4df49165 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -26,35 +26,49 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
 				       unsigned long end)
 {
 	struct gfn_to_pfn_cache *gpc;
+	bool cleared = false;
 
 	spin_lock(&kvm->gpc_lock);
 	list_for_each_entry(gpc, &kvm->gpc_list, list) {
-		read_lock_irq(&gpc->lock);
-
-		/* Only a single page so no need to care about length */
-		if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
+		/*
+		 * Only a single page so no need to care about length.
+		 * A cache whose refresh is in flight has valid == false
+		 * and is caught by the gpc_invalidate_seq check in
+		 * hva_to_pfn_retry() instead.
+		 */
+		/*
+		 * The acquire pairs with the release-publish of valid in
+		 * hva_to_pfn_retry(): if valid is seen true, the uhva read
+		 * below is guaranteed to see the value which was stored
+		 * before valid was published, not a stale one. (A stale
+		 * uhva paired with a fresh valid could otherwise cause a
+		 * cache whose new uhva is in the invalidated range to be
+		 * skipped.)
+		 */
+		if (smp_load_acquire(&gpc->valid) &&
 		    gpc->uhva >= start && gpc->uhva < end) {
-			read_unlock_irq(&gpc->lock);
-
-			/*
-			 * There is a small window here where the cache could
-			 * be modified, and invalidation would no longer be
-			 * necessary. Hence check again whether invalidation
-			 * is still necessary once the write lock has been
-			 * acquired.
-			 */
-
-			write_lock_irq(&gpc->lock);
-			if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
-			    gpc->uhva >= start && gpc->uhva < end)
-				gpc->valid = false;
-			write_unlock_irq(&gpc->lock);
-			continue;
+			WRITE_ONCE(gpc->valid, false);
+			cleared = true;
 		}
-
-		read_unlock_irq(&gpc->lock);
 	}
 	spin_unlock(&kvm->gpc_lock);
+
+	/*
+	 * Readers may still be using the old mapping, having sampled
+	 * gpc->valid before it was cleared. Wait for them all to drain
+	 * before the caller proceeds to zap the page tables; like a TLB
+	 * shootdown, this guarantees no access via the stale mapping
+	 * once the invalidation completes.
+	 *
+	 * This wait is safe even on unblockable ranges (the OOM reaper):
+	 * the constraint there is not "no sleeping" but "no blocking on
+	 * anything which may itself depend on memory allocation to make
+	 * progress" (see the reasoning in commit 312364f3534c and its
+	 * discussion). An RCU grace period has no such dependency: GPC
+	 * readers never allocate, never take mmap_lock, and never sleep.
+	 */
+	if (cleared)
+		synchronize_rcu();
 }
 
 static bool kvm_gpc_is_valid_len(gpa_t gpa, unsigned long uhva,
@@ -74,6 +88,22 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
 {
 	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
 
+	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
+			 "kvm_gpc_check() without RCU read lock");
+
+	/*
+	 * Check valid *first*. The acquire pairs with the release-publish
+	 * in hva_to_pfn_retry(), so every field read below — and any use
+	 * of gpc->khva by the caller — is guaranteed to be from the
+	 * published generation, not a stale value reordered from before
+	 * the publish. The fields are then stable for the remainder of
+	 * the RCU read-side critical section, because every mutator
+	 * clears valid and waits a full grace period before changing
+	 * anything.
+	 */
+	if (!smp_load_acquire(&gpc->valid))
+		return false;
+
 	if (!gpc->active)
 		return false;
 
@@ -90,9 +120,6 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
 	if (!kvm_gpc_is_valid_len(gpc->gpa, gpc->uhva, len))
 		return false;
 
-	if (!gpc->valid)
-		return false;
-
 	return true;
 }
 
@@ -134,8 +161,8 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long gpc_s
 	 * is elevated.
 	 *
 	 * Note, it does not matter that mn_active_invalidate_count
-	 * is not protected by gpc->lock.  It is guaranteed to
-	 * be elevated before the mmu_notifier acquires gpc->lock, and
+	 * is not protected by any lock the refresher holds.  It is
+	 * guaranteed to be elevated before the mmu_notifier walk, and
 	 * isn't dropped until after gpc_invalidate_seq is updated.
 	 */
 	if (kvm->mn_active_invalidate_count)
@@ -171,21 +198,12 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 
 	lockdep_assert_held(&gpc->refresh_lock);
 
-	lockdep_assert_held_write(&gpc->lock);
-
-	/*
-	 * Invalidate the cache prior to dropping gpc->lock, the gpa=>uhva
-	 * assets have already been updated and so a concurrent check() from a
-	 * different task may not fail the gpa/uhva/generation checks.
-	 */
-	gpc->valid = false;
+	WARN_ON_ONCE(gpc->valid);
 
 	do {
 		gpc_seq = gpc->kvm->gpc_invalidate_seq;
 		smp_rmb();
 
-		write_unlock_irq(&gpc->lock);
-
 		/*
 		 * If the previous iteration "failed" due to an mmu_notifier
 		 * event, release the pfn and unmap the kernel virtual address
@@ -213,7 +231,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 		/*
 		 * Obtain a new kernel mapping if KVM itself will access the
 		 * pfn.  Note, kmap() and memremap() can both sleep, so this
-		 * too must be done outside of gpc->lock!
+		 * can sleep, which is fine: this path holds no spinning locks.
 		 */
 		if (new_pfn == gpc->pfn)
 			new_khva = old_khva;
@@ -224,20 +242,17 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 			kvm_release_page_unused(page);
 			goto out_error;
 		}
-
-		write_lock_irq(&gpc->lock);
-
-		/*
-		 * Other tasks must wait for _this_ refresh to complete before
-		 * attempting to refresh.
-		 */
-		WARN_ON_ONCE(gpc->valid);
 	} while (mmu_notifier_retry_cache(gpc->kvm, gpc_seq));
 
-	gpc->valid = true;
 	gpc->pfn = new_pfn;
 	gpc->khva = new_khva + offset_in_page(gpc->uhva);
 
+	/*
+	 * Publish. Pairs with the smp_load_acquire() in kvm_gpc_check();
+	 * the pfn/khva stores above must be visible before valid is.
+	 */
+	smp_store_release(&gpc->valid, true);
+
 	/*
 	 * Put the reference to the _new_ page.  The page is now tracked by the
 	 * cache and can be safely migrated, swapped, etc... as the cache will
@@ -248,8 +263,6 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 	return 0;
 
 out_error:
-	write_lock_irq(&gpc->lock);
-
 	return -EFAULT;
 }
 
@@ -259,7 +272,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
 	bool unmap_old = false;
 	unsigned long old_uhva;
 	kvm_pfn_t old_pfn;
-	bool hva_change = false;
+	bool was_valid;
 	void *old_khva;
 	int ret;
 
@@ -269,11 +282,85 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
 
 	lockdep_assert_held(&gpc->refresh_lock);
 
-	write_lock_irq(&gpc->lock);
+	if (!gpc->active)
+		return -EINVAL;
 
-	if (!gpc->active) {
-		ret = -EINVAL;
-		goto out_unlock;
+	/*
+	 * Resolve the target uHVA (and memslot, for a GPA-based cache) before
+	 * deciding whether anything needs to be invalidated at all.
+	 *
+	 * If the cache is still valid and this refresh resolves to exactly the
+	 * same uHVA, in the same memslot, for the same GPA, then nothing which
+	 * a reader can observe is changing: the gPA => uHVA translation is
+	 * unchanged, and gpc->valid being set is precisely the assertion that
+	 * the second stage (uHVA => PFN, and the kernel mapping of it) is
+	 * still good. No mapping is retired, so there is nothing for a grace
+	 * period to wait for. At most the memslot generation needs updating,
+	 * which no reader consults except via kvm_gpc_check() itself.
+	 *
+	 * All of the following must hold to take this shortcut:
+	 *
+	 *  - gpc->valid: the PFN and its kernel mapping are still good.
+	 *  - the resolved uHVA is unchanged. kvm_gpc_check() validates
+	 *    'offset + len <= PAGE_SIZE' from gpc->uhva/gpa on the reader's
+	 *    behalf, and the reader then accesses gpc->khva for 'len' bytes
+	 *    with no recheck; moving the offset under a live reader would let
+	 *    it run off the end of the page.
+	 *  - the memslot is unchanged. kvm_gpc_mark_dirty_in_slot() uses
+	 *    gpc->memslot, so replacing it under a reader could mark the wrong
+	 *    slot dirty and thus lose a dirty page for live migration.
+	 *  - the gPA is unchanged, as it provides the gfn for dirty tracking.
+	 */
+	if (READ_ONCE(gpc->valid)) {
+		struct kvm_memory_slot *new_slot = gpc->memslot;
+		unsigned long new_uhva = KVM_HVA_ERR_BAD;
+		u64 new_generation = gpc->generation;
+
+		if (kvm_is_error_gpa(gpa)) {
+			new_uhva = uhva;
+		} else {
+			struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
+			gfn_t gfn = gpa_to_gfn(gpa);
+
+			new_generation = slots->generation;
+			new_slot = __gfn_to_memslot(slots, gfn);
+			if (new_slot)
+				new_uhva = gfn_to_hva_memslot(new_slot, gfn) +
+					   offset_in_page(gpa);
+		}
+
+		if (!kvm_is_error_hva(new_uhva) && new_uhva == gpc->uhva &&
+		    new_slot == gpc->memslot && gpa == gpc->gpa) {
+			/*
+			 * Nothing to invalidate. A concurrent reader may be
+			 * using the cache right now and can safely continue
+			 * to do so; only the memslot generation, which is
+			 * read solely by kvm_gpc_check(), may need updating.
+			 */
+			if (new_generation != gpc->generation)
+				WRITE_ONCE(gpc->generation, new_generation);
+
+			return 0;
+		}
+	}
+
+	/*
+	 * Take the cache invalid and wait for all current readers to
+	 * drain before mutating anything they might be looking at. Once
+	 * the grace period has elapsed, this task (serialized by
+	 * refresh_lock) owns all the cache fields exclusively: readers
+	 * check valid (with an acquire load) inside their RCU read-side
+	 * critical sections and back off. This mirrors what a TLB
+	 * shootdown does for the hardware page tables.
+	 *
+	 * If the cache wasn't valid, concurrent readers were already
+	 * backing off, and any concurrent invalidation is handled by the
+	 * gpc_invalidate_seq check in hva_to_pfn_retry().
+	 */
+	was_valid = READ_ONCE(gpc->valid);
+	if (was_valid) {
+		WRITE_ONCE(gpc->valid, false);
+		synchronize_rcu();
 	}
 
 	old_pfn = gpc->pfn;
@@ -286,9 +373,6 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
 		gpc->gpa = INVALID_GPA;
 		gpc->memslot = NULL;
 		gpc->uhva = PAGE_ALIGN_DOWN(uhva);
-
-		if (gpc->uhva != old_uhva)
-			hva_change = true;
 	} else {
 		struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
 
@@ -308,12 +392,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
 				goto out;
 			}
 
-			/*
-			 * Even if the GPA and/or the memslot generation changed, the
-			 * HVA may still be the same.
-			 */
-			if (gpc->uhva != old_uhva)
-				hva_change = true;
+
 		} else {
 			gpc->uhva = old_uhva;
 		}
@@ -323,21 +402,16 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
 	gpc->uhva += page_offset;
 
 	/*
-	 * If the userspace HVA changed or the PFN was already invalid,
-	 * drop the lock and do the HVA to PFN lookup again.
+	 * Always redo the HVA to PFN lookup: an invalidation of our uhva
+	 * may have raced with (or followed) the valid-clearing above, in
+	 * which case the notifier walk skipped this cache (valid was
+	 * already false) and the old pfn may already be stale. The
+	 * gpc_invalidate_seq check in hva_to_pfn_retry() is what detects
+	 * that race, so the lookup path is the only safe way to publish.
+	 * If the mapping is in fact unchanged, hva_to_pfn_retry() reuses
+	 * the existing kernel mapping for the same pfn.
 	 */
-	if (!gpc->valid || hva_change) {
-		ret = hva_to_pfn_retry(gpc);
-	} else {
-		/*
-		 * If the HVA→PFN mapping was already valid, don't unmap it.
-		 * But do update gpc->khva because the offset within the page
-		 * may have changed.
-		 */
-		gpc->khva = old_khva + page_offset;
-		ret = 0;
-		goto out_unlock;
-	}
+	ret = hva_to_pfn_retry(gpc);
 
  out:
 	/*
@@ -346,17 +420,12 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
 	 * valid, leave it as is.
 	 */
 	if (ret) {
-		gpc->valid = false;
+		WARN_ON_ONCE(gpc->valid);
 		gpc->pfn = KVM_PFN_ERR_FAULT;
 		gpc->khva = NULL;
 	}
 
-	/* Detect a pfn change before dropping the lock! */
 	unmap_old = (old_pfn != gpc->pfn);
-
-out_unlock:
-	write_unlock_irq(&gpc->lock);
-
 	if (unmap_old)
 		gpc_unmap(old_pfn, old_khva);
 
@@ -384,7 +453,6 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len)
 
 void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)
 {
-	rwlock_init(&gpc->lock);
 	mutex_init(&gpc->refresh_lock);
 
 	gpc->kvm = kvm;
@@ -415,11 +483,11 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned
 		/*
 		 * Activate the cache after adding it to the list, a concurrent
 		 * refresh must not establish a mapping until the cache is
-		 * reachable by mmu_notifier events.
+		 * reachable by mmu_notifier events. (Refreshes are serialized
+		 * by refresh_lock, which we hold; the store ordering matters
+		 * only against the notifier walk, which holds gpc_lock.)
 		 */
-		write_lock_irq(&gpc->lock);
-		gpc->active = true;
-		write_unlock_irq(&gpc->lock);
+		WRITE_ONCE(gpc->active, true);
 	}
 	return __kvm_gpc_refresh(gpc, gpa, uhva);
 }
@@ -454,13 +522,16 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)
 
 	if (gpc->active) {
 		/*
-		 * Deactivate the cache before removing it from the list, KVM
-		 * must stall mmu_notifier events until all users go away, i.e.
-		 * until gpc->lock is dropped and refresh is guaranteed to fail.
+		 * Mark the cache inactive and invalid, and wait for all
+		 * current readers to drain, before tearing down the mapping
+		 * they may have been using. Refreshes are excluded by
+		 * refresh_lock, which we hold.
 		 */
-		write_lock_irq(&gpc->lock);
-		gpc->active = false;
-		gpc->valid = false;
+		WRITE_ONCE(gpc->active, false);
+		if (READ_ONCE(gpc->valid)) {
+			WRITE_ONCE(gpc->valid, false);
+			synchronize_rcu();
+		}
 
 		/*
 		 * Leave the GPA => uHVA cache intact, it's protected by the
@@ -473,7 +544,6 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)
 
 		old_pfn = gpc->pfn;
 		gpc->pfn = KVM_PFN_ERR_FAULT;
-		write_unlock_irq(&gpc->lock);
 
 		spin_lock(&kvm->gpc_lock);
 		list_del(&gpc->list);
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 5282 bytes --]

From: Sean Christopherson <seanjc@google.com>

Move the marking of an event as pending in the target vCPU's vcpu_info
into a separate helper, __kvm_xen_set_evtchn_fast(), invoked after the
shared_info processing is complete and its RCU read-side critical
section has been exited. This makes both halves easier to read.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
[dwmw2: rebased onto the RCU conversion of the GPC locking; the
 read_trylock() failure path in the original no longer exists. The
 caller's kvm->srcu section now extends across the helper call, since
 kvm_gpc_check() on the vcpu_info cache consults the memslot
 generation and the irqfd path enters holding only irq_srcu; in Sean's
 series that was covered by a guard(srcu) spanning the whole function,
 which this series does not carry.]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 arch/x86/kvm/xen.c | 104 ++++++++++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 6bcece2439f5..75fbe88c2102 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1759,6 +1759,67 @@ static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port)
 	}
 }
 
+/* Called with kvm->srcu held, as kvm_gpc_check() consults the memslots. */
+static void __kvm_xen_set_evtchn_fast(struct kvm_vcpu *vcpu, int port_word_bit)
+{
+	struct gfn_to_pfn_cache *gpc = &vcpu->arch.xen.vcpu_info_cache;
+	bool kick_vcpu = false;
+
+	/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
+	rcu_read_lock();
+	if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
+		/*
+		 * Could not access the vcpu_info. Set the bit in-kernel and
+		 * prod the vCPU to deliver it for itself.
+		 */
+		if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel))
+			kick_vcpu = true;
+		goto out_unlock;
+	}
+
+	if (IS_ENABLED(CONFIG_64BIT) && vcpu->kvm->arch.xen.long_mode) {
+		struct vcpu_info *vcpu_info = gpc->khva;
+
+		if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) {
+			WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
+			kick_vcpu = true;
+		}
+	} else {
+		struct compat_vcpu_info *vcpu_info = gpc->khva;
+
+		if (!test_and_set_bit(port_word_bit,
+				      (unsigned long *)&vcpu_info->evtchn_pending_sel)) {
+			WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
+			kick_vcpu = true;
+		}
+	}
+
+out_unlock:
+	rcu_read_unlock();
+
+	/*
+	 * Deliver the upcall or kick the vCPU only after dropping the RCU
+	 * read lock. Both paths end up in kvm_vcpu_kick(), and the MSI
+	 * delivery also walks the APIC map and takes APIC locks; none of
+	 * that wants to be nested inside the GPC read-side critical
+	 * section, which must be no longer than the accesses to gpc->khva
+	 * above. Invalidation waits for a grace period, so holding the
+	 * read lock across the kick would extend how long a memory
+	 * invalidation is blocked.
+	 */
+	if (!kick_vcpu)
+		return;
+
+	/* For the per-vCPU lapic vector, deliver it as MSI. */
+	if (vcpu->arch.xen.upcall_vector) {
+		kvm_xen_inject_vcpu_vector(vcpu);
+		return;
+	}
+
+	kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
+	kvm_vcpu_kick(vcpu);
+}
+
 /*
  * The return value from this function is propagated to kvm_set_irq() API,
  * so it returns:
@@ -1775,7 +1836,6 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	unsigned long *pending_bits, *mask_bits;
 	int port_word_bit;
-	bool kick_vcpu = false;
 	int vcpu_idx, idx, rc;
 
 	vcpu_idx = READ_ONCE(xe->vcpu_idx);
@@ -1825,49 +1885,15 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 		kvm_xen_check_poller(vcpu, xe->port);
 	} else {
 		rc = 1; /* Delivered to the bitmap in shared_info. */
-		/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
-		gpc = &vcpu->arch.xen.vcpu_info_cache;
-
-		if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
-			/*
-			 * Could not access the vcpu_info. Set the bit in-kernel
-			 * and prod the vCPU to deliver it for itself.
-			 */
-			if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel))
-				kick_vcpu = true;
-			goto out_rcu;
-		}
-
-		if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) {
-			struct vcpu_info *vcpu_info = gpc->khva;
-			if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) {
-				WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
-				kick_vcpu = true;
-			}
-		} else {
-			struct compat_vcpu_info *vcpu_info = gpc->khva;
-			if (!test_and_set_bit(port_word_bit,
-					      (unsigned long *)&vcpu_info->evtchn_pending_sel)) {
-				WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
-				kick_vcpu = true;
-			}
-		}
-
-		/* For the per-vCPU lapic vector, deliver it as MSI. */
-		if (kick_vcpu && vcpu->arch.xen.upcall_vector) {
-			kvm_xen_inject_vcpu_vector(vcpu);
-			kick_vcpu = false;
-		}
 	}
 
  out_rcu:
 	rcu_read_unlock();
-	srcu_read_unlock(&kvm->srcu, idx);
 
-	if (kick_vcpu) {
-		kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
-		kvm_vcpu_kick(vcpu);
-	}
+	if (rc == 1)
+		__kvm_xen_set_evtchn_fast(vcpu, port_word_bit);
+
+	srcu_read_unlock(&kvm->srcu, idx);
 
 	return rc;
 }
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 5/7] KVM: x86/xen: Explicitly tag "shared info" page as never being dirty tracked

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 3455 bytes --]

From: Sean Christopherson <seanjc@google.com>

Explicitly mark the Xen shared info page as never being dirty tracked so
that higher-level gpc APIs can be added to automatically take care of
things like dirty tracking, without reintroducing the bug fixed by commit
55749769fe60 ("KVM: x86: Fix wall clock writes in Xen shared_info not to
mark page dirty").  And because the code _looks_ buggy.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 arch/x86/kvm/xen.c        |  2 +-
 include/linux/kvm_host.h  | 10 ++++++++--
 include/linux/kvm_types.h |  1 +
 virt/kvm/pfncache.c       |  4 +++-
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 75fbe88c2102..ff18d888ed47 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -2329,7 +2329,7 @@ void kvm_xen_init_vm(struct kvm *kvm)
 {
 	mutex_init(&kvm->arch.xen.xen_lock);
 	idr_init(&kvm->arch.xen.evtchn_ports);
-	kvm_gpc_init(&kvm->arch.xen.shinfo_cache, kvm);
+	__kvm_gpc_init(&kvm->arch.xen.shinfo_cache, kvm, true);
 }
 
 void kvm_xen_destroy_vm(struct kvm *kvm)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index da0b3669f1a3..1725284121c9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1465,7 +1465,13 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
  * immutable attributes.  Note, the cache must be zero-allocated (or zeroed by
  * the caller before init).
  */
-void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm);
+void __kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm,
+		    bool never_dirty);
+
+static inline void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)
+{
+	__kvm_gpc_init(gpc, kvm, false);
+}
 
 /**
  * kvm_gpc_activate - prepare a cached kernel mapping and HPA for a given guest
@@ -1974,7 +1980,7 @@ static inline void kvm_gpc_mark_dirty_in_slot(struct gfn_to_pfn_cache *gpc)
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
 			 "kvm_gpc_mark_dirty_in_slot() without RCU read lock");
 
-	if (!gpc->memslot)
+	if (!gpc->memslot || gpc->never_dirty)
 		return;
 
 	mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpa_to_gfn(gpc->gpa));
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index fdf7d2273298..1ad35ec01f87 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -93,6 +93,7 @@ struct gfn_to_pfn_cache {
 	kvm_pfn_t pfn;
 	bool active;
 	bool valid;
+	bool never_dirty;
 };
 
 #ifdef KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 3bae4df49165..ffb4e6105344 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -451,7 +451,8 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len)
 	return __kvm_gpc_refresh(gpc, gpc->gpa, uhva);
 }
 
-void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)
+void __kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm,
+		    bool never_dirty)
 {
 	mutex_init(&gpc->refresh_lock);
 
@@ -460,6 +461,7 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm)
 	gpc->gpa = INVALID_GPA;
 	gpc->uhva = KVM_HVA_ERR_BAD;
 	gpc->active = gpc->valid = false;
+	gpc->never_dirty = never_dirty;
 }
 
 static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva,
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 6/7] KVM: x86/xen: Don't dirty track "vCPU info" page

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 1754 bytes --]

From: Sean Christopherson <seanjc@google.com>

Give the Xen per-vCPU info page the same treatment as the per-VM shared
info page, and never mark it dirty, as KVM clearly relies on userspace to
assume the page is always dirty.  While the page is marked dirty on writes
via kvm_xen_inject_pending_events(), it's not marked dirty when written by
__kvm_xen_set_evtchn_fast().

Furthermore, as was the case with the shared info page, writes in the event
channel fastpath may be done without an active vCPU, e.g. when called via
timer callback or irqfd injection.  I.e. attempting to fix the fastpath
would run afoul of the same issue that was fixed by commit 55749769fe60
("KVM: x86: Fix wall clock writes in Xen shared_info not to mark page
dirty").

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 arch/x86/kvm/xen.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index ff18d888ed47..9f1a9c93442d 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -659,7 +659,6 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 		WRITE_ONCE(vi->evtchn_upcall_pending, 1);
 	}
 
-	kvm_gpc_mark_dirty_in_slot(gpc);
 	rcu_read_unlock();
 
 	/* For the per-vCPU lapic vector, deliver it as MSI. */
@@ -2308,7 +2307,7 @@ void kvm_xen_init_vcpu(struct kvm_vcpu *vcpu)
 
 	kvm_gpc_init(&vcpu->arch.xen.runstate_cache, vcpu->kvm);
 	kvm_gpc_init(&vcpu->arch.xen.runstate2_cache, vcpu->kvm);
-	kvm_gpc_init(&vcpu->arch.xen.vcpu_info_cache, vcpu->kvm);
+	__kvm_gpc_init(&vcpu->arch.xen.vcpu_info_cache, vcpu->kvm, true);
 	kvm_gpc_init(&vcpu->arch.xen.vcpu_time_info_cache, vcpu->kvm);
 }
 
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

[PATCH v3 7/7] KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 10352 bytes --]

From: Carsten Stollmaier <stollmc@amazon.com>

This largely reverts commit 7e2175ebd695 ("KVM: x86: Fix recording of
guest steal time / preempted status"), which dropped the use of the
gfn_to_pfn_cache because it was not integrated with the MMU notifiers
at the time. That shortcoming has long since been addressed, making
the GPC work correctly for this use case.

Aside from cleaning up the last open-coded assembler access to user
addresses and associated explicit asm exception fixups, moving back
to the now-functional GPC also resolves an issue with contention on
the mmap_lock with userfaultfd. The contention issue is as follows:

On vcpu_run, before entering the guest, the update of the steal time
information causes a page-fault if the page is not present. In our
scenario, this gets handled by do_user_addr_fault() and successively
handle_userfault() because the region is registered to that.

Since handle_userfault() uses TASK_INTERRUPTIBLE, it is interruptible
by signals. But do_user_addr_fault() then busy-retries if the pending
signal is non-fatal, which leads to heavy contention of the mmap_lock.

By restoring the use of GPC for accessing the guest steal time, the
contention is avoided and refreshing the GPC happens when the vCPU is
next scheduled.

Since the gfn_to_pfn_cache gives a kernel mapping rather than a
userspace HVA, accesses are now plain C instead of unsafe_put_user()
et al. Use READ_ONCE()/WRITE_ONCE() to prevent the compiler from
reordering or tearing the accesses, and add an smp_wmb() before the
final version increment to ensure the data writes are ordered before
the seqcount update — the old unsafe_put_user() inline assembly acted
as an implicit compiler barrier.

In kvm_steal_time_set_preempted(), which is called from the scheduler
path via kvm_sched_out(), just take the RCU read lock and bail if the
cache is not currently valid. The cache cannot be refreshed from that
context, but setting the preempted flag is best-effort anyway — the
old HVA-based code used copy_to_user_nofault(), which could equally
silently fail — and kvm_arch_vcpu_load() unconditionally raises
KVM_REQ_STEAL_UPDATE, so record_steal_time() will refresh the cache
from task context before the vCPU next enters the guest.

[dwmw2: Port to RCU-protected GPC. The original was written against a
        gpc->lock rwlock and needed a read_trylock() dance to be safe
        from the scheduler path on PREEMPT_RT; rcu_read_lock() is
        unconditionally safe there, so all of that goes away.]

Signed-off-by: Carsten Stollmaier <stollmc@amazon.com>
Co-developed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Assisted-by: Claude:claude-mythos-5
---
 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/msrs.c             |   7 +-
 arch/x86/kvm/x86.c              | 119 +++++++++++++++-----------------
 3 files changed, 62 insertions(+), 66 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 283847619ff8..93514cb1f2f7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -873,7 +873,7 @@ struct kvm_vcpu_arch {
 		u8 preempted;
 		u64 msr_val;
 		u64 last_steal;
-		struct gfn_to_hva_cache cache;
+		struct gfn_to_pfn_cache cache;
 	} st;
 
 	u64 l1_tsc_offset;
diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index 66fa7140d65d..37d79d84e2f8 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -1717,8 +1717,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 		vcpu->arch.st.msr_val = data;
 
-		if (!(data & KVM_MSR_ENABLED))
+		if (!(data & KVM_MSR_ENABLED)) {
+			kvm_gpc_deactivate(&vcpu->arch.st.cache);
 			break;
+		}
+
+		kvm_gpc_activate(&vcpu->arch.st.cache, data & ~KVM_MSR_ENABLED,
+				 sizeof(struct kvm_steal_time));
 
 		kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 09425f72f4a5..4a5753623909 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2046,10 +2046,8 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_service_local_tlb_flush_requests);
 
 static void record_steal_time(struct kvm_vcpu *vcpu)
 {
-	struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache;
-	struct kvm_steal_time __user *st;
-	struct kvm_memslots *slots;
-	gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
+	struct gfn_to_pfn_cache *gpc = &vcpu->arch.st.cache;
+	struct kvm_steal_time *st;
 	u64 steal;
 	u32 version;
 
@@ -2064,42 +2062,27 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
 	if (WARN_ON_ONCE(current->mm != vcpu->kvm->mm))
 		return;
 
-	slots = kvm_memslots(vcpu->kvm);
+	/* We rely on the fact that it fits in a single page. */
+	BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
 
-	if (unlikely(slots->generation != ghc->generation ||
-		     gpa != ghc->gpa ||
-		     kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
-		/* We rely on the fact that it fits in a single page. */
-		BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
+	rcu_read_lock();
+	while (!kvm_gpc_check(gpc, sizeof(*st))) {
+		rcu_read_unlock();
 
-		if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
-		    kvm_is_error_hva(ghc->hva) || !ghc->memslot)
+		if (kvm_gpc_refresh(gpc, sizeof(*st)))
 			return;
+
+		rcu_read_lock();
 	}
 
-	st = (struct kvm_steal_time __user *)ghc->hva;
+	st = gpc->khva;
+
 	/*
 	 * Doing a TLB flush here, on the guest's behalf, can avoid
 	 * expensive IPIs.
 	 */
 	if (guest_pv_has(vcpu, KVM_FEATURE_PV_TLB_FLUSH)) {
-		u8 st_preempted = 0;
-		int err = -EFAULT;
-
-		if (!user_access_begin(st, sizeof(*st)))
-			return;
-
-		asm volatile("1: xchgb %0, %2\n"
-			     "xor %1, %1\n"
-			     "2:\n"
-			     _ASM_EXTABLE_UA(1b, 2b)
-			     : "+q" (st_preempted),
-			       "+&r" (err),
-			       "+m" (st->preempted));
-		if (err)
-			goto out;
-
-		user_access_end();
+		u8 st_preempted = xchg(&st->preempted, 0);
 
 		vcpu->arch.st.preempted = 0;
 
@@ -2107,39 +2090,33 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
 				       st_preempted & KVM_VCPU_FLUSH_TLB);
 		if (st_preempted & KVM_VCPU_FLUSH_TLB)
 			kvm_vcpu_flush_tlb_guest(vcpu);
-
-		if (!user_access_begin(st, sizeof(*st)))
-			goto dirty;
 	} else {
-		if (!user_access_begin(st, sizeof(*st)))
-			return;
-
-		unsafe_put_user(0, &st->preempted, out);
+		WRITE_ONCE(st->preempted, 0);
 		vcpu->arch.st.preempted = 0;
 	}
 
-	unsafe_get_user(version, &st->version, out);
+	version = READ_ONCE(st->version);
 	if (version & 1)
 		version += 1;  /* first time write, random junk */
 
 	version += 1;
-	unsafe_put_user(version, &st->version, out);
+	WRITE_ONCE(st->version, version);
 
 	smp_wmb();
 
-	unsafe_get_user(steal, &st->steal, out);
+	steal = READ_ONCE(st->steal);
 	steal += current->sched_info.run_delay -
 		vcpu->arch.st.last_steal;
 	vcpu->arch.st.last_steal = current->sched_info.run_delay;
-	unsafe_put_user(steal, &st->steal, out);
+	WRITE_ONCE(st->steal, steal);
+
+	smp_wmb();
 
 	version += 1;
-	unsafe_put_user(version, &st->version, out);
+	WRITE_ONCE(st->version, version);
 
- out:
-	user_access_end();
- dirty:
-	mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa));
+	kvm_gpc_mark_dirty_in_slot(gpc);
+	rcu_read_unlock();
 }
 
 static inline bool kvm_can_mwait_in_guest(void)
@@ -2613,11 +2590,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
 {
-	struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache;
-	struct kvm_steal_time __user *st;
-	struct kvm_memslots *slots;
-	static const u8 preempted = KVM_VCPU_PREEMPTED;
-	gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
+	struct gfn_to_pfn_cache *gpc = &vcpu->arch.st.cache;
+	struct kvm_steal_time *st;
 
 	/*
 	 * The vCPU can be marked preempted if and only if the VM-Exit was on
@@ -2642,20 +2616,32 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
 	if (unlikely(current->mm != vcpu->kvm->mm))
 		return;
 
-	slots = kvm_memslots(vcpu->kvm);
-
-	if (unlikely(slots->generation != ghc->generation ||
-		     gpa != ghc->gpa ||
-		     kvm_is_error_hva(ghc->hva) || !ghc->memslot))
-		return;
-
-	st = (struct kvm_steal_time __user *)ghc->hva;
-	BUILD_BUG_ON(sizeof(st->preempted) != sizeof(preempted));
-
-	if (!copy_to_user_nofault(&st->preempted, &preempted, sizeof(preempted)))
+	/*
+	 * This is called from the scheduler path (via kvm_sched_out), so
+	 * the cache cannot be refreshed here.  Setting 'preempted' is
+	 * best-effort anyway; the old HVA-based code used
+	 * copy_to_user_nofault(), which could also silently fail.  If the
+	 * cache is invalid, it will be refreshed from task context by
+	 * record_steal_time() before the vCPU next enters the guest, since
+	 * kvm_arch_vcpu_load() unconditionally raises KVM_REQ_STEAL_UPDATE.
+	 */
+	rcu_read_lock();
+	if (kvm_gpc_check(gpc, sizeof(*st))) {
+		st = gpc->khva;
+		WRITE_ONCE(st->preempted, KVM_VCPU_PREEMPTED);
 		vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED;
 
-	mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa));
+		kvm_gpc_mark_dirty_in_slot(gpc);
+	}
+	rcu_read_unlock();
+}
+
+static void kvm_steal_time_reset(struct kvm_vcpu *vcpu)
+{
+	kvm_gpc_deactivate(&vcpu->arch.st.cache);
+	vcpu->arch.st.preempted = 0;
+	vcpu->arch.st.msr_val = 0;
+	vcpu->arch.st.last_steal = 0;
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -9347,6 +9333,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 
 	kvm_gpc_init(&vcpu->arch.pv_time, vcpu->kvm);
 
+	kvm_gpc_init(&vcpu->arch.st.cache, vcpu->kvm);
+
 	if (!irqchip_in_kernel(vcpu->kvm) || kvm_vcpu_is_reset_bsp(vcpu))
 		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 	else
@@ -9454,6 +9442,8 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_clear_async_pf_completion_queue(vcpu);
 	kvm_mmu_unload(vcpu);
 
+	kvm_steal_time_reset(vcpu);
+
 	kvmclock_reset(vcpu);
 
 	for_each_possible_cpu(cpu)
@@ -9574,7 +9564,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	vcpu->arch.apf.msr_en_val = 0;
 	vcpu->arch.apf.msr_int_val = 0;
-	vcpu->arch.st.msr_val = 0;
+
+	kvm_steal_time_reset(vcpu);
 
 	kvmclock_reset(vcpu);
 
-- 
2.43.0


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 15938 bytes --]

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Time-of-check to time-of-use (TOCTOU) race in the lockless MMU notifier retry protocol, leading to a Use-After-Free of physical pages.
- [Critical] Conditional execution of `synchronize_rcu()` based on `gpc->valid == false` bypasses the grace period, leading to Use-After-Free.
- [Medium] vCPU kick nested inside RCU read-side critical section, extending memory invalidation latency.
- [Medium] Data race on `gpc->generation` in `kvm_gpc_check()` due to missing `READ_ONCE()`.

Pre-existing issues:
- [Critical] Multiple callers access KVM memslot structures without holding `srcu_read_lock(&kvm->srcu)`, leading to Use-After-Free of memslots and their dirty bitmaps.
--

--- Patch [3]: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock ---
commit c7d30d550717790c51bcbb357dd39cfdd3210219
Author: Woodhouse, David <dwmw@amazon.co.uk>

KVM: pfncache: Use RCU for readers instead of a rwlock

Replace the per-cache rwlock with RCU for the read side. Readers now
run under rcu_read_lock() alone, which works in any context.

Link: https://syzkaller.appspot.com/bug?extid=208f7f3e5f59c11aeb90

> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -416,20 +414,12 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
>  
>   retry:
>  	/*
> -	 * Attempt to obtain the GPC lock on *both* (if there are two)
> -	 * gfn_to_pfn caches that cover the region.
> +	 * Check *both* (if there are two) gfn_to_pfn caches that cover
> +	 * the region, under a single RCU read-side critical section.
>  	 */
> -	if (atomic) {
> -		local_irq_save(flags);
> -		if (!read_trylock(&gpc1->lock)) {
> -			local_irq_restore(flags);
> -			return;
> -		}
> -	} else {
> -		read_lock_irqsave(&gpc1->lock, flags);
> -	}
> +	rcu_read_lock();
>  	while (!kvm_gpc_check(gpc1, user_len1)) {
> -		read_unlock_irqrestore(&gpc1->lock, flags);
> +		rcu_read_unlock();

[Severity: Critical]
This is a pre-existing issue, but does this code need to hold
srcu_read_lock(&kvm->srcu) before accessing the KVM memslot structures?

kvm_xen_update_runstate_guest initiates the access sequence holding only
rcu_read_lock. Later, kvm_gpc_mark_dirty_in_slot dereferences gpc->memslot.

Since rcu_read_lock does not block synchronize_srcu when memslots are
deleted, could this allow a memslot to be freed concurrently while a reader
is iterating over it or marking pages dirty?

[ ... ]
> @@ -1855,10 +1826,8 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
>  	/*
>  	 * If this port wasn't already set, and if it isn't masked, then
>  	 * we try to set the corresponding bit in the in-kernel shadow of
>  	 * evtchn_pending_sel for the target vCPU. And if *that* wasn't
>  	 * already set, then we kick the vCPU in question to write to the
>  	 * *real* evtchn_pending_sel in its own guest vcpu_info struct.
>  	 */
>  	if (test_and_set_bit(xe->port, pending_bits)) {
>  		rc = 0; /* It was already raised */
>  	} else if (test_bit(xe->port, mask_bits)) {
>  		rc = -ENOTCONN; /* Masked */
>  		kvm_xen_check_poller(vcpu, xe->port);
>  	} else {
>  		rc = 1; /* Delivered to the bitmap in shared_info. */
>  		/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
> -		read_unlock_irqrestore(&gpc->lock, flags);
>  		gpc = &vcpu->arch.xen.vcpu_info_cache;
>  
> -		read_lock_irqsave(&gpc->lock, flags);
>  		if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
>  			/*
>  			 * Could not access the vcpu_info. Set the bit in-kernel
> @@ -1892,7 +1861,7 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
>  	}
>  
>   out_rcu:
> -	read_unlock_irqrestore(&gpc->lock, flags);
> +	rcu_read_unlock();
>  	srcu_read_unlock(&kvm->srcu, idx);

[Severity: Medium]
Does the fallback path for masked event channels synchronously issue a vCPU
kick while still inside the RCU read-side critical section?

In kvm_xen_set_evtchn_fast, if the port is masked, kvm_xen_check_poller is
called directly, which eventually executes kvm_vcpu_kick.

Since this path doesn't defer the kick until after out_rcu and
rcu_read_unlock, could this artificially extend the grace period and delay
host memory invalidations?

> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c
> @@ -26,35 +26,49 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
>  				       unsigned long end)
>  {
>  	struct gfn_to_pfn_cache *gpc;
> +	bool cleared = false;
>  
>  	spin_lock(&kvm->gpc_lock);
>  	list_for_each_entry(gpc, &kvm->gpc_list, list) {
> -		read_lock_irq(&gpc->lock);
> -
> -		/* Only a single page so no need to care about length */
> -		if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
> +		if (smp_load_acquire(&gpc->valid) &&
>  		    gpc->uhva >= start && gpc->uhva < end) {
> -			read_unlock_irq(&gpc->lock);
> -
> -			/*
> -			 * There is a small window here where the cache could
> -			 * be modified, and invalidation would no longer be
> -			 * necessary. Hence check again whether invalidation
> -			 * is still necessary once the write lock has been
> -			 * acquired.
> -			 */
> -
> -			write_lock_irq(&gpc->lock);
> -			if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
> -			    gpc->uhva >= start && gpc->uhva < end)
> -				gpc->valid = false;
> -			write_unlock_irq(&gpc->lock);
> -			continue;
> +			WRITE_ONCE(gpc->valid, false);
> +			cleared = true;
>  		}
> -
> -		read_unlock_irq(&gpc->lock);
>  	}
>  	spin_unlock(&kvm->gpc_lock);
> +
> +	/*
> +	 * Readers may still be using the old mapping, having sampled
> +	 * gpc->valid before it was cleared. Wait for them all to drain
> +	 * before the caller proceeds to zap the page tables; like a TLB
> +	 * shootdown, this guarantees no access via the stale mapping
> +	 * once the invalidation completes.
> +	 *
> +	 * This wait is safe even on unblockable ranges (the OOM reaper):
> +	 * the constraint there is not "no sleeping" but "no blocking on
> +	 * anything which may itself depend on memory allocation to make
> +	 * progress" (see the reasoning in commit 312364f3534c and its
> +	 * discussion). An RCU grace period has no such dependency: GPC
> +	 * readers never allocate, never take mmap_lock, and never sleep.
> +	 */
> +	if (cleared)
> +		synchronize_rcu();
>  }

[Severity: Critical]
Could this conditional wait bypass the grace period and lead to a regression
with physical pages being freed too early?

If a concurrent mutator like __kvm_gpc_refresh transitions gpc->valid to
false and is currently blocking on its own synchronize_rcu wait, the MMU
notifier will see valid as false, skip the wait since cleared is false, and
return immediately.

This would allow the host memory subsystem to free the page while the
original mutator is still waiting for active RCU readers to finish.

[ ... ]
> @@ -74,6 +88,22 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
>  {
>  	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
>  
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> +			 "kvm_gpc_check() without RCU read lock");
> +
> +	/*
> +	 * Check valid *first*. The acquire pairs with the release-publish
> +	 * in hva_to_pfn_retry(), so every field read below — and any use
> +	 * of gpc->khva by the caller — is guaranteed to be from the
> +	 * published generation, not a stale value reordered from before
> +	 * the publish. The fields are then stable for the remainder of
> +	 * the RCU read-side critical section, because every mutator
> +	 * clears valid and waits a full grace period before changing
> +	 * anything.
> +	 */
> +	if (!smp_load_acquire(&gpc->valid))
> +		return false;
> +
>  	if (!gpc->active)
>  		return false;

[Severity: Medium]
Should the read of gpc->generation use READ_ONCE?

Concurrent writers in __kvm_gpc_refresh modify gpc->generation using
WRITE_ONCE. Omitting READ_ONCE here in kvm_gpc_check might theoretically
result in a torn 64-bit read on 32-bit KVM host builds, potentially matching
a bogus slots->generation and resulting in a false-positive validation of
the cache.

[ ... ]
> @@ -224,20 +242,17 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
>  			kvm_release_page_unused(page);
>  			goto out_error;
>  		}
> -
> -		write_lock_irq(&gpc->lock);
> -
> -		/*
> -		 * Other tasks must wait for _this_ refresh to complete before
> -		 * attempting to refresh.
> -		 */
> -		WARN_ON_ONCE(gpc->valid);
>  	} while (mmu_notifier_retry_cache(gpc->kvm, gpc_seq));
>  
> -	gpc->valid = true;
>  	gpc->pfn = new_pfn;
>  	gpc->khva = new_khva + offset_in_page(gpc->uhva);
>  
> +	/*
> +	 * Publish. Pairs with the smp_load_acquire() in kvm_gpc_check();
> +	 * the pfn/khva stores above must be visible before valid is.
> +	 */
> +	smp_store_release(&gpc->valid, true);
> +
>  	/*
>  	 * Put the reference to the _new_ page.  The page is now tracked by the
>  	 * cache and can be safely migrated, swapped, etc... as the cache will

[Severity: Critical]
Is a barrier needed here to order the mmu_notifier_retry_cache check against
the publication of gpc->valid?

If the MMU notifier starts executing after the retry check but before the
valid flag is published, it could observe valid as false in
gfn_to_pfn_cache_invalidate_start and skip invalidation.

This could allow the MMU notifier to proceed to zap the page tables and the
memory subsystem to free the physical page, while this function completes
and leaves the cache marked as valid but pointing to a freed physical page.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805195528.3853473-1-dwmw@amazon.co.uk?part=3

Re: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The refactored helper injects an MSI instead of forcing a VM-exit when the vcpu_info cache check fails.

Pre-existing issues:
- [Critical] The host can be forced to perform unaligned atomic operations on guest-controlled memory, leading to a Denial of Service via split lock.
--

--- Patch [4]: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper ---
commit de0f33b78cabce7ad4bdfa1d85f662391c0c5256
Author: Woodhouse, David <dwmw@amazon.co.uk>
KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

This commit moves the marking of an event as pending in the target vCPU's
vcpu_info into a separate helper function, __kvm_xen_set_evtchn_fast().
The intent is to improve readability without causing functional changes.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c

[ ... ]

> +/* Called with kvm->srcu held, as kvm_gpc_check() consults the memslots. */
> +static void __kvm_xen_set_evtchn_fast(struct kvm_vcpu *vcpu, int port_word_bit)
> +{
> +	struct gfn_to_pfn_cache *gpc = &vcpu->arch.xen.vcpu_info_cache;
> +	bool kick_vcpu = false;
> +
> +	/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
> +	rcu_read_lock();
> +	if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
> +		/*
> +		 * Could not access the vcpu_info. Set the bit in-kernel and
> +		 * prod the vCPU to deliver it for itself.
> +		 */
> +		if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel))
> +			kick_vcpu = true;
> +		goto out_unlock;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_64BIT) && vcpu->kvm->arch.xen.long_mode) {
> +		struct vcpu_info *vcpu_info = gpc->khva;
> +
> +		if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) {

[Severity: Critical]
This is a pre-existing issue, but does test_and_set_bit() here risk a
host denial of service via a hardware split lock?

If a guest or userspace sets the vcpu_info GPA via
KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO to an unaligned address such that
evtchn_pending_sel spans a 64-byte cache line boundary, the 8-byte
atomic lock bts instruction could trigger an #AC exception.

If the host kernel is configured with split_lock_detect=fatal and
panic_on_oops, this would panic the host.

This also applies to the 32-bit compat branch below where the pointer
is cast to unsigned long *.

> +			WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
> +			kick_vcpu = true;
> +		}
> +	} else {
> +		struct compat_vcpu_info *vcpu_info = gpc->khva;
> +
> +		if (!test_and_set_bit(port_word_bit,
> +				      (unsigned long *)&vcpu_info->evtchn_pending_sel)) {
> +			WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
> +			kick_vcpu = true;
> +		}
> +	}
> +
> +out_unlock:
> +	rcu_read_unlock();
> +
> +	/*
> +	 * Deliver the upcall or kick the vCPU only after dropping the RCU
> +	 * read lock. Both paths end up in kvm_vcpu_kick(), and the MSI
> +	 * delivery also walks the APIC map and takes APIC locks; none of
> +	 * that wants to be nested inside the GPC read-side critical
> +	 * section, which must be no longer than the accesses to gpc->khva
> +	 * above. Invalidation waits for a grace period, so holding the
> +	 * read lock across the kick would extend how long a memory
> +	 * invalidation is blocked.
> +	 */
> +	if (!kick_vcpu)
> +		return;
> +
> +	/* For the per-vCPU lapic vector, deliver it as MSI. */
> +	if (vcpu->arch.xen.upcall_vector) {
> +		kvm_xen_inject_vcpu_vector(vcpu);
> +		return;
> +	}

[Severity: High]
Does this new helper inadvertently bypass the required vCPU kick when the
vcpu_info cache check fails?

In the original code, if kvm_gpc_check() failed, the code jumped to the
out_rcu label. This intentionally bypassed the MSI injection block, and
fell straight through to kvm_vcpu_kick(). This forced a VM-exit so KVM
could synchronize the pending events to guest memory via
kvm_xen_inject_pending_events().

With this refactored helper, if the cache check fails and kick_vcpu is set
to true, the code jumps to out_unlock, drops the RCU read lock, and then
unconditionally injects an MSI if upcall_vector is present. Because it
returns early, the kvm_vcpu_kick() is skipped.

If APICv posted interrupts deliver the MSI directly without a VM-exit,
won't KVM fail to sync the in-kernel pending bit, causing the guest to
read stale memory and drop the interrupt?

> +
> +	kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> +	kvm_vcpu_kick(vcpu);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805195528.3853473-1-dwmw@amazon.co.uk?part=4

Re: [PATCH v3 7/7] KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Sleeping in RCU read-side critical section due to TLB flush
--

--- Patch [7]: [PATCH v3 7/7] KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status ---
commit 7254897a5d4882376acaff67e062dd687ad49ea0
Author: Woodhouse, David <dwmw@amazon.co.uk>
KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

This commit transitions the guest steal time and preempted status recording
to use the gfn_to_pfn_cache, resolving mmap_lock contention associated with
userfaultfd. It replaces explicit asm exception fixups and unsafe_put_user()
calls with plain C accesses protected by rcu_read_lock().

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -2064,42 +2062,27 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
>  	if (WARN_ON_ONCE(current->mm != vcpu->kvm->mm))
>  		return;
>  
> -	slots = kvm_memslots(vcpu->kvm);
> +	/* We rely on the fact that it fits in a single page. */
> +	BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
>  
> -	if (unlikely(slots->generation != ghc->generation ||
> -		     gpa != ghc->gpa ||
> -		     kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
> -		/* We rely on the fact that it fits in a single page. */
> -		BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
> +	rcu_read_lock();
> +	while (!kvm_gpc_check(gpc, sizeof(*st))) {
[ ... ]
> +		if (kvm_gpc_refresh(gpc, sizeof(*st)))
>  			return;
> +
> +		rcu_read_lock();
>  	}
>  
> -	st = (struct kvm_steal_time __user *)ghc->hva;
> +	st = gpc->khva;
> +
[ ... ]
>  	if (guest_pv_has(vcpu, KVM_FEATURE_PV_TLB_FLUSH)) {
> -		u8 st_preempted = 0;
[ ... ]
> +		u8 st_preempted = xchg(&st->preempted, 0);
>  
>  		vcpu->arch.st.preempted = 0;
>  
> @@ -2107,39 +2090,33 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
>  				       st_preempted & KVM_VCPU_FLUSH_TLB);
>  		if (st_preempted & KVM_VCPU_FLUSH_TLB)
>  			kvm_vcpu_flush_tlb_guest(vcpu);

[Severity: Critical]
Can this sleep in an RCU read-side critical section?

When TDP is disabled and shadow paging is in use, kvm_vcpu_flush_tlb_guest()
calls kvm_mmu_sync_roots(), which can yield the CPU.

Looking at the call chain:

kvm_vcpu_flush_tlb_guest()
  -> kvm_mmu_sync_roots()
    -> mmu_sync_children()
      -> cond_resched_rwlock_write(&vcpu->kvm->mmu_lock)

Because this TLB flush is now called while holding rcu_read_lock() for the
gfn_to_pfn_cache access, will this trigger a scheduling while atomic panic
and potential RCU stalls when a guest requests a TLB flush?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805195528.3853473-1-dwmw@amazon.co.uk?part=7

Re: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

[David Woodhouse]

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

On Wed, 2026-08-05 at 20:47 +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Does this new helper inadvertently bypass the required vCPU kick when the
> vcpu_info cache check fails?

Oops, I frowned at that for doing the kick *within* the RCU read section,
and missed the bug when moving it down. Will fix.

> [Severity: Critical]
> This is a pre-existing issue, but does test_and_set_bit() here risk a
> host denial of service via a hardware split lock?

It does, and likewise for the lockless double read of long_mode
reported against v2 patch 7¹: max_evtchn_port() and the branch below it
can disagree, in which case port_word_bit can be up to 127 and the
test_and_set_bit() runs off the end of evtchn_pending_sel.

I knew I'd looked at those both before... they're both covered by the
series I posted in June:

  https://lore.kernel.org/all/20260605143034.3603-1-dwmw2@infradead.org/

I owe a refresh and repost of that one.




¹ https://lore.kernel.org/all/20260529174710.29CB31F00893@smtp.kernel.org/

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

Re: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

[David Woodhouse]

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

On Thu, 2026-08-06 at 00:35 +0200, David Woodhouse wrote:
> > [Severity: Critical]
> > This is a pre-existing issue, but does test_and_set_bit() here risk a
> > host denial of service via a hardware split lock?
> 
> It does, and likewise for the lockless double read of long_mode
> reported against v2 patch 7¹: max_evtchn_port() and the branch below it
> can disagree, in which case port_word_bit can be up to 127 and the
> test_and_set_bit() runs off the end of evtchn_pending_sel.
> 
> I knew I'd looked at those both before... they're both covered by the
> series I posted in June:
> 
>   https://lore.kernel.org/all/20260605143034.3603-1-dwmw2@infradead.org/
> 
> I owe a refresh and repost of that one.

Hm, it probably wants to live before the RCU part. I think the plan
should be to first send these in parallel as they don't conflict:

 • Patch 1 from this series (the syzbot races).
 • The updated xen series.

Separately, we can remove the non_block_start() from the OOM reaper
path (patch 2 of this series), marking it as fixing the existing KVM
mn_invalidate_lock splat under RT¹.

The actual conversion to RCU (which fixes the GPC RT issues) can be
rebased on top of all of the above, and wait in the wings for a little
while longer until they've landed.

I'll build a 'xen-rcu' branch with that all in a single linear set, and
redo all the testing.


¹ https://lore.kernel.org/all/787aa26cf62dfd361eea8ed19f384fc517892501.camel@infradead.org/

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

Re: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

[David Woodhouse]

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

On Thu, 2026-08-06 at 12:00 +0200, David Woodhouse wrote:
> 
> Hm, it probably wants to live before the RCU part. I think the plan
> should be to first send these in parallel as they don't conflict:
> 
>  • Patch 1 from this series (the syzbot races).
>  • The updated xen series.
> 
> Separately, we can remove the non_block_start() from the OOM reaper
> path (patch 2 of this series), marking it as fixing the existing KVM
> mn_invalidate_lock splat under RT¹.
> 
> The actual conversion to RCU (which fixes the GPC RT issues) can be
> rebased on top of all of the above, and wait in the wings for a little
> while longer until they've landed.
> 
> I'll build a 'xen-rcu' branch with that all in a single linear set, and
> redo all the testing.

Pushed to
https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu

I'm not going to spam yet another patch series just yet; I'll wait for
Sean to opine. There's an extra SRCU fix that's tacked on to the end of
the xen series, which showed up in testing. 

bfc8471128bb (xen-rcu) KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status
641309005314 KVM: x86: Request the guest TLB flush from record_steal_time()
f36625fcee41 KVM: x86/xen: Don't dirty track "vCPU info" page
ee5a6622c42d KVM: x86/xen: Explicitly tag "shared info" page as never being dirty tracked
77cf50babaa2 KVM: x86/xen: Extract delivery of event to vCPU into a separate helper
5435b60f92ad KVM: pfncache: Use RCU for readers instead of a rwlock
bee88a71959c mm/mmu_notifier: Remove non_block_start/end() from notifier invocation
c3b0e9347be8 (syzbot) KVM: pfncache: use a dedicated invalidation sequence for cache refresh
e8c3b5bdc2b1 (xen-v2) KVM: x86/xen: Take kvm->srcu in __kvm_xen_has_interrupt()
bc35ad561196 KVM: x86/xen: Use atomic*() APIs instead of open coded equivalents
b363d2ca18aa KVM: x86/xen: Use 32-bit atomics if vCPU's evtchn_pending_sel isn't aligned
da38f64bd735 KVM: x86/xen: Use 32-bit locked bts for vcpu_info evtchn_pending_sel
725a62d774ea KVM: x86/xen: Enforce 4-byte alignment of vcpu_info registration
0267ff414f71 KVM: x86/xen: Latch shinfo mode in kvm_xen_schedop_poll()
6ceac154447d KVM: x86/xen: Latch shinfo mode in kvm_xen_set_evtchn_fast()
3300e2ca471e KVM: x86/xen: Rename max_evtchn_port() to kvm_max_evtchn_port()
e8b7e8ae6de8 KVM: x86/xen: Introduce kvm_xen_has_64bit_shinfo() macro
9643c9d83837 KVM: x86/xen: Rename 'longmode' to 'is_64bit' in hypercall handling

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

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Sean Christopherson]

On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> Replace the per-cache rwlock with RCU for the read side.

I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
a non-starter.

Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
the wait time can easily get to 20+ milliseconds, which again is a non-starter for
things like steal-time updates and nVMX pages.  Simply using a per-GPC SRCU would
be gross, as gfn_to_pfn_cache_invalidate_start() would become absurdly complex in
order to juggle gpc_lock with synchronize_srcu_expedited().

A somewhat crazy idea would be to have a per-VM gpc_srcu, *and* a per-GPC srcu.
Readers would take both, refresh() would sync gpc->srcu, and invalidation would
sync kvm->gpc_srcu.  That way, refresh() wouldn't need to wait on concurrent
readers of *other* GPCs.

Actually, a better idea: use kvm->gpc_srcu to synchronize invalidations and
refresh() for GPCs that aren't tightly coupled to a vCPU, but for GPCs that are
*only* accessed by a single loaded vCPU, protect readers and refresh() with
vcpu->mutex.  That way, single-vCPU GPCs wouldn't need to synchronize() on refresh(),
because by definition there can't be concurrent readers with refresh().

That would basically punt on optimizing most of the Xen GPCs, but that's probably
ok?  Because the hot path GPCs, e.g. runstate_cache{,2}, are generally associated
1:1 with a vCPU, i.e. can avoid synchronizing on SRCU.  The one GPC that I see
as being problematic is vcpu_info_cache, because it's accesses cross-vCPU and so
the owning vCPU would need to synchronize() on refresh().  But if you're ok with
potentially high tail latencies if the vcpu_info_cache page is migrated or
reclaimed, then I doubt anyone else will complain.

> > --- a/virt/kvm/pfncache.c
> > +++ b/virt/kvm/pfncache.c
> > @@ -26,35 +26,49 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
> >  				       unsigned long end)
> >  {
> >  	struct gfn_to_pfn_cache *gpc;
> > +	bool cleared = false;
> >  
> >  	spin_lock(&kvm->gpc_lock);
> >  	list_for_each_entry(gpc, &kvm->gpc_list, list) {
> > -		read_lock_irq(&gpc->lock);
> > -
> > -		/* Only a single page so no need to care about length */
> > -		if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
> > +		if (smp_load_acquire(&gpc->valid) &&
> >  		    gpc->uhva >= start && gpc->uhva < end) {
> > -			read_unlock_irq(&gpc->lock);
> > -
> > -			/*
> > -			 * There is a small window here where the cache could
> > -			 * be modified, and invalidation would no longer be
> > -			 * necessary. Hence check again whether invalidation
> > -			 * is still necessary once the write lock has been
> > -			 * acquired.
> > -			 */
> > -
> > -			write_lock_irq(&gpc->lock);
> > -			if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
> > -			    gpc->uhva >= start && gpc->uhva < end)
> > -				gpc->valid = false;
> > -			write_unlock_irq(&gpc->lock);
> > -			continue;
> > +			WRITE_ONCE(gpc->valid, false);
> > +			cleared = true;
> >  		}
> > -
> > -		read_unlock_irq(&gpc->lock);
> >  	}
> >  	spin_unlock(&kvm->gpc_lock);
> > +
> > +	/*
> > +	 * Readers may still be using the old mapping, having sampled
> > +	 * gpc->valid before it was cleared. Wait for them all to drain
> > +	 * before the caller proceeds to zap the page tables; like a TLB
> > +	 * shootdown, this guarantees no access via the stale mapping
> > +	 * once the invalidation completes.
> > +	 *
> > +	 * This wait is safe even on unblockable ranges (the OOM reaper):
> > +	 * the constraint there is not "no sleeping" but "no blocking on
> > +	 * anything which may itself depend on memory allocation to make
> > +	 * progress" (see the reasoning in commit 312364f3534c and its
> > +	 * discussion). An RCU grace period has no such dependency: GPC
> > +	 * readers never allocate, never take mmap_lock, and never sleep.
> > +	 */
> > +	if (cleared)
> > +		synchronize_rcu();
> >  }
> 
> [Severity: Critical]
> Could this conditional wait bypass the grace period and lead to a regression
> with physical pages being freed too early?
> 
> If a concurrent mutator like __kvm_gpc_refresh transitions gpc->valid to
> false and is currently blocking on its own synchronize_rcu wait, the MMU
> notifier will see valid as false, skip the wait since cleared is false, and
> return immediately.
> 
> This would allow the host memory subsystem to free the page while the
> original mutator is still waiting for active RCU readers to finish.

Ya, pretty sure, Sashiko is correct.  The mmu_notifier rules require that all
mappings are removed before .invalidate_range_start() completes.  Seems like the
easiest solution is to key off of gpc->active, not gpc->valid.

> [ ... ]
> > @@ -74,6 +88,22 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
> >  {
> >  	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
> >  
> > +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> > +			 "kvm_gpc_check() without RCU read lock");
> > +
> > +	/*
> > +	 * Check valid *first*. The acquire pairs with the release-publish
> > +	 * in hva_to_pfn_retry(), so every field read below — and any use
> > +	 * of gpc->khva by the caller — is guaranteed to be from the
> > +	 * published generation, not a stale value reordered from before
> > +	 * the publish. The fields are then stable for the remainder of
> > +	 * the RCU read-side critical section, because every mutator
> > +	 * clears valid and waits a full grace period before changing
> > +	 * anything.
> > +	 */
> > +	if (!smp_load_acquire(&gpc->valid))
> > +		return false;
> > +
> >  	if (!gpc->active)
> >  		return false;
> 
> [Severity: Medium]
> Should the read of gpc->generation use READ_ONCE?

+1, the {WRITE,READ}_ONCE() usage looks to be very inconsistent.

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 1709 bytes --]

On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > Replace the per-cache rwlock with RCU for the read side.
> 
> I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
> e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
> strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
> synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
> for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
> were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
> will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
> a non-starter.

Yeah, the refresh() path got pretty slow in my first attempt, before
optimising that *not* to have a grace period if the memslot generation
changed but the actual GPA→uHVA (and memslot) don't *change*.

> Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
> the wait time can easily get to 20+ milliseconds, which again is a non-starter for
> things like steal-time updates and nVMX pages.

But we don't have to synchronize from the read side. The code path
which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
the refresh() path which is already the fallback slow path which can
sleep. Although in my tree I've optimised that *not* to incur a grace
period when it's avoidable, as noted above.

I'm more concerned by the fact that srcu_gp_end() might have to
*allocate*, while the OOM reaper path waits for it. I'll see how we can
deal with that...

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

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Sean Christopherson]

On Thu, Aug 06, 2026, David Woodhouse wrote:
> On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> > On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > > Replace the per-cache rwlock with RCU for the read side.
> > 
> > I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
> > e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
> > strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
> > synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
> > for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
> > were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
> > will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
> > a non-starter.
> 
> Yeah, the refresh() path got pretty slow in my first attempt, before
> optimising that *not* to have a grace period if the memslot generation
> changed but the actual GPA→uHVA (and memslot) don't *change*.
> 
> > Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
> > the wait time can easily get to 20+ milliseconds, which again is a non-starter for
> > things like steal-time updates and nVMX pages.
> 
> But we don't have to synchronize from the read side. The code path
> which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
> the refresh() path which is already the fallback slow path which can sleep.

It's probably a slow path for all current users, but it definitely won't be a slow
path for nested virtualization, because a refresh() will be required any time the
GPA changes, i.e. any time the vCPU runs a different vmc{b,c}12.  That's why I
think we should treat GPCs that are strictly bound to a vCPU differently.

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 806 bytes --]

On Thu, 2026-08-06 at 11:11 -0700, Sean Christopherson wrote:
> 
> It's probably a slow path for all current users, but it definitely won't be a slow
> path for nested virtualization, because a refresh() will be required any time the
> GPA changes, i.e. any time the vCPU runs a different vmc{b,c}12.  

Those need caching anyway, FWIW. We need a pool of nested contexts
(including these GPCs), rather than throwing them away each some the L1
switches to a different L2.

> That's why I think we should treat GPCs that are strictly bound to a
> vCPU differently.

Yes. I wonder if it's reasonable to tie it to the GUEST_USES_PFN case;
they aren't *perfectly* correlated (maybe the steal one could be
considered to be in your category too)... but perhaps those are the
ones that *care*? 

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

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[David Woodhouse]

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

On Thu, 2026-08-06 at 19:59 +0200, Woodhouse, David wrote:
> On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> > On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > > Replace the per-cache rwlock with RCU for the read side.
> > 
> > I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
> > e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
> > strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
> > synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
> > for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
> > were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
> > will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
> > a non-starter.
> 
> Yeah, the refresh() path got pretty slow in my first attempt, before
> optimising that *not* to have a grace period if the memslot generation
> changed but the actual GPA→uHVA (and memslot) don't *change*.
> 
> > Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
> > the wait time can easily get to 20+ milliseconds, which again is a non-starter for
> > things like steal-time updates and nVMX pages.
> 
> But we don't have to synchronize from the read side. The code path
> which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
> the refresh() path which is already the fallback slow path which can
> sleep. Although in my tree I've optimised that *not* to incur a grace
> period when it's avoidable, as noted above.
> 
> I'm more concerned by the fact that srcu_gp_end() might have to
> *allocate*, while the OOM reaper path waits for it. I'll see how we can
> deal with that...

I think I need to invoke Paul et al for that one...

Paul, Boqun, please could I trouble you to look at the top commit in
https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu-srcu

The "gfn_to_pfn_cache" (GPC) here is for all extents and purposes a
TLB. It's caching the full two-stage translation through KVM memslots
which map a guest physical address to a userspace virtual address, and
then through the standard process page tables to a host physical
address.

It is the second stage we are interested in here. Invalidation happens
through an MMU notifier, which (after I obey Sean's request to switch
to SRCU) calls synchronize_srcu_expedited().

My problem:

 1. The MMU notifier invalidation can be called from the OOM reaper
    path. In this case it MUST NOT sleep to allocate memory, or block
    on anything which does so. cf.
    https://lore.kernel.org/all/787aa26cf62dfd361eea8ed19f384fc517892501.camel@infradead.org/

 2. srcu_gp_end() does so, through init_srcu_struct_nodes(), while
    synchronize_srcu_expedited() effectively waits for it — precisely
    the thing we must not do. (Strictly, the waiter's completion is
    queued to the same workqueue on which process_srcu() could be
    blocked in init_srcu_struct_nodes(…, GFP_KERNEL)).

On a system with >= 128 CPUs it's actually OK because that allocation
path never happens; everything's allocated in advance ("rcu: srcu_init:
Setting srcu_struct sizes to big"). But on smaller systems I don't see
that we have a way to ensure that things are safe. (I don't count
setting big_cpu_lim to zero!)

Could we change that init_srcu_struct_nodes(…, GFP_KERNEL) to a non-
sleeping allocation? If the allocation fails, that isn't fatal, is it?
Or some other way to trigger the transition to big under controlled
circumstances in advance...? 

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

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Paul E. McKenney]

On Thu, Aug 06, 2026 at 10:38:56PM +0200, David Woodhouse wrote:
> On Thu, 2026-08-06 at 19:59 +0200, Woodhouse, David wrote:
> > On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> > > On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > > > Replace the per-cache rwlock with RCU for the read side.
> > > 
> > > I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
> > > e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
> > > strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
> > > synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
> > > for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
> > > were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
> > > will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
> > > a non-starter.
> > 
> > Yeah, the refresh() path got pretty slow in my first attempt, before
> > optimising that *not* to have a grace period if the memslot generation
> > changed but the actual GPA→uHVA (and memslot) don't *change*.
> > 
> > > Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
> > > the wait time can easily get to 20+ milliseconds, which again is a non-starter for
> > > things like steal-time updates and nVMX pages.
> > 
> > But we don't have to synchronize from the read side. The code path
> > which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
> > the refresh() path which is already the fallback slow path which can
> > sleep. Although in my tree I've optimised that *not* to incur a grace
> > period when it's avoidable, as noted above.
> > 
> > I'm more concerned by the fact that srcu_gp_end() might have to
> > *allocate*, while the OOM reaper path waits for it. I'll see how we can
> > deal with that...
> 
> I think I need to invoke Paul et al for that one...
> 
> Paul, Boqun, please could I trouble you to look at the top commit in
> https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu-srcu
> 
> The "gfn_to_pfn_cache" (GPC) here is for all extents and purposes a
> TLB. It's caching the full two-stage translation through KVM memslots
> which map a guest physical address to a userspace virtual address, and
> then through the standard process page tables to a host physical
> address.
> 
> It is the second stage we are interested in here. Invalidation happens
> through an MMU notifier, which (after I obey Sean's request to switch
> to SRCU) calls synchronize_srcu_expedited().
> 
> My problem:
> 
>  1. The MMU notifier invalidation can be called from the OOM reaper
>     path. In this case it MUST NOT sleep to allocate memory, or block
>     on anything which does so. cf.
>     https://lore.kernel.org/all/787aa26cf62dfd361eea8ed19f384fc517892501.camel@infradead.org/
> 
>  2. srcu_gp_end() does so, through init_srcu_struct_nodes(), while
>     synchronize_srcu_expedited() effectively waits for it — precisely
>     the thing we must not do. (Strictly, the waiter's completion is
>     queued to the same workqueue on which process_srcu() could be
>     blocked in init_srcu_struct_nodes(…, GFP_KERNEL)).
> 
> On a system with >= 128 CPUs it's actually OK because that allocation
> path never happens; everything's allocated in advance ("rcu: srcu_init:
> Setting srcu_struct sizes to big"). But on smaller systems I don't see
> that we have a way to ensure that things are safe. (I don't count
> setting big_cpu_lim to zero!)
> 
> Could we change that init_srcu_struct_nodes(…, GFP_KERNEL) to a non-
> sleeping allocation? If the allocation fails, that isn't fatal, is it?
> Or some other way to trigger the transition to big under controlled
> circumstances in advance...? 

We could easily provide such an interface.  However, is there some
check that would allow us to determine when it is OK to use GFP_KERNEL?
If there was, we could replace that GFP_KERNEL with something like this:

	sleeping_alloc_ok() ? GFP_KERNEL : GPF_HEY_YOU_TELL_ME

My best guess for GPF_HEY_YOU_TELL_ME is GFP_NOWAIT.

That way, you don't need another SRCU API to transition to big, and
everything "just works".

What say you?

							Thanx, Paul

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[David Woodhouse]

On 6 August 2026 23:52:06 CEST, "Paul E. McKenney" <paulmck@kernel.org> wrote:
>On Thu, Aug 06, 2026 at 10:38:56PM +0200, David Woodhouse wrote:
>> On Thu, 2026-08-06 at 19:59 +0200, Woodhouse, David wrote:
>> > On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
>> > > On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
>> > > > Replace the per-cache rwlock with RCU for the read side.
>> > > 
>> > > I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
>> > > e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
>> > > strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
>> > > synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
>> > > for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
>> > > were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
>> > > will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
>> > > a non-starter.
>> > 
>> > Yeah, the refresh() path got pretty slow in my first attempt, before
>> > optimising that *not* to have a grace period if the memslot generation
>> > changed but the actual GPA→uHVA (and memslot) don't *change*.
>> > 
>> > > Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
>> > > the wait time can easily get to 20+ milliseconds, which again is a non-starter for
>> > > things like steal-time updates and nVMX pages.
>> > 
>> > But we don't have to synchronize from the read side. The code path
>> > which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
>> > the refresh() path which is already the fallback slow path which can
>> > sleep. Although in my tree I've optimised that *not* to incur a grace
>> > period when it's avoidable, as noted above.
>> > 
>> > I'm more concerned by the fact that srcu_gp_end() might have to
>> > *allocate*, while the OOM reaper path waits for it. I'll see how we can
>> > deal with that...
>> 
>> I think I need to invoke Paul et al for that one...
>> 
>> Paul, Boqun, please could I trouble you to look at the top commit in
>> https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu-srcu
>> 
>> The "gfn_to_pfn_cache" (GPC) here is for all extents and purposes a
>> TLB. It's caching the full two-stage translation through KVM memslots
>> which map a guest physical address to a userspace virtual address, and
>> then through the standard process page tables to a host physical
>> address.
>> 
>> It is the second stage we are interested in here. Invalidation happens
>> through an MMU notifier, which (after I obey Sean's request to switch
>> to SRCU) calls synchronize_srcu_expedited().
>> 
>> My problem:
>> 
>>  1. The MMU notifier invalidation can be called from the OOM reaper
>>     path. In this case it MUST NOT sleep to allocate memory, or block
>>     on anything which does so. cf.
>>     https://lore.kernel.org/all/787aa26cf62dfd361eea8ed19f384fc517892501.camel@infradead.org/
>> 
>>  2. srcu_gp_end() does so, through init_srcu_struct_nodes(), while
>>     synchronize_srcu_expedited() effectively waits for it — precisely
>>     the thing we must not do. (Strictly, the waiter's completion is
>>     queued to the same workqueue on which process_srcu() could be
>>     blocked in init_srcu_struct_nodes(…, GFP_KERNEL)).
>> 
>> On a system with >= 128 CPUs it's actually OK because that allocation
>> path never happens; everything's allocated in advance ("rcu: srcu_init:
>> Setting srcu_struct sizes to big"). But on smaller systems I don't see
>> that we have a way to ensure that things are safe. (I don't count
>> setting big_cpu_lim to zero!)
>> 
>> Could we change that init_srcu_struct_nodes(…, GFP_KERNEL) to a non-
>> sleeping allocation? If the allocation fails, that isn't fatal, is it?
>> Or some other way to trigger the transition to big under controlled
>> circumstances in advance...? 
>
>We could easily provide such an interface.  However, is there some
>check that would allow us to determine when it is OK to use GFP_KERNEL?
>If there was, we could replace that GFP_KERNEL with something like this:
>
>	sleeping_alloc_ok() ? GFP_KERNEL : GPF_HEY_YOU_TELL_ME
>
>My best guess for GPF_HEY_YOU_TELL_ME is GFP_NOWAIT.
>
>That way, you don't need another SRCU API to transition to big, and
>everything "just works".
>
>What say you?

Thanks for the prompt answer!

It's a nice idea, but I'm not sure what we'd trigger on. In an earlier patch in this same series I already ripped *out* the non_block_start() for this OOM reaper path, because that would make it sad even about the non-allocating sleep for the (S)RCU wait. The actual criterion is more subtle than that.

And anyway, the allocation isn't even running from that thread; it's on the workqueue.

I think it has to be an API which lets us upgrade in advance. Or just make it GFP_NOWAIT unconditionally? What's the failure mode if the allocation fails? My unreliable AI friend told me it was harmless enough... but I don't trust it much, which is why I reached out to my meat friend...

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 3494 bytes --]

On Thu, 2026-08-06 at 11:11 -0700, Sean Christopherson wrote:
> On Thu, Aug 06, 2026, David Woodhouse wrote:
> > On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> > > On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > > > Replace the per-cache rwlock with RCU for the read side.
> > > 
> > > I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
> > > e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
> > > strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
> > > synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
> > > for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
> > > were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
> > > will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
> > > a non-starter.
> > 
> > Yeah, the refresh() path got pretty slow in my first attempt, before
> > optimising that *not* to have a grace period if the memslot generation
> > changed but the actual GPA→uHVA (and memslot) don't *change*.
> > 
> > > Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
> > > the wait time can easily get to 20+ milliseconds, which again is a non-starter for
> > > things like steal-time updates and nVMX pages.
> > 
> > But we don't have to synchronize from the read side. The code path
> > which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
> > the refresh() path which is already the fallback slow path which can sleep.
> 
> It's probably a slow path for all current users, but it definitely won't be a slow
> path for nested virtualization, because a refresh() will be required any time the
> GPA changes, i.e. any time the vCPU runs a different vmc{b,c}12.  That's why I
> think we should treat GPCs that are strictly bound to a vCPU differently.

"treat GPCs that are strictly bound to a vCPU differently"....

Of course, at this point we have to make the observation that with a
naïve and simple spinlock, we don't *need* any additional complexity to
treat single-CPU users differently; they just *naturally* go faster
because they always own that cache line anyway, while the rarer case of
concurrent access is naturally correct too.

The thing that's held me back is the shared_info and vcpu_info cases
which are used for incoming interrupts; I'd really need to do some
serious scalability testing to compare with the rwlock we have at the
moment.

But honestly, the Xen 2-level event channel mechanism is *already* a
cache contention minefield. Even if the GPC is lockless, two concurrent
host interrupts attempting to deliver event channels to the guest are
*also* going to clash on atomic accesses to the shinfo->evtchn_pending
array, as nobody makes any attempt to ensure that event channels for
different vCPUs are placed on different cache lines in that bitmap.

We might even find that concurrent event channel delivery is *better*
when it's serialised by a spinlock, than using rwlocks. Either way, I
think we can *certainly* live with just flipping it to be a (raw)
spinlock in the RT case.

I'll play. In the meantime, the rest of the fixes are in my branch¹
before the RCU stuff, so I'll post those probably on Monday.

¹ https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu-srcu

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

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

[David Woodhouse]

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

On Fri, 2026-08-07 at 10:57 +0200, Woodhouse, David wrote:
> 
> We might even find that concurrent event channel delivery is *better*
> when it's serialised by a spinlock, than using rwlocks. Either way, I
> think we can *certainly* live with just flipping it to be a (raw)
> spinlock in the RT case.
> 
> I'll play.

Hm, initial results are that the spinlock version sucks for parallel
delivery, but rwlock isn't that great either. RCU scales nicely. I
built a test with multiple threads writing eventfds to delivery event
channel interrupts in parallel.

Slightly artificial as it doesn't really do any invalidations (although
that *is* the steady state for a running guest). I still need to do
some proper performance testing of a Xen guest with both emulated and
PCI passthrough network.

My friend says...
=================================

Deliveries/sec, spread layout (clean kernel, only kvm.ko swapped)

┌───────┬────────┬────────┬──────────────┐
│ vCPUs │ rwlock │  RCU   │ raw spinlock │
├───────┼────────┼────────┼──────────────┤
│ 1     │ 3.45M  │ 4.06M  │ 3.85M        │
├───────┼────────┼────────┼──────────────┤
│ 2     │ 5.72M  │ 7.37M  │ 5.70M        │
├───────┼────────┼────────┼──────────────┤
│ 4     │ 12.57M │ 16.60M │ 9.79M        │
├───────┼────────┼────────┼──────────────┤
│ 8     │ 16.25M │ 31.33M │ 6.70M        │
├───────┼────────┼────────┼──────────────┤
│ 16    │ 10.65M │ 50.48M │ 5.13M        │
└───────┴────────┴────────┴──────────────┘

RCU wins enormously — 4.7× the rwlock at 16 vCPUs, and it's the only
arm that keeps scaling (10.65M → 50.48M while rwlock regresses past 8
vCPUs). Mean latency stays flat at ~313ns where rwlock climbs to
1499ns.

The raw spinlock is a disaster under contention — 5.13M/s at 16 vCPUs,
half the rwlock, with 3108ns mean latency. It actively degrades from 4
vCPUs on.

So my queued_read_lock() reasoning was wrong. Yes it does an
atomic_add_return_acquire(), but readers still proceed concurrently
after that — they don't serialise the critical section the way a
spinlock does. The GPC read sections here (a test_and_set_bit() plus a
couple of writes) are long enough that mutual exclusion costs real
throughput. The line-bouncing on the lock word is not the dominant
term; serialisation of the critical section is.

That means the argument in your draft to Sean — that a naïve spinlock
is fine because single-vCPU users own the line anyway — holds for the
uncontended case (3.85M vs 3.45M at 1 vCPU, spinlock slightly ahead)
but fails badly for shinfo_cache, which is per-VM and genuinely
contended. Exactly the case you flagged as holding you back. The data
says your instinct to worry was right.

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