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

["Woodhouse, David" <dwmw@amazon.co.uk>]

[-- 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 --]