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

[Marc Zyngier <maz@kernel.org>]

On Tue, 13 Oct 2026 15:33:38 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
>
> [...]
> > 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);
> 

I don't think this is right. Yes, it probably does enough request
checks that we end-up with not getting it completely wrong, but the
issues are that:

- we have state change before we evaluate the requests

- we mix request generation and state change in a single helper

and we should be able to fix that so that there is only a single
request checking point in the whole entry sequence.

I ended-up with a much bigger change, which really needs to be split
in two:

- untangle state generation and requests: this means that the
  maintenance interrupt generation and the request. The request
  generating stuff must happen before being IN_GUEST_MODE and request
  checking.

- the flushing must happen later on, once we don't look back anymore
  and have committed to entering the guest.

This, in turn, simplifies the "shit, I must rollback all the previous
changes as I can't enter the guest and must handle a request instead"
horror.

Another bug fix is that we now generate the maintenance interrupt
before deciding whether to generate a request switching from L2 to L1
as we have a pending interrupt.

Thoughts?

	M.

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 0576c2022ef57..3fa9e158f0be1 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1321,10 +1321,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 
 		local_irq_disable();
 
-		kvm_vgic_flush_hwstate(vcpu);
-
-		kvm_pmu_update_vcpu_events(vcpu);
-
 		/*
 		 * Ensure we set mode to IN_GUEST_MODE after we disable
 		 * interrupts and before the final VCPU requests check.
@@ -1334,18 +1330,19 @@ 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;
 		}
 
+		kvm_vgic_flush_hwstate(vcpu);
+
+		kvm_pmu_update_vcpu_events(vcpu);
+
 		kvm_arch_vcpu_ctxflush_fp(vcpu);
 
 		/**************************************************************
@@ -1356,7 +1353,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
@@ -1390,6 +1386,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
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index b191365d97cc2..41f3def0eecae 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -2046,6 +2046,9 @@ void check_nested_vcpu_requests(struct kvm_vcpu *vcpu)
  *
  * What follows is some slop to (1) react to exception routing / masking and (2)
  * preserve the pending SError state across translation regimes.
+ *
+ * Also look generate the maintenance interrupt state, and finally check
+ * that if we need to take an interrupt targetting the guest hypervisor.
  */
 void kvm_nested_flush_hwstate(struct kvm_vcpu *vcpu)
 {
@@ -2054,6 +2057,11 @@ void kvm_nested_flush_hwstate(struct kvm_vcpu *vcpu)
 
 	if (unlikely(vcpu_test_and_clear_flag(vcpu, NESTED_SERROR_PENDING)))
 		kvm_inject_serror_esr(vcpu, vcpu_get_vsesr(vcpu));
+
+	vgic_v3_nested_update_mi(vcpu);
+
+	if (vgic_state_is_nested(vcpu) && kvm_vgic_vcpu_pending_irq(vcpu))
+		kvm_make_request(KVM_REQ_GUEST_HYP_IRQ_PENDING, vcpu);
 }
 
 void kvm_nested_sync_hwstate(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 5cf5a1ef86cdd..7061b4ab206c9 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -1152,7 +1152,7 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	 *
 	 *   We do that by placing a request to ourselves which will
 	 *   abort the entry procedure and inject the exception at the
-	 *   beginning of the run loop.
+	 *   beginning of the run loop (see kvm_nested_flush_hwstate()).
 	 *
 	 * - Otherwise, do exactly *NOTHING* apart from enabling the virtual
 	 *   CPU interface. The guest state is already loaded, and we can
@@ -1162,16 +1162,10 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	 * maintenance interrupt state, as it may fire.
 	 */
 	if (vgic_state_is_nested(vcpu)) {
-		if (kvm_vgic_vcpu_pending_irq(vcpu))
-			kvm_make_request(KVM_REQ_GUEST_HYP_IRQ_PENDING, vcpu);
-
 		vgic_v3_flush_nested(vcpu);
 		return;
 	}
 
-	if (vcpu_has_nv(vcpu))
-		vgic_v3_nested_update_mi(vcpu);
-
 	DEBUG_SPINLOCK_BUG_ON(!irqs_disabled());
 
 	vgic_flush_state(vcpu);
-- 
2.47.3


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