[RFC PATCH 00/18] PCI: convert IRQs to use qdev gpios

[RFC PATCH 00/18] PCI: convert IRQs to use qdev gpios

[Mark Cave-Ayland]

This series is something I've been playing with for a while, and it came up in
again in a conversation with Phil and Alex when discussing modelling of buses
and IRQs for heterogenerous binaries. The basic premise of the series is that
it converts PCI devices IRQs to use a qdev out gpio so that PCI devices can
potentially be wired up using standard qdev APIs.

In its current form the series adds a qdev out gpio to PCIDevice, adds a set of
input IRQs to PCIBus (once for each devfn) and wires them up at the very end of
pci_qdev_realize() once the device has been realised. This allows pci_set_irq()
to be changed into a simple wrapper over qemu_set_irq(), and the resulting
series passes GitLab CI to help prove the basic concept.

Note that this series is only concerned with providing a standard qdev gpio for
the PCI device IRQ, and not with how the PCI bus itself is modelled - that is
a discussion to be left for another day.

Another advantage of using qdev gpios is that it becomes possible to remove the
pci_allocate_irq() function which has long been a source of memory leaks. For
now I've added a new qdev named input gpio "pci-input-irq" which is used as its
replacement.

If everyone is happy that this series is going in the right direction then I'd
be inclined to add the qemu_irq and qdev gpio out to each individual PCI device
rather than using PCIDevice, and replace calls to pci_set_irq() with the
corresponding qemu_set_irq(). This would allow the "pci-input-irq" input gpio
to be dropped completely, and so PCIDevice IRQs can be treated like those of
any other qdev device (but at the cost of making this a larger series).

Thoughts/suggestions/comments?

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (18):
  hw/pci: add device IRQ to PCIDevice
  hw/pci: introduce PCI bus input IRQs
  hw/pci: use PCIDevice gpio for device IRQ
  hw/pci: introduce PCI device input gpio
  hw/char/serial-pci.c: switch SerialState to use PCI device input gpio
  hw/ide/ich.c: switch AHCIState to use PCI device input gpio
  hw/net/can/can_mioe3680_pci.c: switch Mioe3680PCIState to use PCI
    device input gpio
  hw/net/can/can_pcm3680_pci.c: switch SerialState to use PCI device
    input gpio
  hw/net/can/ctucan_pci.c: switch CtuCanPCIState to use PCI device input
    gpio
  hw/net/ne2000-pci.c: switch NE2000State to use PCI device input gpio
  hw/net/pcnet-pci.c: switch PCIPCNetState to use PCI device input gpio
  hw/net/tulip.c: switch TULIPState to use PCI device input gpio
  hw/scsi/esp-pci.c: switch ESPState to use PCI device input gpio
  hw/sd/sdhci-pci.c: switch SDHCIState to use PCI device input gpio
  hw/usb/hcd-ehci-pci.c: switch EHCIState to use PCI device input gpio
  hw/usb/hcd-ohci-pci.c: switch OHCIState to use PCI device input gpio
  hw/usb/hcd-uhci.c: switch UHCIState to use PCI device input gpio
  hw/pci/pci.c: remove pci_allocate_irq()

 hw/char/serial-pci.c          |  3 +-
 hw/ide/ich.c                  |  3 +-
 hw/net/can/can_mioe3680_pci.c |  4 +--
 hw/net/can/can_pcm3680_pci.c  |  4 +--
 hw/net/can/ctucan_pci.c       |  4 +--
 hw/net/ne2000-pci.c           |  3 +-
 hw/net/pcnet-pci.c            |  3 +-
 hw/net/tulip.c                |  3 +-
 hw/pci/pci.c                  | 65 +++++++++++++++++++++++++++++++----
 hw/scsi/esp-pci.c             | 11 +-----
 hw/sd/sdhci-pci.c             |  2 +-
 hw/usb/hcd-ehci-pci.c         |  3 +-
 hw/usb/hcd-ohci-pci.c         |  2 +-
 hw/usb/hcd-uhci.c             |  2 +-
 include/hw/pci/pci.h          |  1 -
 include/hw/pci/pci_bus.h      |  3 ++
 include/hw/pci/pci_device.h   |  3 ++
 17 files changed, 78 insertions(+), 41 deletions(-)

-- 
2.30.2

[RFC PATCH 01/18] hw/pci: add device IRQ to PCIDevice

[Mark Cave-Ayland]

Introduce pci_device_init() and use it to initialise an output IRQ.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c                | 8 ++++++++
 include/hw/pci/pci_device.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8a87ccc8b0..f29ac20895 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2161,6 +2161,13 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
     pci_dev->msi_trigger = pci_msi_trigger;
 }
 
+static void pci_device_init(Object *obj)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
+
+    qdev_init_gpio_out(DEVICE(obj), &pci_dev->irq, 1);
+}
+
 PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
                                  const char *name)
 {
@@ -2812,6 +2819,7 @@ void pci_set_power(PCIDevice *d, bool state)
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
+    .instance_init = pci_device_init,
     .instance_size = sizeof(PCIDevice),
     .abstract = true,
     .class_size = sizeof(PCIDeviceClass),
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index d3dd0f64b2..2c6dca5d15 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -149,6 +149,9 @@ struct PCIDevice {
     MemoryRegion rom;
     uint32_t rom_bar;
 
+    /* PCI IRQ */
+    qemu_irq irq;
+
     /* INTx routing notifier */
     PCIINTxRoutingNotifier intx_routing_notifier;
 
-- 
2.30.2

[RFC PATCH 02/18] hw/pci: introduce PCI bus input IRQs

[Mark Cave-Ayland]

Introduce a new pci_bus_irq_handler input IRQ handler that can be triggered by
each unique PCI devfn on the bus. Add a new pci_bus_init() instance init
function to initialise the IRQ array and a corresponding pci_bus_finalize()
instance finalize function to free it.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c             | 34 ++++++++++++++++++++++++++++++++++
 include/hw/pci/pci_bus.h |  3 +++
 2 files changed, 37 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index f29ac20895..9471f996a7 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -179,6 +179,24 @@ static void pci_bus_unrealize(BusState *qbus)
     vmstate_unregister(NULL, &vmstate_pcibus, bus);
 }
 
+static void pci_bus_irq_handler(void *opaque, int devfn, int level);
+
+static void pci_bus_init(Object *obj)
+{
+    PCIBus *bus = PCI_BUS(obj);
+
+    /* IRQs */
+    bus->irq_in = qemu_allocate_irqs(pci_bus_irq_handler, bus,
+                                     PCI_SLOT_MAX * PCI_FUNC_MAX);
+}
+
+static void pci_bus_finalize(Object *obj)
+{
+    PCIBus *bus = PCI_BUS(obj);
+
+    qemu_free_irqs(bus->irq_in, PCI_SLOT_MAX * PCI_FUNC_MAX);
+}
+
 static int pcibus_num(PCIBus *bus)
 {
     if (pci_bus_is_root(bus)) {
@@ -211,7 +229,9 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
 static const TypeInfo pci_bus_info = {
     .name = TYPE_PCI_BUS,
     .parent = TYPE_BUS,
+    .instance_init = pci_bus_init,
     .instance_size = sizeof(PCIBus),
+    .instance_finalize = pci_bus_finalize,
     .class_size = sizeof(PCIBusClass),
     .class_init = pci_bus_class_init,
 };
@@ -1636,6 +1656,20 @@ static void pci_irq_handler(void *opaque, int irq_num, int level)
     pci_change_irq_level(pci_dev, irq_num, change);
 }
 
+/* Bus IRQ handler */
+static void pci_bus_irq_handler(void *opaque, int devfn, int level)
+{
+    PCIBus *bus = PCI_BUS(opaque);
+    PCIDevice *pci_dev;
+    int intx;
+
+    assert(0 <= devfn && devfn < PCI_SLOT_MAX * PCI_FUNC_MAX);
+    pci_dev = bus->devices[devfn];
+    intx = pci_intx(pci_dev);
+
+    pci_irq_handler(pci_dev, intx, level);
+}
+
 qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
 {
     int intx = pci_intx(pci_dev);
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5653175957..8486df283e 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -54,6 +54,9 @@ struct PCIBus {
     int nirq;
     int *irq_count;
 
+    /* Bus IRQ handler */
+    qemu_irq *irq_in;
+
     Notifier machine_done;
 };
 
-- 
2.30.2

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

[Mark Cave-Ayland]

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]);
 }
 
 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,
-- 
2.30.2

[RFC PATCH 04/18] hw/pci: introduce PCI device input gpio

[Mark Cave-Ayland]

This is to allow other devices to drive the PCI device IRQ if required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 3da1481eb5..0dd3b24b62 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1683,6 +1683,13 @@ void pci_set_irq(PCIDevice *pci_dev, int level)
     qemu_set_irq(pci_dev->irq, level);
 }
 
+static void pci_device_input_irq_handler(void *opaque, int n, int level)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(opaque);
+
+    pci_set_irq(pci_dev, level);
+}
+
 /* Special hooks used by device assignment */
 void pci_bus_set_route_irq_fn(PCIBus *bus, pci_route_irq_fn route_intx_to_irq)
 {
@@ -2203,6 +2210,8 @@ static void pci_device_init(Object *obj)
     PCIDevice *pci_dev = PCI_DEVICE(obj);
 
     qdev_init_gpio_out(DEVICE(obj), &pci_dev->irq, 1);
+    qdev_init_gpio_in_named(DEVICE(obj), pci_device_input_irq_handler,
+                            "pci-input-irq", 1);
 }
 
 PCIDevice *pci_new_multifunction(int devfn, bool multifunction,
@@ -2856,6 +2865,8 @@ void pci_set_power(PCIDevice *d, bool state)
 /*
  * QEMU interface:
  * + Unnamed GPIO output: set to 1 if the PCI Device has asserted its irq
+ * + Named GPIO input "pci-input-irq": set to 1 if a downstream device
+ *   wishes to assert the PCI Device irq directly itself (optional)
  */
 
 static const TypeInfo pci_device_type_info = {
-- 
2.30.2

[RFC PATCH 05/18] hw/char/serial-pci.c: switch SerialState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/char/serial-pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 087da3059a..cd7e8b5b94 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -55,7 +55,7 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp)
 
     pci->dev.config[PCI_CLASS_PROG] = pci->prog_if;
     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
-    s->irq = pci_allocate_irq(&pci->dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
 
     memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, "serial", 8);
     pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
@@ -67,7 +67,6 @@ static void serial_pci_exit(PCIDevice *dev)
     SerialState *s = &pci->state;
 
     qdev_unrealize(DEVICE(s));
-    qemu_free_irq(s->irq);
 }
 
 static const VMStateDescription vmstate_pci_serial = {
-- 
2.30.2

[RFC PATCH 06/18] hw/ide/ich.c: switch AHCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/ich.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index d61faab532..646de05663 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -123,7 +123,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     /* XXX Software should program this register */
     dev->config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
 
-    d->ahci.irq = pci_allocate_irq(dev);
+    d->ahci.irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
 
     pci_register_bar(dev, ICH9_IDP_BAR, PCI_BASE_ADDRESS_SPACE_IO,
                      &d->ahci.idp);
@@ -159,7 +159,6 @@ static void pci_ich9_uninit(PCIDevice *dev)
 
     msi_uninit(dev);
     ahci_uninit(&d->ahci);
-    qemu_free_irq(d->ahci.irq);
 }
 
 static void ich_ahci_class_init(ObjectClass *klass, void *data)
-- 
2.30.2

[RFC PATCH 07/18] hw/net/can/can_mioe3680_pci.c: switch Mioe3680PCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/can/can_mioe3680_pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/net/can/can_mioe3680_pci.c b/hw/net/can/can_mioe3680_pci.c
index b9918773b3..686bf16104 100644
--- a/hw/net/can/can_mioe3680_pci.c
+++ b/hw/net/can/can_mioe3680_pci.c
@@ -163,7 +163,7 @@ static void mioe3680_pci_realize(PCIDevice *pci_dev, Error **errp)
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
 
-    d->irq = pci_allocate_irq(&d->dev);
+    d->irq = qdev_get_gpio_in_named(DEVICE(pci_dev), "pci-input-irq", 0);
 
     for (i = 0 ; i < MIOe3680_PCI_SJA_COUNT; i++) {
         can_sja_init(&d->sja_state[i], d->irq);
@@ -195,8 +195,6 @@ static void mioe3680_pci_exit(PCIDevice *pci_dev)
     for (i = 0 ; i < MIOe3680_PCI_SJA_COUNT; i++) {
         can_sja_disconnect(&d->sja_state[i]);
     }
-
-    qemu_free_irq(d->irq);
 }
 
 static const VMStateDescription vmstate_mioe3680_pci = {
-- 
2.30.2

[RFC PATCH 08/18] hw/net/can/can_pcm3680_pci.c: switch SerialState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/can/can_pcm3680_pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/net/can/can_pcm3680_pci.c b/hw/net/can/can_pcm3680_pci.c
index 8ef3e4659c..50f01d02bd 100644
--- a/hw/net/can/can_pcm3680_pci.c
+++ b/hw/net/can/can_pcm3680_pci.c
@@ -163,7 +163,7 @@ static void pcm3680i_pci_realize(PCIDevice *pci_dev, Error **errp)
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
 
-    d->irq = pci_allocate_irq(&d->dev);
+    d->irq = qdev_get_gpio_in_named(DEVICE(pci_dev), "pci-input-irq", 0);
 
     for (i = 0; i < PCM3680i_PCI_SJA_COUNT; i++) {
         can_sja_init(&d->sja_state[i], d->irq);
@@ -196,8 +196,6 @@ static void pcm3680i_pci_exit(PCIDevice *pci_dev)
     for (i = 0; i < PCM3680i_PCI_SJA_COUNT; i++) {
         can_sja_disconnect(&d->sja_state[i]);
     }
-
-    qemu_free_irq(d->irq);
 }
 
 static const VMStateDescription vmstate_pcm3680i_pci = {
-- 
2.30.2

[RFC PATCH 09/18] hw/net/can/ctucan_pci.c: switch CtuCanPCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/can/ctucan_pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/net/can/ctucan_pci.c b/hw/net/can/ctucan_pci.c
index ea079e2af5..94f8f58518 100644
--- a/hw/net/can/ctucan_pci.c
+++ b/hw/net/can/ctucan_pci.c
@@ -173,7 +173,7 @@ static void ctucan_pci_realize(PCIDevice *pci_dev, Error **errp)
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
 
-    d->irq = pci_allocate_irq(&d->dev);
+    d->irq = qdev_get_gpio_in_named(DEVICE(pci_dev), "pci-input-irq", 0);
 
     for (i = 0 ; i < CTUCAN_PCI_CORE_COUNT; i++) {
         ctucan_init(&d->ctucan_state[i], d->irq);
@@ -207,8 +207,6 @@ static void ctucan_pci_exit(PCIDevice *pci_dev)
     for (i = 0 ; i < CTUCAN_PCI_CORE_COUNT; i++) {
         ctucan_disconnect(&d->ctucan_state[i]);
     }
-
-    qemu_free_irq(d->irq);
 }
 
 static const VMStateDescription vmstate_ctucan_pci = {
-- 
2.30.2

[RFC PATCH 10/18] hw/net/ne2000-pci.c: switch NE2000State to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/ne2000-pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/ne2000-pci.c b/hw/net/ne2000-pci.c
index edc6689d33..27bdb3e11b 100644
--- a/hw/net/ne2000-pci.c
+++ b/hw/net/ne2000-pci.c
@@ -64,7 +64,7 @@ static void pci_ne2000_realize(PCIDevice *pci_dev, Error **errp)
     s = &d->ne2000;
     ne2000_setup_io(s, DEVICE(pci_dev), 0x100);
     pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
-    s->irq = pci_allocate_irq(&d->dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(pci_dev), "pci-input-irq", 0);
 
     qemu_macaddr_default_if_unset(&s->c.macaddr);
     ne2000_reset(s);
@@ -81,7 +81,6 @@ static void pci_ne2000_exit(PCIDevice *pci_dev)
     NE2000State *s = &d->ne2000;
 
     qemu_del_nic(s->nic);
-    qemu_free_irq(s->irq);
 }
 
 static void ne2000_instance_init(Object *obj)
-- 
2.30.2

[RFC PATCH 11/18] hw/net/pcnet-pci.c: switch PCIPCNetState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/pcnet-pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 96a302c141..cd2e080b4f 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -182,7 +182,6 @@ static void pci_pcnet_uninit(PCIDevice *dev)
 {
     PCIPCNetState *d = PCI_PCNET(dev);
 
-    qemu_free_irq(d->state.irq);
     timer_free(d->state.poll_timer);
     qemu_del_nic(d->state.nic);
 }
@@ -227,7 +226,7 @@ static void pci_pcnet_realize(PCIDevice *pci_dev, Error **errp)
 
     pci_register_bar(pci_dev, 1, 0, &s->mmio);
 
-    s->irq = pci_allocate_irq(pci_dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(pci_dev), "pci-input-irq", 0);
     s->phys_mem_read = pci_physical_memory_read;
     s->phys_mem_write = pci_physical_memory_write;
     s->dma_opaque = DEVICE(pci_dev);
-- 
2.30.2

[RFC PATCH 12/18] hw/net/tulip.c: switch TULIPState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/tulip.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index 915e5fb595..0b4bd8af51 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -979,7 +979,7 @@ static void pci_tulip_realize(PCIDevice *pci_dev, Error **errp)
     pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
     pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->memory);
 
-    s->irq = pci_allocate_irq(&s->dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(pci_dev), "pci-input-irq", 0);
 
     s->nic = qemu_new_nic(&net_tulip_info, &s->c,
                           object_get_typename(OBJECT(pci_dev)),
@@ -992,7 +992,6 @@ static void pci_tulip_exit(PCIDevice *pci_dev)
     TULIPState *s = DO_UPCAST(TULIPState, dev, pci_dev);
 
     qemu_del_nic(s->nic);
-    qemu_free_irq(s->irq);
     eeprom93xx_free(&pci_dev->qdev, s->eeprom);
 }
 
-- 
2.30.2

[RFC PATCH 13/18] hw/scsi/esp-pci.c: switch ESPState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed. Since esp_pci_scsi_exit() is now empty it can also
be removed at the same time.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp-pci.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 2f7f11e70b..9447e6c429 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -386,19 +386,11 @@ static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp)
                           "esp-io", 0x80);
 
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io);
-    s->irq = pci_allocate_irq(dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info);
 }
 
-static void esp_pci_scsi_exit(PCIDevice *d)
-{
-    PCIESPState *pci = PCI_ESP(d);
-    ESPState *s = ESP(&pci->esp);
-
-    qemu_free_irq(s->irq);
-}
-
 static void esp_pci_init(Object *obj)
 {
     PCIESPState *pci = PCI_ESP(obj);
@@ -412,7 +404,6 @@ static void esp_pci_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->realize = esp_pci_scsi_realize;
-    k->exit = esp_pci_scsi_exit;
     k->vendor_id = PCI_VENDOR_ID_AMD;
     k->device_id = PCI_DEVICE_ID_AMD_SCSI;
     k->revision = 0x10;
-- 
2.30.2

[RFC PATCH 14/18] hw/sd/sdhci-pci.c: switch SDHCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sd/sdhci-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
index c737c8b930..d2bb23c3f2 100644
--- a/hw/sd/sdhci-pci.c
+++ b/hw/sd/sdhci-pci.c
@@ -40,7 +40,7 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 
     dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
-    s->irq = pci_allocate_irq(dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
     s->dma_as = pci_get_address_space(dev);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->iomem);
 }
-- 
2.30.2

[RFC PATCH 15/18] hw/usb/hcd-ehci-pci.c: switch EHCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/usb/hcd-ehci-pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 4c37c8e227..eed98799b7 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -65,7 +65,7 @@ static void usb_ehci_pci_realize(PCIDevice *dev, Error **errp)
     pci_conf[0x6e] = 0x00;
     pci_conf[0x6f] = 0xc0;  /* USBLEFCTLSTS */
 
-    s->irq = pci_allocate_irq(dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
     s->as = pci_get_address_space(dev);
 
     usb_ehci_realize(s, DEVICE(dev), NULL);
@@ -107,7 +107,6 @@ static void usb_ehci_pci_exit(PCIDevice *dev)
 
     usb_ehci_unrealize(s, DEVICE(dev));
 
-    g_free(s->irq);
     s->irq = NULL;
 }
 
-- 
2.30.2

[RFC PATCH 16/18] hw/usb/hcd-ohci-pci.c: switch OHCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq() and also allows the corresponding
qemu_free_irq() to be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/usb/hcd-ohci-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci-pci.c b/hw/usb/hcd-ohci-pci.c
index 6b630d35a7..35e4be2a62 100644
--- a/hw/usb/hcd-ohci-pci.c
+++ b/hw/usb/hcd-ohci-pci.c
@@ -75,7 +75,7 @@ static void usb_ohci_realize_pci(PCIDevice *dev, Error **errp)
         return;
     }
 
-    ohci->state.irq = pci_allocate_irq(dev);
+    ohci->state.irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
     pci_register_bar(dev, 0, 0, &ohci->state.mem);
 }
 
-- 
2.30.2

[RFC PATCH 17/18] hw/usb/hcd-uhci.c: switch UHCIState to use PCI device input gpio

[Mark Cave-Ayland]

This replaces the call to pci_allocate_irq().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/usb/hcd-uhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 77baaa7a6b..0b08dadd1b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1168,7 +1168,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
     /* TODO: reset value should be 0. */
     pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
     pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
-    s->irq = pci_allocate_irq(dev);
+    s->irq = qdev_get_gpio_in_named(DEVICE(dev), "pci-input-irq", 0);
 
     if (s->masterbus) {
         USBPort *ports[NB_PORTS];
-- 
2.30.2

[RFC PATCH 18/18] hw/pci/pci.c: remove pci_allocate_irq()

[Mark Cave-Ayland]

This function is no longer used and can now be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c         | 8 --------
 include/hw/pci/pci.h | 1 -
 2 files changed, 9 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0dd3b24b62..79eb427709 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1670,14 +1670,6 @@ static void pci_bus_irq_handler(void *opaque, int devfn, int level)
     pci_irq_handler(pci_dev, intx, level);
 }
 
-qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
-{
-    int intx = pci_intx(pci_dev);
-    assert(0 <= intx && intx < PCI_NUM_PINS);
-
-    return qemu_allocate_irq(pci_irq_handler, pci_dev, intx);
-}
-
 void pci_set_irq(PCIDevice *pci_dev, int level)
 {
     qemu_set_irq(pci_dev->irq, level);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 935b4b91b4..080c5a0df2 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -587,7 +587,6 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
 void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev);
 
-qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
 void pci_set_irq(PCIDevice *pci_dev, int level);
 
 static inline void pci_irq_assert(PCIDevice *pci_dev)
-- 
2.30.2

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

[Bernhard Beschow]

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,

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

[Michael S. Tsirkin]

On Thu, May 11, 2023 at 09:44:51PM +0000, Bernhard Beschow wrote:
> 
> 
> 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.

True, interrupt lines (not lanes I think - lanes is a PCI express
unrelated to interrupts since interrupts are just messages under PCIe)
bypass the PCI bus. They are in fact even used outside the
normal GNT#/REQ# protocol.

	The system vendor is free to combine the various INTx# signals from the PCI connector(s)
	in any way to connect them to the interrupt controller. They may be wire-ORed or
	electronically switched under program control, or any combination thereof. The system
	designer must insure that each INTx# signal from each connector is connected to an input
	on the interrupt controller. This means the device driver may not make any assumptions
	about interrupt sharing. All PCI device drivers must be able to share an interrupt (chaining)
	with any other logical device including devices in the same multi-function package.

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

the swizzeling always applies in case of PCI bridges:

However, since bridges will be used on add-in cards, the BIOS will assume an association
between device location and which INTx# line it uses when requesting an interrupt.
...
The BIOS code will assume the following binding behind the bridge and will
write the IRQ number in each device as described in Table 9-1. The interrupt binding
defined in this table is mandatory for add-in cards utilizing a bridge.





> 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,

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

[Mark Cave-Ayland]

On 11/05/2023 22:44, Bernhard Beschow wrote:

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

That's definitely true: the restriction here is caused by the fact that QEMU's PCI 
IRQ routing is already deeply integrated into the PCIBus object. This is visible by 
the fact that pci_bus_map_irqs() is used to set the IRQ swizzling for each PCIBus so 
I don't see there is a way to untangle without a massive redesign of the PCI buses in 
QEMU, which is certainly outside the scope of this series.

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

Another restriction on the current QEMU design of PCI devices is that there is an 
implicit assumption that there is only one IRQ i.e. it's the current INTX rather than 
representing the 4 separate PIRQ lines. Again this is something that people have 
expressed interest in resolving, but as soon as you get here you end up with the same 
problem above in that you need to revisit the PCI IRQ swizzling code above.

In particular there are some interesting use cases such as SPARC64 sabre whereby we 
use virtual interrupt numbers during IRQ swizzling because we lose the PCIDevice of 
the originating device as the IRQ propagates upwards through PCI bridges. So that 
would also need a redesign assuming we move towards a more physical PCI model.

For the moment the PCI input IRQs are stored within PCIBus to ensure the existing 
interrupt code works without having to touch any IRQ swizzling code, and so as the 
current design assumes a single interrupt for each PCIDevice we only need a single 
IRQ for each devfn. That's why there is no overflow of the array, and I can confirm 
the code passed both gitlab CI and a local --enable-sanitizers "make check" without 
introducing any additional failures.

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

I hope the above explanation gives a bit more background as to why the series is 
structured in the way it is. In effect it makes no attempt to alter the existing 
PCIBus routing, but starts by considering that PCI devices have their own unique IRQ 
handling using pci_set_irq(). So let's take a baby step which is to convert PCI 
devices to use a standard qdev gpio instead of their own custom PCI IRQ handler, 
which then allows for the possibility of using the standard qdev APIs for PCI IRQ 
routing in future.


ATB,

Mark.

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

[Mark Cave-Ayland]

On 12/05/2023 06:51, Michael S. Tsirkin wrote:

> On Thu, May 11, 2023 at 09:44:51PM +0000, Bernhard Beschow wrote:
>>
>>
>> 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.
> 
> True, interrupt lines (not lanes I think - lanes is a PCI express
> unrelated to interrupts since interrupts are just messages under PCIe)
> bypass the PCI bus. They are in fact even used outside the
> normal GNT#/REQ# protocol.
> 
> 	The system vendor is free to combine the various INTx# signals from the PCI connector(s)
> 	in any way to connect them to the interrupt controller. They may be wire-ORed or
> 	electronically switched under program control, or any combination thereof. The system
> 	designer must insure that each INTx# signal from each connector is connected to an input
> 	on the interrupt controller. This means the device driver may not make any assumptions
> 	about interrupt sharing. All PCI device drivers must be able to share an interrupt (chaining)
> 	with any other logical device including devices in the same multi-function package.

I hope I covered this in my reply to Bernhard, but I think this supports the idea 
that using a gpio is the right approach here: in the case of PCI IRQ the gpio 
represents the physical signal and can be wired using standard qdev APIs, whereas for 
PCIe the GPIOs can be wired to an internal message handler in exactly the same way.

>> 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.
> 
> the swizzeling always applies in case of PCI bridges:
> 
> However, since bridges will be used on add-in cards, the BIOS will assume an association
> between device location and which INTx# line it uses when requesting an interrupt.
> ...
> The BIOS code will assume the following binding behind the bridge and will
> write the IRQ number in each device as described in Table 9-1. The interrupt binding
> defined in this table is mandatory for add-in cards utilizing a bridge.

Also just to clarify in line with my previous reply: this series only changes the PCI 
device IRQ endpoint to use a gpio as a starting point to facilitate using standard 
qdev APIs to enable physical PCI device routing in future, as opposed to using a 
bespoke pci_set_irq() function just for PCI.

If this makes sense, I'd be interested to hear whether you think the current approach 
of using a named gpio "pci-input-irq" (which appears in "info qom-tree") is suitable, 
or whether it makes sense to embed the PCI IRQ directly in the device which is the 
normal qdev approach but will take more work as it involves updating all the relevant 
PCIDevices.


ATB,

Mark.