Skip to contentSkip to conversation

pci: Don't call pci_irq_handler() for a negative intx

PATCH In review Benjamin Herrenschmidt 2 participants 3 messages Jul 4, 2015 – Jul 5, 2015

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 pci: Don't call pci_irq_handler() for a negative intx

Under some circumstances, pci_intx() can return -1 (when the interrupt pin in the config space is 0 which normally means no interrupt).

I have seen cases of pci_set_irq() being called on such devices, in turn causing pci_irq_handler() to be called with "-1" as an argument which doesn't seem like a terribly good idea.

---
 hw/pci/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
hw/pci/pci.c +3 -1
index 8185bbc..eea6f5d 100644
@@ -1281,7 +1281,9 @@ qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
 void pci_set_irq(PCIDevice *pci_dev, int level)
 {
     int intx = pci_intx(pci_dev);
-    pci_irq_handler(pci_dev, intx, level);
+    if (intx >= 0) {
+        pci_irq_handler(pci_dev, intx, level);
+    }
 }
 
 /* Special hooks used by device assignment */
#1 lore raw
Quoted 8 lines · Benjamin Herrenschmidt

Under some circumstances, pci_intx() can return -1 (when the interrupt pin in the config space is 0 which normally means no interrupt).

I have seen cases of pci_set_irq() being called on such devices, in turn causing pci_irq_handler() to be called with "-1" as an argument which doesn't seem like a terribly good idea.

Isn't this a device bug though?

I did a grep over all callers of pci_set_irq and didn't find any that fails to set an interrupt pin.

So how about an assert instead?

And maybe stick it in pci_intx to make sure all callers get checked.

Quoted 20 lines
---
 hw/pci/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
hw/pci/pci.c +3 -1
index 8185bbc..eea6f5d 100644
@@ -1281,7 +1281,9 @@ qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
 void pci_set_irq(PCIDevice *pci_dev, int level)
 {
     int intx = pci_intx(pci_dev);
-    pci_irq_handler(pci_dev, intx, level);
+    if (intx >= 0) {
+        pci_irq_handler(pci_dev, intx, level);
+    }
 }
 
 /* Special hooks used by device assignment */
#2 lore raw
Quoted 11 lines · Michael S. Tsirkin
Quoted 8 lines · Benjamin Herrenschmidt

Under some circumstances, pci_intx() can return -1 (when the interrupt pin in the config space is 0 which normally means no interrupt).

I have seen cases of pci_set_irq() being called on such devices, in turn causing pci_irq_handler() to be called with "-1" as an argument which doesn't seem like a terribly good idea.

Isn't this a device bug though?

Possibly, I can try to dig a bit more see if I can reproduce and find out who is causing it.

Quoted 7 lines

I did a grep over all callers of pci_set_irq and didn't find any that fails to set an interrupt pin.

So how about an assert instead?

And maybe stick it in pci_intx to make sure all callers get checked.

Ok, It's also possible that this doesn't happen anymore, I've carried that patch for monthes and rebased several times on top of newer qemu's.

I *think* it might have been something that happens due to some generic code initializations, something like pci_update_irq_disabled() in pci_default_write_config()... I'll dbl check.

Cheers,
Ben.