On Wed, Oct 15, 2025 at 01:14:20PM -0400, Jason Andryuk wrote:
> On 2025-10-15 08:59, Jan Beulich wrote:
> > On 14.10.2025 09: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.
> > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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.
> >
> > Would this intention ...
> >
> > > > --- 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?
> >
> > ... possibly conflict with such a change?
>
> I changed only the parameter to const, and the return value is still
> non-const. So I think that will be re-usable.
>
> I placed next_entry() immediately before its use in
> io_apic_level_ack_pending(). It would need to be earlier in the file to be
> used more. Should I move its addition earlier?
>
> And another Minor question. Roger asked for ~Linux style in the for loop.
> But in next_entry() I have Xen style:
> if ( !entry->next )
>
> Should I switch to:
> if (!entry->next)
>
> ?
IMO for complete functions newly introduced it's fine to use Xen
style, I don't think we will ever import anything else from Linux to
this file, we have already diverged too much.
Regards, Roger.
