Re: [PATCH v2 0/2] KVM: arm64: vgic-its: Fix LPI double mapping and DISCARD

[Fuad Tabba <fuad.tabba@linux.dev>]

Hi Marc,

On Tue, 13 Oct 2026 14:25:09 +0100, Marc Zyngier <maz@kernel.org> wrote:
[...]
> > We do still need it for the window after the exit: with the delay
> > there and the same patches applied, the guest takes it in none of 20
> > runs, against 13 of 20 on v2.
>
> I don't understand what these probabilities represent. Either it works
> or it doesn't. And given that I have no idea what you are testing
> with, I can't really make head or tail of the above.

They're the number of runs, out of 20, in which the guest took the
discarded LPI. My reproducer has vCPU 0 issue DISCARD for an LPI pending
in vCPU 1's LR, at a varying time, with a debug mdelay() widening the
window, so only some runs hit it. I've since replaced the delay with
debug hooks that force the race, so now it either works or it doesn't.

[...]
> If you have a herd litmus test, how about posting it? It would really
> avoid a lot of that back and forth where the S/N ratio is pretty poor.

Sure, here are two, run with herd7 7.58 and the LKMM from v7.3-rc3. The
first models your diff and comes out Sometimes. The second adds the
request check before the flush and comes out Never.

C discard+halt

(*
 * P0 is the vCPU in kvm_arch_vcpu_ioctl_run() with your diff, with a
 * pending LPI queued on it: check_vcpu_requests(), the IN_GUEST_MODE
 * store, the flush moving pending_latch into an LR, the fold (on the
 * early exit, or after a guest run that leaves the LPI pending), and
 * the release of OUTSIDE_GUEST_MODE.
 *
 * P1 is DISCARD: kvm_arm_halt_guest() making KVM_REQ_SLEEP and reading
 * the mode in kvm_vcpu_exiting_guest_mode(), then clearing
 * pending_latch. A halt that reads IN or EXITING (1, 2) waits for the
 * IPI, which the vCPU takes after its fold, so only one that reads
 * OUTSIDE (0) clears here.
 *)

{
	pending_latch = 1;
}

P0(int *mode, int *requests, int *pending_latch, spinlock_t *irq_lock)
{
	int r0;
	int r1 = 0;

	r0 = READ_ONCE(*requests);
	if (r0 == 0) {
		smp_store_mb(*mode, 1);

		spin_lock(irq_lock);
		r1 = READ_ONCE(*pending_latch);
		if (r1 == 1)
			WRITE_ONCE(*pending_latch, 0);
		spin_unlock(irq_lock);

		spin_lock(irq_lock);
		if (r1 == 1)
			WRITE_ONCE(*pending_latch, 1);
		spin_unlock(irq_lock);

		smp_store_release(mode, 0);
	}
}

P1(int *mode, int *requests, int *pending_latch, spinlock_t *irq_lock)
{
	int r3;

	smp_wmb();
	WRITE_ONCE(*requests, 1);
	smp_mb__before_atomic();
	r3 = cmpxchg(mode, 1, 2);
	if (r3 == 0) {
		spin_lock(irq_lock);
		WRITE_ONCE(*pending_latch, 0);
		spin_unlock(irq_lock);
	}
}

exists (0:r1=1 /\ 1:r3=0 /\ pending_latch=1)

C discard+halt+check

(*
 * As discard+halt, with the request check between the IN_GUEST_MODE
 * store and the flush.
 *)

{
	pending_latch = 1;
}

P0(int *mode, int *requests, int *pending_latch, spinlock_t *irq_lock)
{
	int r0;
	int r1 = 0;
	int r2;

	r0 = READ_ONCE(*requests);
	if (r0 == 0) {
		smp_store_mb(*mode, 1);

		r2 = READ_ONCE(*requests);
		if (r2 == 0) {
			spin_lock(irq_lock);
			r1 = READ_ONCE(*pending_latch);
			if (r1 == 1)
				WRITE_ONCE(*pending_latch, 0);
			spin_unlock(irq_lock);

			spin_lock(irq_lock);
			if (r1 == 1)
				WRITE_ONCE(*pending_latch, 1);
			spin_unlock(irq_lock);
		}

		smp_store_release(mode, 0);
	}
}

P1(int *mode, int *requests, int *pending_latch, spinlock_t *irq_lock)
{
	int r3;

	smp_wmb();
	WRITE_ONCE(*requests, 1);
	smp_mb__before_atomic();
	r3 = cmpxchg(mode, 1, 2);
	if (r3 == 0) {
		spin_lock(irq_lock);
		WRITE_ONCE(*pending_latch, 0);
		spin_unlock(irq_lock);
	}
}

exists (0:r1=1 /\ 1:r3=0 /\ pending_latch=1)

[...]
> Is the hack below what you are asking for? I can't tell. If it isn't,
> please post your own version.

Almost: it's missing a request check between the IN_GUEST_MODE store and
the flush. Without it, a halt that reads OUTSIDE_GUEST_MODE just before
the store doesn't wait for the vCPU, which then loads the LPI into an
LR, takes the early exit on KVM_REQ_SLEEP and folds the LPI back after
DISCARD has cleared it. That's the outcome in the first litmus test's
exists clause. With the debug hooks forcing it on QEMU, the guest takes
the discarded LPI in every run with your diff, and in none with this on
top:

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index f1449b0..896bc76 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1329,11 +1329,29 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		 */
 		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
 
+		/*
+		 * Don't load the LRs with a request pending:
+		 * kvm_arm_halt_guest() doesn't wait for a vCPU it reads
+		 * as OUTSIDE_GUEST_MODE.
+		 */
+		if (ret <= 0 || kvm_vcpu_exit_request(vcpu, &ret)) {
+			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);
+			smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);
+			local_irq_enable();
+			preempt_enable();
+			continue;
+		}
+
 		kvm_vgic_flush_hwstate(vcpu);
 
 		kvm_pmu_update_vcpu_events(vcpu);
 
-		if (ret <= 0 || kvm_vcpu_exit_request(vcpu, &ret)) {
+		/* kvm_vgic_flush_hwstate() can make a request when nested */
+		if (kvm_vcpu_exit_request(vcpu, &ret)) {
 			isb(); /* Ensure work in x_flush_hwstate is committed */
 			if (kvm_vcpu_has_pmu(vcpu))
 				kvm_pmu_sync_hwstate(vcpu);

Cheers,
/fuad