Re: [PATCH v2 1/7] KVM: arm64: Move OUTSIDE_GUEST_MODE publication past context being saved

On Tue, 29 Sep 2026 13:59:23 +0100,
Fuad Tabba [off-list ref] wrote:

Hi Marc,

On Tue, 29 Sep 2026 10:35:42 +0100, Marc Zyngier [off-list ref] wrote:
[...]

quoted

Move the publication of OUTSIDE_GUEST_MODE to the point where the state
is actually written, and give this write release semantics to ensure the
correct ordering.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org

No Fixes: tag?

No. It's always been fsck'd.

[...]

quoted

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c

[...]

quoted

@@ -1386,6 +1385,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)

              kvm_arch_vcpu_ctxsync_fp(vcpu);

+             /*
+              * All the state has been synchronised, let advertise
+              * we're outside of the guest.
+              */
+             smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);

Pardon my atomics :)

This is not an atomic instruction. However, it composes with atomics.

, but what does the release pair with? On the halt
path, the only reader I can find is the cmpxchg() in
kvm_vcpu_exiting_guest_mode()

From Documentation/atomic_t.txt:

<quote>
 - RMW operations that have a return value are fully ordered;

 - RMW operations that are conditional are unordered on FAILURE,
   otherwise the above rules apply.
</quote>

The acquire side of cmpxchg() is therefore interacting with the above
release, which gives us the required ordering.

However, there is a problem if cmpxchg() fails, as there is no
ordering in that case, and I'm not sure the smp_mb__before_atomic()
saves the bacon in that case. It feels we'd need an acquire
somewhere, a bit like this:

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 03bfc92864b6e..2efb4febcb235 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h

@@ -563,9 +563,15 @@ static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
 	 * The memory barrier ensures a previous write to vcpu->requests cannot
 	 * be reordered with the read of vcpu->mode.  It pairs with the general
 	 * memory barrier following the write of vcpu->mode in VCPU RUN.
+	 *
+	 * cmpxchg() is not ordered when failing, so make sure we perform an
+	 * acquire in that case.
 	 */
 	smp_mb__before_atomic();
-	return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
+	if (cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE) != IN_GUEST_MODE)
+		return smp_load_acquire(&vcpu->mode);
+
+	return IN_GUEST_MODE;
 }
 
 /*

, and the LPI-disable and MOVALL halts
then take ap_list_lock or irq_lock. Would WRITE_ONCE() be enough?

We need a release so that we know for sure that any state stored
before is visible by the time we can observe OUTSIDE_GUEST_MODE, and
WRITE_ONCE() doesn't provide that (it can be reordered).

I don't see what taking a lock changes to the ordering requirement.

Should the early exit path (the kvm_vcpu_exit_request() bail-out) get
the same treatment? I think that's what Sashiko is trying to say in
the patch 5 review [1].

I don't understand what sashiko is trying to say, but this is clearly
missing from the patch, see below. Not sure how I missed that one.

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 9a4871cd796bc..1a3a15bc6f55c 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c

@@ -1333,13 +1333,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
 
 		if (ret <= 0 || kvm_vcpu_exit_request(vcpu, &ret)) {
-			vcpu->mode = OUTSIDE_GUEST_MODE;
 			isb(); /* Ensure work in x_flush_hwstate is committed */
 			if (kvm_vcpu_has_pmu(vcpu))
 				kvm_pmu_sync_hwstate(vcpu);
 			if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
 				kvm_timer_sync_user(vcpu);
 			kvm_vgic_sync_hwstate(vcpu);
+			smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);
 			local_irq_enable();
 			preempt_enable();
 			continue;

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.