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

["hao_zhang_kdev@163.com" <hao_zhang_kdev@163.com>]

On Mon, Aug 10, 2026, Huang, Kai wrote:
> On Mon, 2026-08-10 at 14:17 +0800, Hao Zhang wrote:
> > From: Hao Zhang <zhanghao1@kylinos.cn>
> > 
> > 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 <zhanghao1@kylinos.cn>
> > ---
> > 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/anSOdijwS6LBbfYJ@192.168.1.215/
> > 
> >  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. :-(

Thanks Kai,

I checked the Intel I/O APIC redirection table documentation(https://edc.intel.com/content/www/it/it/design/products-and-solutions/processors-and-chipsets/comet-lake-u/intel-400-series-chipset-on-package-platform-controller-hub-register-database/1.2/redirection-table-entry-0-rte0-offset-10/).  
The Remote IRR bit is defined for level-triggered interrupts; for edge-triggered
interrupts its meaning is undefined. So yes, treating irr_delivered like
remote_irr was wrong.

For edge-triggered interrupts, once the I/O APIC starts delivering the
interrupt, the interrupt should no longer be considered pending in the I/O
APIC state that is saved through KVM_GET_IRQCHIP, regardless of whether a
local APIC eventually accepts it.

So I agree that v2 went too far by moving the irr_delivered update behind
the delivery result check.  I will drop that part and restore the patch to
only fix the level-triggered remote_irr case.

Thanks,
Hao