Re: [RFC PATCH 03/18] hw/pci: use PCIDevice gpio for device IRQ

[Bernhard Beschow <shentey@gmail.com>]

Am 11. Mai 2023 08:57:16 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>Change pci_set_irq() to call qemu_set_irq() on the PCI device IRQ rather than
>calling PCI bus IRQ handler function directly. In order to preserve the
>existing behaviour update pci_qdev_realize() so that it automatically connects
>the PCI device IRQ to the PCI bus IRQ handler.
>
>Finally add a "QEMU interface" description documenting the new PCI device IRQ
>gpio next to the declaration of TYPE_PCI_DEVICE.
>
>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>---
> hw/pci/pci.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>index 9471f996a7..3da1481eb5 100644
>--- a/hw/pci/pci.c
>+++ b/hw/pci/pci.c
>@@ -1680,8 +1680,7 @@ 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);
>+    qemu_set_irq(pci_dev->irq, level);
> }
> 
> /* Special hooks used by device assignment */
>@@ -2193,6 +2192,10 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>     pci_set_power(pci_dev, true);
> 
>     pci_dev->msi_trigger = pci_msi_trigger;
>+
>+    /* Connect device IRQ to bus */
>+    qdev_connect_gpio_out(DEVICE(pci_dev), 0,
>+                          pci_get_bus(pci_dev)->irq_in[pci_dev->devfn]);

I think this is confusing a few things. In my understanding -- unlike ISA -- PCI considers interrupt lanes only for PCI slots but not for buses. So for example each PCI slot could have its own direct connections (up to four, intA..intD) to the interrupt controller. IOW interrupt lanes and PCI buses are unrelated, thus PCIBus shouldn't really have IRQs.

Moreover, in case the interrupt lines are shared between multiple PCI slots, a usual pattern is to swizzle these lines such that the intAs from the slots don't all occupy just one IRQ line. That means that depending on the slot the device is plugged into a different lane is triggered. Above code, however, would always trigger the same line and wouldn't even allow for modeling the swizzeling.

Also, above code would cause out of bounds array accesses if a PCI device had more functions than there are on "the bus":
For example, consider PIIX which has four PIRQs, so ARRAY_SIZE(irq_fn) == 4, right? devfn can be up to 8 according to the PCI spec which would cause an out if bounds array access above.

I think that this commit does actually re-define how PCI buses work in QEMU although the cover letter claims to save this for another day. We should probably not apply the series in its current form.

Best regards,
Bernhard

> }
> 
> static void pci_device_init(Object *obj)
>@@ -2850,6 +2853,11 @@ void pci_set_power(PCIDevice *d, bool state)
>     }
> }
> 
>+/*
>+ * QEMU interface:
>+ * + Unnamed GPIO output: set to 1 if the PCI Device has asserted its irq
>+ */
>+
> static const TypeInfo pci_device_type_info = {
>     .name = TYPE_PCI_DEVICE,
>     .parent = TYPE_DEVICE,