[PATCH v2 0/7] KVM: arm64: vgic-v3: Make LPI disabling robust (and more)

[PATCH v2 0/7] KVM: arm64: vgic-v3: Make LPI disabling robust (and more)

[Marc Zyngier]

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.

There are two reasons for this:

- the last_lr_irq pointer does not contribute to LPI refcount, and
  that LPI being removed results in a dangling pointer

- LPIs can be deleted from a remote vcpu by disabling them while that
  vcpu is actually running, and has LPIs in its LRs.

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.

Review of the initial version pointed out two more general issues:

- OUTSIDE_GUEST_MODE is published too early, when the guest state is
  not yet visible to other threads, resulting in the wrong state being
  evaluated from another CPU.

- kvm_{halt,resume}_guest() can be called without holding a global
  lock, and therefore can nest. This can result in a vcpu being
  restarted too early.

This is addressed by the first two patches.

Finally, MOVALL suffers from similar issues as LPI disabling, but also
appears to be broken (the filtering on the source RD was accidentally
removed a while ago). Fix the filtering and move MOVALL to a "stop the
world" approach.

* From v1 [1]:

  - Fix OUTSIDE_GUEST_MODE publication to occur after the saving of
    the guest state

  - Turn vcpu->arch.pause into an atomic counter, allowing nesting

  - Fix MOVALL to filter by source RD

  - Make MOVALL a "stop the world" command

[1] https://lore.kernel.org/r/20260922214212.3327146-1-maz@kernel.org

Marc Zyngier (7):
  KVM: arm64: Move OUTSIDE_GUEST_MODE publication past context being
    saved
  KVM: arm64: Turn vcpu->arch.pause into a counter
  KVM: arm64: vgic: Allow last_lr_irq to be NULL when LRs are not
    overflowing
  KVM: arm64: vgic: Take a refcount on IRQs referenced by last_lr_irq
  KVM: arm64: vgic: Stop the VM when disabling LPIs
  KVM: arm64: vgic-its: Fix MOVALL handling of source redistributor
  KVM: arm64: vgic-its: Stop the VM when handling MOVALL

 arch/arm64/include/asm/kvm_host.h  |  7 +++----
 arch/arm64/kvm/arm.c               | 28 ++++++++++++++++++----------
 arch/arm64/kvm/vgic/vgic-its.c     | 23 +++++++++++++++++++----
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 10 ++++++++++
 arch/arm64/kvm/vgic/vgic-v2.c      |  6 ++++--
 arch/arm64/kvm/vgic/vgic-v3.c      |  6 ++++--
 arch/arm64/kvm/vgic/vgic.c         | 21 ++++++++++++++++-----
 7 files changed, 74 insertions(+), 27 deletions(-)

-- 
2.47.3

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

[Marc Zyngier]

OUTSIDE_GUEST_MODE indicates to the rest of KVM that the vcpu has
exited. So far, this is set as soon as we return from the notional
hypervisor (which with VHE is just a function call away).

However, code that uses OUTSIDE_GUEST_MODE as a synchronisation point
relies on the vcpu state to have been written back to the in-memory
structures. Clearly, this is not what is happening

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
---
 arch/arm64/kvm/arm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 8b080804bc90b..5f5dd8bead4b9 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1352,7 +1352,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 
 		ret = kvm_arm_vcpu_enter_exit(vcpu);
 
-		vcpu->mode = OUTSIDE_GUEST_MODE;
 		vcpu->stat.exits++;
 		/*
 		 * Back from guest
@@ -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);
+
 		/*
 		 * We must ensure that any pending interrupts are taken before
 		 * we exit guest timing so that timer ticks are accounted as
-- 
2.47.3

[PATCH v2 2/7] KVM: arm64: Turn vcpu->arch.pause into a counter

[Marc Zyngier]

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 <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/include/asm/kvm_host.h |  7 +++----
 arch/arm64/kvm/arm.c              | 21 ++++++++++++---------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 27fe0cd5b2d7a..66ea2372f9870 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -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
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 5f5dd8bead4b9..9a4871cd796bc 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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

[PATCH v2 3/7] KVM: arm64: vgic: Allow last_lr_irq to be NULL when LRs are not overflowing

[Marc Zyngier]

last_lr_irq is always populated when there is any interrupt populated in
the AP list. Not only this is not necessary (it is only useful when we
completely fill the LRs), but this is in the way of further fixes.

Make sure last_lr_irq is kept to NULL when we LRs are not completely
full.

Fixes: 6da5e537f5afe ("KVM: arm64: vgic: Pick EOIcount deactivations from AP-list tail")
Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba <fuad.tabba@linux.dev>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/vgic/vgic-v2.c | 6 ++++--
 arch/arm64/kvm/vgic/vgic-v3.c | 6 ++++--
 arch/arm64/kvm/vgic/vgic.c    | 8 +++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 7182f63fc9382..70cc53ba810ad 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -122,6 +122,10 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 	for (int lr = 0; lr < vgic_cpu->vgic_v2.used_lrs; lr++)
 		vgic_v2_fold_lr(vcpu, cpuif->vgic_lr[lr]);
 
+	cpuif->used_lrs = 0;
+	if (!irq)
+		return;
+
 	/* See the GICv3 equivalent for the EOIcount handling rationale */
 	list_for_each_entry_continue(irq, &vgic_cpu->ap_list_head, ap_list) {
 		u32 lr;
@@ -144,8 +148,6 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 		vgic_v2_fold_lr(vcpu, lr);
 		eoicount--;
 	}
-
-	cpuif->used_lrs = 0;
 }
 
 void vgic_v2_deactivate(struct kvm_vcpu *vcpu, u32 val)
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 726e20a1da6e7..346bacb3198f2 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -155,6 +155,10 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 	for (int lr = 0; lr < cpuif->used_lrs; lr++)
 		vgic_v3_fold_lr(vcpu, cpuif->vgic_lr[lr]);
 
+	cpuif->used_lrs = 0;
+	if (!irq)
+		return;
+
 	/*
 	 * EOIMode=0: use EOIcount to emulate deactivation. We are
 	 * guaranteed to deactivate in reverse order of the activation, so
@@ -188,8 +192,6 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 		vgic_v3_fold_lr(vcpu, lr);
 		eoicount--;
 	}
-
-	cpuif->used_lrs = 0;
 }
 
 void vgic_v3_deactivate(struct kvm_vcpu *vcpu, u64 val)
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index b25303d9919fd..425503e0c5825 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -866,9 +866,6 @@ static void vgic_fold_state(struct kvm_vcpu *vcpu)
 		return;
 	}
 
-	if (!*host_data_ptr(last_lr_irq))
-		return;
-
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_fold_lr_state(vcpu);
 	else
@@ -1021,11 +1018,12 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
 		scoped_guard(raw_spinlock,  &irq->irq_lock) {
 			if (likely(vgic_target_oracle(irq) == vcpu)) {
 				vgic_populate_lr(vcpu, irq, count++);
-				*host_data_ptr(last_lr_irq) = irq;
+				if (count == kvm_vgic_global_state.nr_lr)
+					*host_data_ptr(last_lr_irq) = irq;
 			}
 		}
 
-		if (count == kvm_vgic_global_state.nr_lr)
+		if (*host_data_ptr(last_lr_irq))
 			break;
 	}
 
-- 
2.47.3

[PATCH v2 4/7] KVM: arm64: vgic: Take a refcount on IRQs referenced by last_lr_irq

[Marc Zyngier]

Referencing the last interrupt inserted in an LR is rather fragile, as
this interrupt can vanish if a concurrently unmapped LPI.

Solve this by bumping up the refcount on the interrupt when populating
last_lr_irq, and drop it at vgic_prune_ap_list() time, when LPIs are
being reclaimed.

Fixes: 6da5e537f5afe ("KVM: arm64: vgic: Pick EOIcount deactivations from AP-list tail")
Reported-by: Yuchao Zhang <ndaugoing@gmail.com>
Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba <fuad.tabba@linux.dev>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/vgic/vgic.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 425503e0c5825..5cf5a1ef86cdd 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -853,6 +853,17 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		goto retry;
 	}
 
+	/*
+	 * Fix the last_lr_irq refcount which was obtained while
+	 * populating the LRs. This can also result in the LPI being
+	 * deleted.
+	 */
+	irq = *host_data_ptr(last_lr_irq);
+	if (irq) {
+		deleted_lpis |= vgic_put_irq_norelease(vcpu->kvm, irq);
+		*host_data_ptr(last_lr_irq) = NULL;
+	}
+
 	raw_spin_unlock(&vgic_cpu->ap_list_lock);
 
 	if (unlikely(deleted_lpis))
@@ -1018,8 +1029,10 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
 		scoped_guard(raw_spinlock,  &irq->irq_lock) {
 			if (likely(vgic_target_oracle(irq) == vcpu)) {
 				vgic_populate_lr(vcpu, irq, count++);
-				if (count == kvm_vgic_global_state.nr_lr)
+				if (count == kvm_vgic_global_state.nr_lr) {
+					vgic_get_irq_ref(irq);
 					*host_data_ptr(last_lr_irq) = irq;
+				}
 			}
 		}
 
-- 
2.47.3

[PATCH v2 5/7] KVM: arm64: vgic: Stop the VM when disabling LPIs

[Marc Zyngier]

Disabling LPIs is pretty nasty, as it directly messes with the AP list
of the affected CPU, which could be running... This has the potential to
lead to really bad behaviours, and we should not be doing that.

Take example on the way the Active state is handled and simply pause all
the vcpus so that we are sure they are all in a quiescent state, and the
state be safely manipulated.

Nobody has any expectation of performance for this anyway.

Fixes: 96085b949672d ("KVM: arm/arm64: vgic-v3: Retire pending interrupts on disabling LPIs")
Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba <fuad.tabba@linux.dev>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 5913a20d83019..adefc42964732 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -303,9 +303,19 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		if (ctlr != GICR_CTLR_ENABLE_LPIS)
 			return;
 
+		/*
+		 * Yes, disabling LPIs is painful, since it can be done from
+		 * a *remote* vcpu! So let's not take any chance, and make
+		 * sure that everybody has written their LRs back to the irq
+		 * structures, and release any reference they would have.
+		 *
+		 * If it hurts, don't do it.
+		 */
+		kvm_arm_halt_guest(vcpu->kvm);
 		vgic_flush_pending_lpis(vcpu);
 		vgic_its_invalidate_all_caches(vcpu->kvm);
 		atomic_set_release(&vgic_cpu->ctlr, 0);
+		kvm_arm_resume_guest(vcpu->kvm);
 	} else {
 		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
 					      GICR_CTLR_ENABLE_LPIS);
-- 
2.47.3

[PATCH v2 6/7] KVM: arm64: vgic-its: Fix MOVALL handling of source redistributor

[Marc Zyngier]

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 <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/vgic/vgic-its.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 9e782a4fea7e5..cc02d9b62bd95 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -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

[PATCH v2 7/7] KVM: arm64: vgic-its: Stop the VM when handling MOVALL

[Marc Zyngier]

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 <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-its.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index cc02d9b62bd95..094eede267e54 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -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

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

[Fuad Tabba]

Hi Marc,

On Tue, 29 Sep 2026 10:35:42 +0100, Marc Zyngier <maz@kernel.org> wrote:
[...]
> 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?

[...]
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
[...]
> @@ -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 :), 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].

nit: "let advertise" -> "publish"?

Cheers,
/fuad

[1] https://sashiko.dev/#/patchset/20260929093548.3598547-1-maz%40kernel.org?part=5

Re: [PATCH v2 2/7] KVM: arm64: Turn vcpu->arch.pause into a counter

[Fuad Tabba]

On Tue, 29 Sept 2026 at 10:36, Marc Zyngier <maz@kernel.org> wrote:
>
> 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 <maz@kernel.org>
> Cc: stable@vger.kernel.org

Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba < fuad.tabba@linux.dev>

Cheers,
/fuad


> ---
>  arch/arm64/include/asm/kvm_host.h |  7 +++----
>  arch/arm64/kvm/arm.c              | 21 ++++++++++++---------
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 27fe0cd5b2d7a..66ea2372f9870 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -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
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 5f5dd8bead4b9..9a4871cd796bc 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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
>

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

[Marc Zyngier]

On Tue, 29 Sep 2026 13:59:23 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> Hi Marc,
> 
> On Tue, 29 Sep 2026 10:35:42 +0100, Marc Zyngier <maz@kernel.org> wrote:
> [...]
> > 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.

> 
> [...]
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> [...]
> > @@ -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.

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

[Fuad Tabba]

Hi Marc,

On Tue, 29 Sep 2026 15:13:28 +0100, Marc Zyngier <maz@kernel.org> wrote:
[...]
> 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:

It doesn't save the bacon. I ran it through herd7 with the LKMM, the
fold walk in vgic_v3_fold_lr_state() against the list_del() in
vgic_flush_pending_lpis(): with smp_mb__before_atomic() and a failing
cmpxchg(), the walk can see the list_del(). With your
smp_load_acquire() it can't.

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

The fold walk takes no lock, so the locks on the halt side don't order
it against the list_del(). herd7 agrees: WRITE_ONCE() with your
acquire can see the list_del() again.

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

I don't understand Sashiko half of the time either :) That said, the
diff looks right to me.

Cheers,
/fuad

Re: [PATCH v2 6/7] KVM: arm64: vgic-its: Fix MOVALL handling of source redistributor

[Fuad Tabba]

On Tue, 29 Sept 2026 at 10:36, Marc Zyngier <maz@kernel.org> wrote:
>
> 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 <maz@kernel.org>
> Cc: stable@vger.kernel.org

Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba < fuad.tabba@linux.dev>

Cheers,
/fuad


> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 9e782a4fea7e5..cc02d9b62bd95 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -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
>

Re: [PATCH v2 7/7] KVM: arm64: vgic-its: Stop the VM when handling MOVALL

[Fuad Tabba]

On Tue, 29 Sept 2026 at 10:36, Marc Zyngier <maz@kernel.org> wrote:
>
> 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 <maz@kernel.org>

This is the only patch in the series without Cc: stable. Was that deliberate?

Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
Tested-by: Fuad Tabba < fuad.tabba@linux.dev>

Cheers,
/fuad


> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index cc02d9b62bd95..094eede267e54 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -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
>

[PATCH v1 0/2] KVM: arm64: selftests: Cover the ITS MOVALL command

[Fuad Tabba]

Hi Marc,

I wrote this to test your series [1]. It fails without patch 6 and
passes with the whole series, on QEMU with pKVM, VHE and nVHE hosts. On
an Apple M4 host, the series boots and runs protected and non-protected
guests. The M4's GIC has no ITS, so the test doesn't run there.

Feel free to take it with your series if you think it's useful.

Based on kvmarm/next (5c49bb52faf38).

Cheers,
/fuad

[1] https://lore.kernel.org/all/20260929093548.3598547-1-maz@kernel.org/

Fuad Tabba (2):
  KVM: arm64: selftests: Add a MOVALL command to the ITS library
  KVM: arm64: selftests: Add an ITS MOVALL test

 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/arm64/vgic_its_movall.c     | 341 ++++++++++++++++++
 .../selftests/kvm/include/arm64/gic_v3_its.h  |   1 +
 .../selftests/kvm/lib/arm64/gic_v3_its.c      |  16 +
 4 files changed, 359 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/arm64/vgic_its_movall.c


base-commit: 5c49bb52faf38ddf9c3383807b1d814ef8769b80
-- 
2.39.5

[PATCH v1 1/2] KVM: arm64: selftests: Add a MOVALL command to the ITS library

[Fuad Tabba]

Add its_send_movall_cmd() for a MOVALL test. Both redistributors are
encoded as processor numbers, as its_send_mapc_cmd() does.

Assisted-by: LLM
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 .../selftests/kvm/include/arm64/gic_v3_its.h     |  1 +
 .../testing/selftests/kvm/lib/arm64/gic_v3_its.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/arm64/gic_v3_its.h b/tools/testing/selftests/kvm/include/arm64/gic_v3_its.h
index a43a407e2d5c1..6a410c89c6445 100644
--- a/tools/testing/selftests/kvm/include/arm64/gic_v3_its.h
+++ b/tools/testing/selftests/kvm/include/arm64/gic_v3_its.h
@@ -14,6 +14,7 @@ void its_send_mapc_cmd(void *cmdq_base, u32 vcpu_id, u32 collection_id, bool val
 void its_send_mapti_cmd(void *cmdq_base, u32 device_id, u32 event_id,
 			u32 collection_id, u32 intid);
 void its_send_invall_cmd(void *cmdq_base, u32 collection_id);
+void its_send_movall_cmd(void *cmdq_base, u32 from_vcpu, u32 to_vcpu);
 void its_send_sync_cmd(void *cmdq_base, u32 vcpu_id);
 
 #endif // __SELFTESTS_GIC_V3_ITS_H__
diff --git a/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c b/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c
index 1188b578121dd..9237401a94fe8 100644
--- a/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c
+++ b/tools/testing/selftests/kvm/lib/arm64/gic_v3_its.c
@@ -159,6 +159,11 @@ static void its_encode_target(struct its_cmd_block *cmd, u64 target_addr)
 	its_mask_encode(&cmd->raw_cmd[2], target_addr >> 16, 51, 16);
 }
 
+static void its_encode_target2(struct its_cmd_block *cmd, u64 target_addr)
+{
+	its_mask_encode(&cmd->raw_cmd[3], target_addr >> 16, 51, 16);
+}
+
 static void its_encode_collection(struct its_cmd_block *cmd, u16 col)
 {
 	its_mask_encode(&cmd->raw_cmd[2], col, 15, 0);
@@ -253,6 +258,17 @@ void its_send_invall_cmd(void *cmdq_base, u32 collection_id)
 	its_send_cmd(cmdq_base, &cmd);
 }
 
+void its_send_movall_cmd(void *cmdq_base, u32 from_vcpu, u32 to_vcpu)
+{
+	struct its_cmd_block cmd = {};
+
+	its_encode_cmd(&cmd, GITS_CMD_MOVALL);
+	its_encode_target(&cmd, procnum_to_rdbase(from_vcpu));
+	its_encode_target2(&cmd, procnum_to_rdbase(to_vcpu));
+
+	its_send_cmd(cmdq_base, &cmd);
+}
+
 void its_send_sync_cmd(void *cmdq_base, u32 vcpu_id)
 {
 	struct its_cmd_block cmd = {};
-- 
2.39.5

[PATCH v1 2/2] KVM: arm64: selftests: Add an ITS MOVALL test

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

Assisted-by: LLM
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 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

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 908bdc7cf4f58..fb0bc99242975 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -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
diff --git a/tools/testing/selftests/kvm/arm64/vgic_its_movall.c b/tools/testing/selftests/kvm/arm64/vgic_its_movall.c
new file mode 100644
index 0000000000000..41918576bdf63
--- /dev/null
+++ b/tools/testing/selftests/kvm/arm64/vgic_its_movall.c
@@ -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 <fuad.tabba@linux.dev>
+ */
+
+#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;
+
+	gpa_t		device_table;
+	gpa_t		collection_table;
+	gpa_t		cmdq_base;
+	void		*cmdq_base_va;
+	gpa_t		itt_table;
+
+	gpa_t		lpi_prop_table;
+	gpa_t		lpi_pend_tables;
+} test_data;
+
+static atomic_uint lpi_taken[NR_VCPUS][NR_LPIS];
+
+static void guest_irq_handler(struct ex_regs *regs)
+{
+	u32 intid = gic_get_and_ack_irq();
+
+	if (intid == IAR_SPURIOUS)
+		return;
+
+	GUEST_ASSERT(intid >= GIC_LPI_OFFSET && intid < GIC_LPI_OFFSET + NR_LPIS);
+	atomic_fetch_add(&lpi_taken[guest_get_vcpuid()][intid - GIC_LPI_OFFSET], 1);
+	gic_set_eoi(intid);
+}
+
+static void guest_setup_its(void)
+{
+	void *cmdq = test_data.cmdq_base_va;
+	u32 i;
+
+	its_init(test_data.collection_table, SZ_64K,
+		 test_data.device_table, SZ_64K,
+		 test_data.cmdq_base, SZ_64K);
+
+	for (i = 0; i < NR_VCPUS; i++)
+		its_send_mapc_cmd(cmdq, i, i, true);
+
+	its_send_mapd_cmd(cmdq, DEVICE_ID, test_data.itt_table, SZ_64K, true);
+	its_send_mapti_cmd(cmdq, DEVICE_ID, 0, LPI_A_COLL, GIC_LPI_OFFSET);
+	its_send_mapti_cmd(cmdq, DEVICE_ID, 1, LPI_B_COLL, GIC_LPI_OFFSET + 1);
+
+	for (i = 0; i < NR_VCPUS; i++)
+		its_send_invall_cmd(cmdq, i);
+
+	for (i = 0; i < NR_VCPUS; i++)
+		its_send_sync_cmd(cmdq, i);
+}
+
+static void guest_move_lpi_a(void)
+{
+	void *cmdq = test_data.cmdq_base_va;
+
+	its_send_mapc_cmd(cmdq, MOVALL_TO, LPI_A_COLL, true);
+	/* The GICv3 spec requires a SYNC to the old redistributor before MOVALL. */
+	its_send_sync_cmd(cmdq, MOVALL_FROM);
+	its_send_movall_cmd(cmdq, MOVALL_FROM, MOVALL_TO);
+	its_send_sync_cmd(cmdq, MOVALL_TO);
+}
+
+static void guest_code(void)
+{
+	static atomic_int nr_cpus_ready;
+	u32 cpuid = guest_get_vcpuid();
+
+	gic_init(GIC_V3, NR_VCPUS);
+	gic_rdist_enable_lpis(test_data.lpi_prop_table, SZ_64K,
+			      test_data.lpi_pend_tables + (cpuid * SZ_64K));
+
+	atomic_fetch_add(&nr_cpus_ready, 1);
+
+	if (cpuid == 0) {
+		while (atomic_load(&nr_cpus_ready) < NR_VCPUS)
+			cpu_relax();
+
+		guest_setup_its();
+		guest_move_lpi_a();
+	}
+
+	local_irq_enable();
+
+	GUEST_SYNC(0);
+
+	/*
+	 * Don't use WFI here to avoid blocking the vCPU thread indefinitely and
+	 * never getting the stop signal.
+	 */
+	while (!READ_ONCE(test_data.request_vcpus_stop))
+		cpu_relax();
+
+	GUEST_DONE();
+}
+
+static void setup_memslot(void)
+{
+	size_t pages;
+	size_t sz;
+
+	/*
+	 * For the ITS: device table, collection table, command queue and one
+	 * ITT. For the redistributors: the LPI configuration table and an LPI
+	 * pending table per vCPU.
+	 */
+	sz = (5 + NR_VCPUS) * SZ_64K;
+
+	pages = sz / vm->page_size;
+	gpa_base = ((vm_compute_max_gfn(vm) + 1) * vm->page_size) - sz;
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, gpa_base,
+				    TEST_MEMSLOT_INDEX, pages, 0);
+}
+
+static void configure_lpis(void)
+{
+	u8 *tbl = addr_gpa2hva(vm, test_data.lpi_prop_table);
+	int i;
+
+	for (i = 0; i < NR_LPIS; i++)
+		tbl[i] = LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1 | LPI_PROP_ENABLED;
+}
+
+static gpa_t alloc_64k(size_t nr)
+{
+	size_t pages_per_64k = vm_calc_num_guest_pages(vm->mode, SZ_64K);
+
+	return vm_phy_pages_alloc(vm, pages_per_64k * nr, gpa_base, TEST_MEMSLOT_INDEX);
+}
+
+static void setup_test_data(void)
+{
+	size_t pages_per_64k = vm_calc_num_guest_pages(vm->mode, SZ_64K);
+
+	test_data.device_table = alloc_64k(1);
+	test_data.collection_table = alloc_64k(1);
+
+	test_data.cmdq_base = alloc_64k(1);
+	virt_map(vm, test_data.cmdq_base, test_data.cmdq_base, pages_per_64k);
+	test_data.cmdq_base_va = (void *)test_data.cmdq_base;
+
+	test_data.itt_table = alloc_64k(1);
+
+	test_data.lpi_prop_table = alloc_64k(1);
+	configure_lpis();
+
+	test_data.lpi_pend_tables = alloc_64k(NR_VCPUS);
+
+	sync_global_to_guest(vm, test_data);
+}
+
+static void signal_lpi(u32 event_id)
+{
+	gpa_t db_addr = GITS_BASE_GPA + GITS_TRANSLATER;
+
+	struct kvm_msi msi = {
+		.address_lo	= db_addr,
+		.address_hi	= db_addr >> 32,
+		.data		= event_id,
+		.devid		= DEVICE_ID,
+		.flags		= KVM_MSI_VALID_DEVID,
+	};
+
+	TEST_ASSERT(__vm_ioctl(vm, KVM_SIGNAL_MSI, &msi) == 1,
+		    "KVM_SIGNAL_MSI ioctl failed");
+}
+
+static pthread_barrier_t test_setup_barrier;
+
+static void *vcpu_worker_thread(void *data)
+{
+	struct kvm_vcpu *vcpu = data;
+	struct ucall uc;
+
+	while (true) {
+		vcpu_run(vcpu);
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_SYNC:
+			pthread_barrier_wait(&test_setup_barrier);
+			continue;
+		case UCALL_DONE:
+			return NULL;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		default:
+			TEST_FAIL("Unknown ucall: %lu", uc.cmd);
+		}
+	}
+
+	return NULL;
+}
+
+static unsigned int lpi_taken_on(atomic_uint (*taken)[NR_LPIS], int vcpu, int lpi)
+{
+	return atomic_load(&taken[vcpu][lpi]);
+}
+
+static void wait_for_lpis(atomic_uint (*taken)[NR_LPIS])
+{
+	int lpi, vcpu, i;
+
+	for (i = 0; i < 10000; i++) {
+		unsigned int nr = 0;
+
+		for (lpi = 0; lpi < NR_LPIS; lpi++)
+			for (vcpu = 0; vcpu < NR_VCPUS; vcpu++)
+				nr += !!lpi_taken_on(taken, vcpu, lpi);
+
+		if (nr == NR_LPIS)
+			return;
+
+		usleep(1000);
+	}
+
+	TEST_FAIL("LPIs not taken after 10s");
+}
+
+static void check_movall(atomic_uint (*taken)[NR_LPIS])
+{
+	int vcpu;
+
+	for (vcpu = 0; vcpu < NR_VCPUS; vcpu++) {
+		TEST_ASSERT(!lpi_taken_on(taken, vcpu, 0) == (vcpu != MOVALL_TO),
+			    "LPI A taken %u times on vCPU%d, expected only on vCPU%d",
+			    lpi_taken_on(taken, vcpu, 0), vcpu, MOVALL_TO);
+		TEST_ASSERT(!lpi_taken_on(taken, vcpu, 1) == (vcpu != LPI_B_VCPU),
+			    "LPI B taken %u times on vCPU%d, expected only on vCPU%d",
+			    lpi_taken_on(taken, vcpu, 1), vcpu, LPI_B_VCPU);
+	}
+}
+
+static void run_test(void)
+{
+	atomic_uint (*taken)[NR_LPIS] = addr_gva2hva(vm, (gva_t)lpi_taken);
+	pthread_t vcpu_threads[NR_VCPUS];
+	size_t i;
+
+	pthread_barrier_init(&test_setup_barrier, NULL, NR_VCPUS + 1);
+
+	for (i = 0; i < NR_VCPUS; i++)
+		kvm_pthread_create(&vcpu_threads[i], NULL, vcpu_worker_thread, vcpus[i]);
+
+	pthread_barrier_wait(&test_setup_barrier);
+
+	signal_lpi(0);
+	signal_lpi(1);
+	wait_for_lpis(taken);
+
+	write_guest_global(vm, test_data.request_vcpus_stop, true);
+
+	for (i = 0; i < NR_VCPUS; i++)
+		kvm_pthread_join(vcpu_threads[i], NULL);
+
+	check_movall(taken);
+}
+
+static void setup_vm(void)
+{
+	int i;
+
+	vm = vm_create_with_vcpus(NR_VCPUS, guest_code, vcpus);
+
+	vm_init_descriptor_tables(vm);
+	for (i = 0; i < NR_VCPUS; i++)
+		vcpu_init_descriptor_tables(vcpus[i]);
+
+	vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
+
+	setup_memslot();
+
+	its_fd = vgic_its_setup(vm);
+
+	setup_test_data();
+}
+
+static void destroy_vm(void)
+{
+	close(its_fd);
+	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	TEST_REQUIRE(kvm_supports_vgic_v3());
+
+	setup_vm();
+
+	run_test();
+
+	destroy_vm();
+
+	return 0;
+}
-- 
2.39.5

Re: (subset) [PATCH v2 0/7] KVM: arm64: vgic-v3: Make LPI disabling robust (and more)

[Oliver Upton]

On Tue, 29 Sep 2026 10:35:41 +0100, Marc Zyngier wrote:
> 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.
> 
> [...]

Applied to fixes, thanks!

[3/7] KVM: arm64: vgic: Allow last_lr_irq to be NULL when LRs are not overflowing
      https://git.kernel.org/kvmarm/kvmarm/c/e079ef0b1c40
[4/7] KVM: arm64: vgic: Take a refcount on IRQs referenced by last_lr_irq
      https://git.kernel.org/kvmarm/kvmarm/c/19bc5d79ece6
[6/7] KVM: arm64: vgic-its: Fix MOVALL handling of source redistributor
      https://git.kernel.org/kvmarm/kvmarm/c/751f4641560b

--
Best,
Oliver

Re: [PATCH v1 2/2] KVM: arm64: selftests: Add an ITS MOVALL test

[Marc Zyngier]

On Tue, 29 Sep 2026 20:04:52 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> 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.

> 
> Assisted-by: LLM
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> ---
>  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
> 
> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 908bdc7cf4f58..fb0bc99242975 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -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
> diff --git a/tools/testing/selftests/kvm/arm64/vgic_its_movall.c b/tools/testing/selftests/kvm/arm64/vgic_its_movall.c
> new file mode 100644
> index 0000000000000..41918576bdf63
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arm64/vgic_its_movall.c
> @@ -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 <fuad.tabba@linux.dev>
> + */
> +
> +#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;
> +
> +	gpa_t		device_table;
> +	gpa_t		collection_table;
> +	gpa_t		cmdq_base;
> +	void		*cmdq_base_va;
> +	gpa_t		itt_table;
> +
> +	gpa_t		lpi_prop_table;
> +	gpa_t		lpi_pend_tables;
> +} test_data;

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.

Re: [PATCH v1 2/2] KVM: arm64: selftests: Add an ITS MOVALL test

[Fuad Tabba]

Hi Marc,

On Wed, 30 Sept 2026 at 13:21, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 29 Sep 2026 20:04:52 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> >
> > 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.

Ack.

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

If you think this test is worth having, I'll respin v2 that way.

Cheers,
/fuad

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

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

[Will Deacon]

Hi folks,

Sorry, but this is probably an incredibly unhelpful drive-by comment
but Marc was talking about vcpu->mode the other day and I couldn't
resist looking at it some more. Like a moth to a flame...

See below.

On Tue, Sep 29, 2026 at 03:13:28PM +0100, Marc Zyngier wrote:
> On Tue, 29 Sep 2026 13:59:23 +0100,
> Fuad Tabba <fuad.tabba@linux.dev> wrote:
> > On Tue, 29 Sep 2026 10:35:42 +0100, Marc Zyngier <maz@kernel.org> wrote:
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > [...]
> > > @@ -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:

(as discussed off list, you can use smp_acquire__after_ctrl_dep() if
you're feeling really brave)

> 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);

I'm struggling to see why a release is sufficient here, but I'm also
struggling to understand the bigger picture so I'm probably just confused.

I can see why a release is necessary for the saved state to be visible
to another CPU that has kicked the vCPU out of the guest and then uses
vcpu->mode == OUTSIDE_GUEST_MODE as the indication that the state is
safe to consume. However, don't we also need to make sure that any
subsequent check for a pending request on _this_ vCPU is ordered after
that write to the mode? Now that we've toggled it away from IN_GUEST_MODE,
I think IPIs can be elided by the kick, so a subsequent call to
e.g. kvm_request_pending() must be observed after that toggle, otherwise
I think we could miss a request.

Can you see the tree I'm barking up here?

Will

Re: [PATCH v2 0/7] KVM: arm64: vgic-v3: Make LPI disabling robust (and more)

[Fuad Tabba]

Hi Marc,

On Tue, 29 Sep 2026 10:35:41 +0100, Marc Zyngier <maz@kernel.org> wrote:
[...]
> 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?

Cheers,
/fuad

Re: [PATCH v2 0/7] KVM: arm64: vgic-v3: Make LPI disabling robust (and more)

[Oliver Upton]

Hi Fuad,

On Thu, Oct 08, 2026 at 07:15:46PM +0100, Fuad Tabba wrote:
> Hi Marc,
> 
> On Tue, 29 Sep 2026 10:35:41 +0100, Marc Zyngier <maz@kernel.org> wrote:
> [...]
> > 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?

Hmm, since there's no parent lock between disabling LPIs and translation
cache fills I believe there's still a chance for this to race. 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.

If only Arm went a bit further than "strongly recommends" on migrating
LPIs _before_ flipping the bit...

Thanks,
Oliver