[PATCH v2 0/2] KVM: arm64: vgic-its: Fix LPI double mapping and DISCARD
Marc Zyngier
maz at kernel.org
Wed Oct 14 06:34:09 PDT 2026
On Tue, 13 Oct 2026 15:33:38 +0100,
Fuad Tabba <fuad.tabba at 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.
More information about the linux-arm-kernel
mailing list