Re: [PATCH v4 1/4] KVM: x86: ioapic: Update remote_irr only after successful delivery

["Huang, Kai" <kai.huang@intel.com>]

On Tue, 2026-08-11 at 11:11 +0000, Huang, Kai wrote:
> On Tue, 2026-08-11 at 15:34 +0800, Hao Zhang wrote:
> > From: Hao Zhang <zhanghao1@kylinos.cn>
> > 
> > The I/O APIC sets remote_irr for level-triggered interrupts that have
> > been accepted by a local APIC and are waiting for an EOI.
> > 
> > ioapic_service() currently treats any non-zero return from
> > kvm_irq_delivery_to_apic() as successful delivery.  But
> > kvm_irq_delivery_to_apic() returns -1 when no destination is found.
> > Setting remote_irr for that failed delivery leaves the pin blocked
> > forever waiting for an EOI that will never be generated.
> > 
> > Update remote_irr 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.")
> 
> The code LGTM, but I am not sure this is the right commit for the Fixes tag.
> 
> Looking at the relevant diff:
> 
> +static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
>  {
>         union ioapic_redir_entry *pent;
> +       int injected = -1;
>  
>         pent = &ioapic->redirtbl[idx];
>  
>         if (!pent->fields.mask) {
> -               int injected = ioapic_deliver(ioapic, idx);
> +               injected = ioapic_deliver(ioapic, idx);
>                 if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
>                         pent->fields.remote_irr = 1;
>         }
> 
> This commit didn't change any logic related to remote_irr, since it didn't touch
> ioapic_deliver() at all.
> 

I asked the AI to double confirm what I was reading last night, and indeed I got
mixed up a bit among multiple commits in history.

Firstly, the above "it didn't touch ioapic_deliver() at all" isn't true.  It
indeed introduced 'r = -1' initializer at the beginning of the function.  While
I was right for "no target on destination" it returns 0 (but not -1), AI also
pointed out there are other cases such as invalid delivery_mode which can cause
ioapic_deliver() to return -1.

FWIW, AI also found it was commit 

  a53c17d21c46 ("KVM: ioapic/msi interrupt delivery consolidation")

which made ioapic_deliver() return -1 when "no target on destination":

        if (find_first_bit(deliver_bitmask, KVM_MAX_VCPUS) >= KVM_MAX_VCPUS) {
                ioapic_debug("no target on destination\n");
  -             return 0;
  +             return r;
        }

So I guess you are right, 4925663a079c is the right one to blame, but not due to
"no target on destination" case.

Anyway, IMHO we might also want to put such history digging into the changelog
to justify the Fixes tag, so people can easily see the reason behind it.

That being said:

Reviewed-by: Kai Huang <kai.huang@intel.com>