Re: [PATCH v2] x86/apic: Avoid infinite loop in io_apic_level_ack_pending()

[Jason Andryuk <jason.andryuk@xxxxxxx>]

Hi Roger,

On 2025-10-14 03:37, Roger Pau Monné wrote:
> On Mon, Oct 13, 2025 at 05:11:06PM -0400, Jason Andryuk wrote:
> > io_apic_level_ack_pending() will end up in an infinite loop if
> > entry->pin == -1.  entry does not change, so it will keep reading -1.
> Do you know how you end up with an entry with pin == -1 on the
> irq_pin_list? Are there systems with gaps in the GSI space between
> IO-APICs?  So far everything I saw had the IO-APIC in contiguous GSI
> space.
I only noticed this potential infinite loop during code inspection.  I 
mentioned that in a v1 post commit comment, but I forgot it in v2.
> > Convert to a proper for loop so that continue works.  Add a new helper,
> > next_entry(), to handle advancing to the next irq_pin_list entry.
> >
> > Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
> > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> > ---
> > v2:
> > continue (not break) for pin == -1.
> >
> > I added the next_entry() helper since putting the expression in the for
> > loop is a little cluttered.  The helper can also be re-used for other
> > instances within the file.
> > ---
> >   xen/arch/x86/io_apic.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> > index c384f10c1b..7b58345c96 100644
> > --- a/xen/arch/x86/io_apic.c
> > +++ b/xen/arch/x86/io_apic.c
> > @@ -1586,14 +1586,21 @@ static int __init cf_check setup_ioapic_ack(const char 
> > *s)
> >   }
> >   custom_param("ioapic_ack", setup_ioapic_ack);
> >   
> > +static struct irq_pin_list *next_entry(struct irq_pin_list *entry)
> I think you can make the entry parameter const?
Ok.

> > +{
> > +    if ( !entry->next )
> > +        return NULL;
> > +
> > +    return irq_2_pin + entry->next;
> > +}
> > +
> >   static bool io_apic_level_ack_pending(unsigned int irq)
> >   {
> >       struct irq_pin_list *entry;
> >       unsigned long flags;
> >   
> >       spin_lock_irqsave(&ioapic_lock, flags);
> > -    entry = &irq_2_pin[irq];
> > -    for (;;) {
> > +    for ( entry = &irq_2_pin[irq]; entry ; entry = next_entry(entry) ) {
> I'm not sure where we stand regarding coding style here, but it looks
> you either want to remove the space between parentheses (my
> preference), or place the opening for braces on a newline.  I would
> possibly do:
>
> for (entry = &irq_2_pin[irq]; entry; entry = next_entry(entry)) {
> ...
>
> As I think it fits better given the small change and the surrounding
> coding style.
That works for me.

> >           unsigned int reg;
> >           int pin;
> Below here you can remove the:
>
>          if (!entry)
>              break;
>
> Chunk, as the for loop already checks for this condition.
Yes, thanks for catching that.

Regards,
Jason