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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2025 19:14, 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 <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.
>>
>> 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?

I think so. One other thing which came to mind only after sending the earlier
reply: "next_entry()" is overly generic a name when it's to be moved away
from its only user. "next_pin_list_entry()" maybe? Or "pin_list_next()"?

> 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)
> 
> ?

I'd say no.

Jan