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.

Switched to breaking out of the loop.

Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
Noticed during code inspection.
---
 xen/arch/x86/io_apic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 17e6827f4b..b21f0515f5 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1715,7 +1715,7 @@ static bool io_apic_level_ack_pending(unsigned int irq)
 
         pin = entry->pin;
         if (pin == -1)
-            continue;
+            break;
         reg = io_apic_read(entry->apic, 0x10 + pin*2);
         /* Is the remote IRR bit set? */
         if (reg & IO_APIC_REDIR_REMOTE_IRR) {
-- 
2.51.0

On 04.09.2025 23:51, 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.
> 
> Switched to breaking out of the loop.
> 
> Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> Noticed during code inspection.

Well spotted, just that ...

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1715,7 +1715,7 @@ static bool io_apic_level_ack_pending(unsigned int irq)
>  
>          pin = entry->pin;
>          if (pin == -1)
> -            continue;
> +            break;

... we shouldn't terminate the loop here, but rather continue with the next
entry in the list (if any). Hence presumably why "continue" was used, without
achieving the intended effect.

Jan

On 2025-09-05 03:39, Jan Beulich wrote:
> On 04.09.2025 23:51, 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.
>>
>> Switched to breaking out of the loop.
>>
>> Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> ---
>> Noticed during code inspection.
> 
> Well spotted, just that ...
> 
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -1715,7 +1715,7 @@ static bool io_apic_level_ack_pending(unsigned int irq)
>>   
>>           pin = entry->pin;
>>           if (pin == -1)
>> -            continue;
>> +            break;
> 
> ... we shouldn't terminate the loop here, but rather continue with the next
> entry in the list (if any). Hence presumably why "continue" was used, without
> achieving the intended effect.

Ok, makes sense.  Though after the sending the patch, I was wondering if 
it was an unreachable condition, and we should not end up here with pin 
== -1.

Thanks,
Jason

On 05.09.2025 22:34, Jason Andryuk wrote:
> On 2025-09-05 03:39, Jan Beulich wrote:
>> On 04.09.2025 23:51, 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.
>>>
>>> Switched to breaking out of the loop.
>>>
>>> Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement.")
>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>> ---
>>> Noticed during code inspection.
>>
>> Well spotted, just that ...
>>
>>> --- a/xen/arch/x86/io_apic.c
>>> +++ b/xen/arch/x86/io_apic.c
>>> @@ -1715,7 +1715,7 @@ static bool io_apic_level_ack_pending(unsigned int irq)
>>>   
>>>           pin = entry->pin;
>>>           if (pin == -1)
>>> -            continue;
>>> +            break;
>>
>> ... we shouldn't terminate the loop here, but rather continue with the next
>> entry in the list (if any). Hence presumably why "continue" was used, without
>> achieving the intended effect.
> 
> Ok, makes sense.  Though after the sending the patch, I was wondering if 
> it was an unreachable condition, and we should not end up here with pin 
> == -1.

I can't exclude that, but other code (e.g. add_pin_to_irq()) also deal with
this case.

Jan