Re: [PATCH 1/2] KVM: x86: ioapic: Update state only after successful delivery

On Mon, 2026-08-10 at 14:17 +0800, Hao Zhang wrote:
> From: Hao Zhang <[email protected]>
> 
> The I/O APIC tracks delivered interrupts in state that is later used to
> decide whether an interrupt is still pending or blocked waiting for an
> EOI.
> 
> For level-triggered interrupts, remote_irr means that a local APIC
> accepted the interrupt and that the I/O APIC must wait for the
> corresponding EOI before delivering the interrupt again.  For
> edge-triggered interrupts, irr_delivered is used to hide delivered
> interrupts from KVM_GET_IRQCHIP so that userspace does not reinject an
> interrupt that has already left the I/O APIC.
> 
> But ioapic_service() currently updates that state before or without
> checking that interrupt delivery actually succeeded.
> kvm_irq_delivery_to_apic() can return -1 when no destination is found.
> Treating failed delivery as success can either leave a level-triggered
> pin blocked forever waiting for an EOI that will never be generated, or
> cause KVM_GET_IRQCHIP to drop an undelivered edge-triggered interrupt
> from the saved IRR state.
> 
> Update I/O APIC delivery state only when the delivery result is positive,
> i.e. when at least one local APIC accepted the interrupt.
> 
> Fixes: 4925663a079c ("KVM: Report IRQ injection status to userspace.")
> Fixes: 5bda6eed2e36 ("KVM: ioapic: Record edge-triggered interrupts delivery status")
> Signed-off-by: Hao Zhang <[email protected]>
> ---
> Changes in v2:
>  - Address Kai Huang's review by deferring both remote_irr and
>    irr_delivered updates until interrupt delivery succeeds.
>  - Extend the selftest to cover failed edge-triggered delivery and verify
>    that the interrupt remains pending in IRR.
> 
> Link to v1: https://lore.kernel.org/all/[email protected]/
> 
>  arch/x86/kvm/ioapic.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 757667fb2bfa..540e5665fbe4 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -474,9 +474,6 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
>  	irqe.shorthand = APIC_DEST_NOSHORT;
>  	irqe.msi_redir_hint = false;
>  
> -	if (irqe.trig_mode == IOAPIC_EDGE_TRIG)
> -		ioapic->irr_delivered |= 1 << irq;
> -
>  	if (irq == RTC_GSI && line_status) {
>  		/*
>  		 * pending_eoi cannot ever become negative (see
> @@ -491,8 +488,12 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
>  	} else
>  		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
>  
> -	if (ret && irqe.trig_mode == IOAPIC_LEVEL_TRIG)
> -		entry->fields.remote_irr = 1;
> +	if (ret > 0) {
> +		if (irqe.trig_mode == IOAPIC_EDGE_TRIG)
> +			ioapic->irr_delivered |= 1 << irq;
> +		else if (irqe.trig_mode == IOAPIC_LEVEL_TRIG)
> +			entry->fields.remote_irr = 1;
> +	}
> 

Ah looking at this I immediately realized I misread the code, that I thought the
irr_delivered was for level triggered as well, but actually it is for edge
triggered.  I guess we both got it wrong :-)

For edge triggered IRQ the existing code is correct I believe, since once the
function is called the IRQ is considered delivered no matter whether it is
actually accepted by LAPIC.  I think this exactly reflects the hardware
behaviour.  (In fact, in ioapic_set_irq() you can see the IRQ is removed from
irr_delivered for edge triggered right before ioapic_service() is called.)

For level triggered, I don't see ioapic->irr is cleared by KVM, but only cleared
when ioapic_set_irq() is called with irq_level == 0.  I now (AFAICT) realize it
is the right behaviour since it should be the driver which dessert the level to
stop the level-triggered IRQ.

So in short, I think your v1 is correct, and sorry about my noise. :-(