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.

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@amd.com>
---
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)
+{
+    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) ) {
         unsigned int reg;
         int pin;
 
@@ -1609,9 +1616,6 @@ static bool io_apic_level_ack_pending(unsigned int irq)
             spin_unlock_irqrestore(&ioapic_lock, flags);
             return 1;
         }
-        if (!entry->next)
-            break;
-        entry = irq_2_pin + entry->next;
     }
     spin_unlock_irqrestore(&ioapic_lock, flags);
 
-- 
2.51.0

On 10/13/25 11:11 PM, 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.
>
> 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@amd.com>

Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

> ---
> 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)
> +{
> +    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) ) {
>           unsigned int reg;
>           int pin;
>   
> @@ -1609,9 +1616,6 @@ static bool io_apic_level_ack_pending(unsigned int irq)
>               spin_unlock_irqrestore(&ioapic_lock, flags);
>               return 1;
>           }
> -        if (!entry->next)
> -            break;
> -        entry = irq_2_pin + entry->next;
>       }
>       spin_unlock_irqrestore(&ioapic_lock, flags);
>   

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@amd.com>
> ---
> 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?

> +{
> +    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.

>          unsigned int reg;
>          int pin;

Below here you can remove the:

        if (!entry)
            break;

Chunk, as the for loop already checks for this condition.

Otherwise looks good, I think we should consider for 4.21 inclusion.

Thanks, Roger.

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@amd.com>
>> ---
>> 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?

Jan

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@amd.com>
>>> ---
>>> 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)

?

Thanks,
Jason

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@amd.com>
>>>> ---
>>>> 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

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 <jason.andryuk@amd.com>
> > > > ---
> > > > 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.

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@amd.com>
>> ---
>> 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