KVM: arm64: vgic-v3: Make LPI disabling robust (and more)

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

Cc [email protected]

arch/arm64/kvm/arm.c +0 -0

Hi Marc,

On Tue, 29 Sep 2026 10:35:42 +0100, Marc Zyngier <[email protected]> wrote:
[...]

Quoted 4 lines

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.

Quoted 2 lines

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

No Fixes: tag?

[...]

Quoted 1 line

arch/arm64/kvm/arm.c +0 -0

[...]

Quoted 4 lines

arch/arm64/kvm/arm.c +0 -0

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

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 1392

Quoted 5 lines

arch/arm64/kvm/arm.c +5 -0

+             /*
+              * 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 :)

, 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()

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

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

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:

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.

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.

Quoted 7 lines · Fuad Tabba

Quoted 1 line · Marc Zyngier

arch/arm64/kvm/arm.c +0 -0

[...]

Quoted 4 lines

arch/arm64/kvm/arm.c +0 -0

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

Quoted 7 lines

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 1392

Quoted 5 lines

arch/arm64/kvm/arm.c +5 -0

+             /*
+              * 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.

Quoted 3 lines

, 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:

include/linux/kvm_host.h +7 -1

index 03bfc92864b6e..2efb4febcb235 100644
@@ -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;
 }
 
 /*

Quoted 3 lines

, 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.

Quoted 4 lines

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.

arch/arm64/kvm/arm.c +0 -0

index 9a4871cd796bc..1a3a15bc6f55c 100644
@@ -1333,13 +1333,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		smp_store_mb(vcpu->mode, IN_GUEST_MODE);

We have situations where we can call kvm_arm_halt_guest() without holding config_lock, which means that halt and resume can overlap in funny ways when called from separate contexts, and result in situations where a vcpu is resumed while other parts of KVM assume it is halted. Consequences are left to the imagination of the reader.

Fix this sorry situation by turning kvm_vcpu_arch::pause into an atomic counter, which makes the races described above harmless.

Fixes 3b92830ad41b2 ("KVM: arm/arm64: implement kvm_arm_[halt,resume]_guest")

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/include/asm/kvm_host.h |  7 +++----
 arch/arm64/kvm/arm.c              | 21 ++++++++++++---------
 2 files changed, 15 insertions(+), 13 deletions(-)

arch/arm64/include/asm/kvm_host.h +3 -4

index 27fe0cd5b2d7a..66ea2372f9870 100644
@@ -889,11 +889,10 @@ struct kvm_vcpu_arch {
        /*
         * Don't run the guest (internal implementation need).
         *
-        * Contrary to the flags above, this is set/cleared outside of
-        * a vcpu context, and thus cannot be mixed with the flags
-        * themselves (or the flag accesses need to be made atomic).
+        * Contrary to the flags above, this is updated outside of
+        * a vcpu context, and thus cannot be mixed with the flags.
         */
-       bool pause;
+       atomic_t pause;
 
        /*
         * We maintain more than a single set of debug registers to support

arch/arm64/kvm/arm.c +12 -10

index 5f5dd8bead4b9..9a4871cd796bc 100644
@@ -561,6 +561,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
        kvm_arm_pvtime_vcpu_init(&vcpu->arch);
 
        vcpu->arch.hw_mmu = &vcpu->kvm->arch.mmu;
+       atomic_set(&vcpu->arch.pause, 0);
 
        /*
         * This vCPU may have been created after mpidr_data was initialized.

@@ -835,6 +836,11 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
        return ret;
 }
 
+static bool vcpu_can_run(struct kvm_vcpu *vcpu)
+{
+       return !kvm_arm_vcpu_stopped(vcpu) && !atomic_read(&vcpu->arch.pause);
+}
+
 /**
  * kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled
  * @v:         The VCPU pointer

@@ -850,8 +856,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
                      (kvm_timer_should_notify_user(v) ||
                       kvm_pmu_should_notify_user(v)));
 
-       return ((irq_lines || kvm_vgic_vcpu_pending_irq(v))
-               && !kvm_arm_vcpu_stopped(v) && !v->arch.pause);
+       return ((irq_lines || kvm_vgic_vcpu_pending_irq(v)) && vcpu_can_run(v));
 }
 
 bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)

@@ -1014,7 +1019,7 @@ void kvm_arm_halt_guest(struct kvm *kvm)
        struct kvm_vcpu *vcpu;
 
        kvm_for_each_vcpu(i, vcpu, kvm)
-               vcpu->arch.pause = true;
+               atomic_inc(&vcpu->arch.pause);
        kvm_make_all_cpus_request(kvm, KVM_REQ_SLEEP);
 }
 

@@ -1024,8 +1029,8 @@ void kvm_arm_resume_guest(struct kvm *kvm)
        struct kvm_vcpu *vcpu;
 
        kvm_for_each_vcpu(i, vcpu, kvm) {
-               vcpu->arch.pause = false;
-               __kvm_vcpu_wake_up(vcpu);
+               if (atomic_dec_and_test(&vcpu->arch.pause))
+                       __kvm_vcpu_wake_up(vcpu);
        }
 }
 

@@ -1033,11 +1038,9 @@ static void kvm_vcpu_sleep(struct kvm_vcpu *vcpu)
 {
        struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
 
-       rcuwait_wait_event(wait,
-                          (!kvm_arm_vcpu_stopped(vcpu)) && (!vcpu->arch.pause),
-                          TASK_INTERRUPTIBLE);
+       rcuwait_wait_event(wait, vcpu_can_run(vcpu), TASK_INTERRUPTIBLE);
 
-       if (kvm_arm_vcpu_stopped(vcpu) || vcpu->arch.pause) {
+       if (!vcpu_can_run(vcpu)) {
                /* Awaken to handle a signal, request we sleep again later. */
                kvm_make_request(KVM_REQ_SLEEP, vcpu);
        }
--

2.47.3

The MOVALL command moves the pending state of all LPIs targeting a given redistributor to another one. However, we seem to have lost the filtering on the source RD, which means we move all LPIs to the target.

Not quite what the spec mandates.

Hack update_affinity() to take an optional source vcpu that is used as a filter when non-NULL, restoring the filtering that was performed by vgic_copy_lpi_list() back in the days.

Fixes 11f4f8f3e6e06 ("KVM: arm64: vgic-its: Walk LPI xarray in vgic_its_cmd_handle_movall()")

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/kvm/vgic/vgic-its.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

arch/arm64/kvm/vgic/vgic-its.c +8 -5

index 9e782a4fea7e5..cc02d9b62bd95 100644
@@ -319,12 +319,16 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
        return ret;
 }
 
-static int update_affinity(struct vgic_irq *irq, struct kvm_vcpu *vcpu)
+static int update_affinity(struct vgic_irq *irq,
+                          struct kvm_vcpu *from_vcpu, struct kvm_vcpu *vcpu)
 {
        struct its_vlpi_map map;
        int ret;
 
        guard(raw_spinlock_irqsave)(&irq->irq_lock);
+       if (from_vcpu && irq->target_vcpu != from_vcpu)
+               return 0;
+
        irq->target_vcpu = vcpu;
 
        if (!irq->hw)

@@ -362,7 +366,7 @@ static void update_affinity_ite(struct kvm *kvm, struct its_ite *ite)
                return;
 
        vcpu = collection_to_vcpu(kvm, ite->collection);
-       update_affinity(ite->irq, vcpu);
+       update_affinity(ite->irq, NULL, vcpu);
 }
 
 /*

@@ -856,7 +860,7 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 
        vgic_its_invalidate_cache(its);
 
-       return update_affinity(ite->irq, vcpu);
+       return update_affinity(ite->irq, NULL, vcpu);
 }
 
 static bool __is_visible_gfn_locked(struct vgic_its *its, gpa_t gpa)

@@ -1383,7 +1387,7 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
                if (!irq)
                        continue;
 
-               update_affinity(irq, vcpu2);
+               update_affinity(irq, vcpu1, vcpu2);
 
                vgic_put_irq(kvm, irq);
        }
--

2.47.3

MOVALL is a royal pain, as it requires us to iterate over all the LPIs and move them around if they are on the correct source vcpu, and requiring locking for each of them.

This locking can result in contention for vcpus that enter and exit the guest, and hinders forward progress. In order to make sure such process is not contended, just stop the VM altogether.

Nobody in their right mind uses MOVALL anyway, as this is a rather braindead aspect of the GICv3 architecture.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/vgic/vgic-its.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

arch/arm64/kvm/vgic/vgic-its.c +11 -1

index cc02d9b62bd95..094eede267e54 100644
@@ -1382,6 +1382,15 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
        if (vcpu1 == vcpu2)
                return 0;
 
+       /*
+        * Bulk operations such as MOVALL are a pain, as they can clash
+        * badly locking-wise with other vcpus entering and exiting the
+        * guest, should they be affected by it. Stopping the guest is a
+        * safer bet to ensure uncontended access and ultimately forward
+        * progress. Yeah...
+        */
+       kvm_arm_halt_guest(kvm);
+
        xa_for_each(&dist->lpi_xa, intid, irq) {
                irq = vgic_get_irq(kvm, intid);
                if (!irq)

@@ -1394,6 +1403,8 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
 
        vgic_its_invalidate_cache(its);
 
+       kvm_arm_resume_guest(kvm);
+
        return 0;
 }
 
--

2.47.3

Map LPI A to vCPU0's collection and LPI B to vCPU1's, remap vCPU0's collection to vCPU2 and MOVALL vCPU0's LPIs there, then inject both. A must be taken on vCPU2 and B on vCPU1. KVM's MAPC handling already moves A to vCPU2, so only MOVALL handling can move B: without "KVM: arm64:
vgic-its: Fix MOVALL handling of source redistributor", it moves B to vCPU2 as well.

Assisted-by LLM

Signed-off-by Fuad Tabba <[email protected]>

---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/arm64/vgic_its_movall.c     | 341 ++++++++++++++++++
 2 files changed, 342 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/arm64/vgic_its_movall.c

tools/testing/selftests/kvm/Makefile.kvm +1 -0

index 908bdc7cf4f58..fb0bc99242975 100644
@@ -190,6 +190,7 @@ TEST_GEN_PROGS_arm64 += arm64/stage2_block_transitions
 TEST_GEN_PROGS_arm64 += arm64/vcpu_width_config
 TEST_GEN_PROGS_arm64 += arm64/vgic_init
 TEST_GEN_PROGS_arm64 += arm64/vgic_irq
+TEST_GEN_PROGS_arm64 += arm64/vgic_its_movall
 TEST_GEN_PROGS_arm64 += arm64/vgic_its_save
 TEST_GEN_PROGS_arm64 += arm64/vgic_lpi_stress
 TEST_GEN_PROGS_arm64 += arm64/vgic_v5

tools/testing/selftests/kvm/arm64/vgic_its_movall.c new file +50 -0

new file mode 100644
index 0000000000000..41918576bdf63
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vgic_its_movall - MOVALL does not move the LPIs of other redistributors.
+ *
+ * Two LPIs, A and B, target two different redistributors. A's collection is
+ * remapped to a third redistributor with MAPC, followed by MOVALL from the
+ * first redistributor to the third. Both LPIs are then injected. A must be
+ * taken on the third redistributor, and B must stay on its own, which is not
+ * MOVALL's source.
+ *
+ * Copyright (c) 2026 Google LLC
+ * Author: Fuad Tabba <[email protected]>
+ */
+
+#include <linux/sizes.h>
+#include <pthread.h>
+#include <stdatomic.h>
+
+#include "kvm_util.h"
+#include "gic.h"
+#include "gic_v3.h"
+#include "gic_v3_its.h"
+#include "processor.h"
+#include "ucall.h"
+#include "vgic.h"
+
+#define TEST_MEMSLOT_INDEX	1
+
+#define GIC_LPI_OFFSET		8192
+#define NR_VCPUS		3
+#define NR_LPIS			2
+#define DEVICE_ID		0
+
+/* LPI A (event 0) starts on vCPU0 and moves to vCPU2; B (event 1) stays on vCPU1 */
+#define LPI_A_COLL		0
+#define LPI_B_COLL		1
+#define MOVALL_FROM		0
+#define MOVALL_TO		2
+#define LPI_B_VCPU		1
+
+#define LPI_PROP_DEFAULT_PRIO	0xa0
+
+static gpa_t gpa_base;
+
+static struct kvm_vm *vm;
+static struct kvm_vcpu *vcpus[NR_VCPUS];
+static int its_fd;
+
+static struct test_data {
+	bool		request_vcpus_stop;

Quoted 7 lines · Fuad Tabba

Map LPI A to vCPU0's collection and LPI B to vCPU1's, remap vCPU0's collection to vCPU2 and MOVALL vCPU0's LPIs there, then inject both. A must be taken on vCPU2 and B on vCPU1. KVM's MAPC handling already moves A to vCPU2, so only MOVALL handling can move B: without "KVM: arm64:
vgic-its: Fix MOVALL handling of source redistributor", it moves B to vCPU2 as well.

MOVALL is about moving the pending bits (see 5.3.13 in the IHI0069H.b spec). Doing MOVALL first and only then injecting the interrupts doesn't quite check the MOVALL requirement. That doesn't impact KVM itself (we don't use pending tables at all), but you probably don't want to hardcode implementation specific behaviours here.

There seem to be a lot of commonality with the existing vgic_lpi_stress test. I'd rather we make this test the container for most ITS-related tests, instead of coming up with new individual tests.

Thanks,

        N,

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

This is v2 of this series addressing shortcomings of LPIs being disabled on one CPU from another. It has now expanded into some more common areas.

Yuchao Zhang reported that disabling LPIs on one CPU from another could result in UAFs and other horrors.

[...]

Address the two issues in one go, by actively taking a refcount on all IRQs referenced by last_lr_irq, and making sure that disabling LPIs force all vcpus to be paused, making it safe.

Hi Marc,

On Tue, 29 Sep 2026 10:35:41 +0100, Marc Zyngier <[email protected]> wrote:
[...]

Quoted 3 lines

Address the two issues in one go, by actively taking a refcount on all IRQs referenced by last_lr_irq, and making sure that disabling LPIs force all vcpus to be paused, making it safe.

I'm triaging Sashiko's pre-existing bug database, and I ran into something that I think this series doesn't cover.

vgic_its_inject_cached_translation() doesn't check vgic_lpis_enabled() (the slow path, vgic_its_resolve_lpi(), does), and pausing the vCPUs doesn't stop an MSI coming in through irqfd or KVM_SIGNAL_MSI. So an MSI that hits the translation cache between vgic_flush_pending_lpis() and vgic_its_invalidate_all_caches() can still end up on the ap_list of a vCPU whose LPIs are being disabled, after the flush.

Could the cached path check vgic_lpis_enabled() on the target vCPU, or am I missing something?

We could have vgic_target_oracle() return NULL if LPIs are disabled at the redistributor, then the rest of the AP list machinery will "just work" for injections that slip between the cracks.