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