[RFC PATCH 55/55] KVM: planes: Annotate the lockless twin-publication protocol

[David Woodhouse]

From: David Woodhouse <dwmw@xxxxxxxxxxxx>

Plane twins are published with a plain store to common->vcpus[] plus an
smp_wmb() that pairs with online_vcpus, not with twin initialisation,
while the lockless readers grown across the series (the twin-kick
paths, the Hyper-V TLB-flush and stimer twin lookups, the SEND_IPI
target lookup, vcpu_for_each_plane) read the slots with a mixture of
READ_ONCE and bare loads.  x86-TSO hides the lot, but the protocol is
unannotated generic code: KCSAN reports it, and nothing stops a
reordered publication from exposing a half-initialised twin to a kick.

Publish the slot with smp_store_release(), make every lockless reader
use READ_ONCE (vcpu_for_each_plane included), and document the pairing
at the iterator.  Mutex-serialised lookups (the in-kernel VTL switch
runs under the shared vCPU mutex that twin creation also takes) stay
plain.  Also document KVM_REQUEST_PER_PLANE's two scopes at its
definition: the fan-out to twins happens in kvm_make_vcpu_request(),
while a direct kvm_make_request() marks only the twin it was given - a
distinction a future direct-raise caller would otherwise trip over.

No functional change on x86.

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
Assisted-by: LLM
---
 arch/x86/kvm/hyperv.c    |  6 +++---
 include/linux/kvm_host.h | 13 ++++++++++++-
 virt/kvm/kvm_main.c      |  8 +++++++-
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index dbe1b83c1f28..8abc61f46608 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -612,7 +612,7 @@ static void stimer_mark_pending(struct kvm_vcpu_hv_stimer *stimer,
 		int p;
 
 		for (p = 0; p < vcpu->plane_level; p++) {
-			twin = vcpu->common->vcpus[p];
+			twin = READ_ONCE(vcpu->common->vcpus[p]);
 			if (twin) {
 				kvm_make_request(KVM_REQ_UNBLOCK, twin);
 				kvm_vcpu_kick(twin);
@@ -2370,7 +2370,7 @@ static void hv_tlb_flush_enqueue_all_planes(struct kvm_vcpu *v,
 	int p;
 
 	for (p = 0; p < KVM_MAX_PLANES; p++) {
-		twin = v->common->vcpus[p];
+		twin = READ_ONCE(v->common->vcpus[p]);
 		if (!twin || !to_hv_vcpu(twin))
 			continue;
 		fifo = kvm_hv_get_tlb_flush_fifo(twin, false);
@@ -2601,7 +2601,7 @@ static void kvm_hv_send_ipi_to_many(struct kvm *kvm, u32 vector,
 		 * VTL): deliver into that plane's APIC.
 		 */
 		if (target_plane) {
-			target = vcpu->common->vcpus[target_plane];
+			target = READ_ONCE(vcpu->common->vcpus[target_plane]);
 			if (!target)
 				continue;
 		}
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6a1ac96d64ba..4d98de4c7523 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -159,6 +159,12 @@ static inline bool kvm_is_error_gpa(gpa_t gpa)
 #define KVM_REQUEST_NO_WAKEUP      BIT(8)
 #define KVM_REQUEST_WAIT           BIT(9)
 #define KVM_REQUEST_NO_ACTION      BIT(10)
+/*
+ * A KVM_REQUEST_PER_PLANE request has two scopes: raised through
+ * kvm_make_vcpu_request() (kicks, mass requests) it fans out to every
+ * plane twin of the vCPU, while a direct kvm_make_request() marks only
+ * the twin it was given.  A direct caller must pick the twin it means.
+ */
 #define KVM_REQUEST_PER_PLANE      BIT(11)
 /*
  * Architecture-independent vcpu->requests bit members
@@ -400,9 +406,14 @@ struct kvm_vcpu_common {
 	struct kvm_vcpu_arch_common arch;
 };
 
+/*
+ * The slot read pairs with the smp_store_release() publication in
+ * kvm_vcpu_finish_common() for iterators running outside the locks that
+ * serialise twin creation.
+ */
 #define vcpu_for_each_plane(common, i, v)			\
 	for ((i) = 0; (i) < KVM_MAX_PLANES; ++(i))		\
-		if (((v) = common->vcpus[(i)]) != NULL)
+		if (((v) = READ_ONCE(common->vcpus[(i)])) != NULL)
 
 /* Tracked per plane-VCPU - used for deciding which plane-vcpu to run */
 enum kvm_vcpu_state {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f0b474b0547d..6031ba9a6667 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -563,7 +563,13 @@ static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned
 static void kvm_vcpu_finish_common(struct kvm_vcpu *vcpu)
 {
 	WARN_ON(vcpu->common->vcpus[vcpu->plane_level] != NULL);
-	vcpu->common->vcpus[vcpu->plane_level] = vcpu;
+	/*
+	 * Publish with release semantics so the lockless twin readers
+	 * (the kick paths, vcpu_for_each_plane, the hyperv twin lookups)
+	 * observe a fully initialised vCPU through their READ_ONCE of the
+	 * slot; the plain store relied on x86-TSO.
+	 */
+	smp_store_release(&vcpu->common->vcpus[vcpu->plane_level], vcpu);
 	smp_wmb();
 	if (vcpu->plane_level == 0) {
 		/*
-- 
2.50.1