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