diff mbox series

[v6,04/33] hw/pci/pci: Factor out pci_bus_map_irqs() from pci_bus_irqs()

Message ID 20230109172347.1830-5-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series Consolidate PIIX south bridges | expand

Commit Message

Bernhard Beschow Jan. 9, 2023, 5:23 p.m. UTC
pci_bus_irqs() coupled together the assignment of pci_set_irq_fn and
pci_map_irq_fn to a PCI bus. This coupling gets in the way when the
pci_map_irq_fn is board-specific while the pci_set_irq_fn is device-
specific.

For example, both of QEMU's PIIX south bridge models have different
pci_map_irq_fn implementations which are board-specific rather than
device-specific. These implementations should therefore reside in board
code. The pci_set_irq_fn's, however, should stay in the device models
because they access memory internal to the model.

Factoring out pci_bus_map_irqs() from pci_bus_irqs() allows the
assignments to be decoupled, resolving the problem described above.

Note also how pci_vpb_realize() which gets touched in this commit
assigns different pci_map_irq_fn's depending on the board.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci/pci.h    |  3 ++-
 hw/i386/pc_q35.c        |  4 ++--
 hw/isa/piix3.c          |  8 ++++----
 hw/isa/piix4.c          |  3 ++-
 hw/pci-host/raven.c     |  3 ++-
 hw/pci-host/versatile.c |  3 ++-
 hw/pci/pci.c            | 12 +++++++++---
 hw/remote/machine.c     |  3 ++-
 8 files changed, 25 insertions(+), 14 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 13, 2023, 10:13 a.m. UTC | #1
On 9/1/23 18:23, Bernhard Beschow wrote:
> pci_bus_irqs() coupled together the assignment of pci_set_irq_fn and
> pci_map_irq_fn to a PCI bus. This coupling gets in the way when the
> pci_map_irq_fn is board-specific while the pci_set_irq_fn is device-
> specific.
> 
> For example, both of QEMU's PIIX south bridge models have different
> pci_map_irq_fn implementations which are board-specific rather than
> device-specific. These implementations should therefore reside in board
> code. The pci_set_irq_fn's, however, should stay in the device models
> because they access memory internal to the model.
> 
> Factoring out pci_bus_map_irqs() from pci_bus_irqs() allows the
> assignments to be decoupled, resolving the problem described above.
> 
> Note also how pci_vpb_realize() which gets touched in this commit
> assigns different pci_map_irq_fn's depending on the board.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/pci/pci.h    |  3 ++-
>   hw/i386/pc_q35.c        |  4 ++--
>   hw/isa/piix3.c          |  8 ++++----
>   hw/isa/piix4.c          |  3 ++-
>   hw/pci-host/raven.c     |  3 ++-
>   hw/pci-host/versatile.c |  3 ++-
>   hw/pci/pci.c            | 12 +++++++++---
>   hw/remote/machine.c     |  3 ++-
>   8 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 7048a373d1..85ee458cd2 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -282,8 +282,9 @@ PCIBus *pci_root_bus_new(DeviceState *parent, const char *name,
>                            MemoryRegion *address_space_io,
>                            uint8_t devfn_min, const char *typename);
>   void pci_root_bus_cleanup(PCIBus *bus);
> -void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> +void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
>                     void *irq_opaque, int nirq);
> +void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq);

I'm squashing:

-- >8 --
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index fe1fdfb5f7..46171f22f7 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -667,4 +667,4 @@ void vfu_object_set_bus_irq(PCIBus *pci_bus)

-    pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus,
-                 max_bdf);
+    pci_bus_irqs(pci_bus, vfu_object_set_irq, , pci_bus, max_bdf);
+    pci_bus_map_irqs(pci_bus, vfu_object_map_irq);
  }
---

to fix:

../hw/remote/vfio-user-obj.c: In function ‘vfu_object_set_bus_irq’:
../hw/remote/vfio-user-obj.c:668:67: error: passing argument 4 of 
‘pci_bus_irqs’ makes integer from pointer without a cast 
[-Werror=int-conversion]
      pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus,
                                                                    ^~~~~~~
In file included from include/hw/pci/pci_device.h:4,
                  from include/hw/remote/iohub.h:14,
                  from include/hw/remote/machine.h:18,
                  from ../hw/remote/vfio-user-obj.c:43:
include/hw/pci/pci.h:286:41: note: expected ‘int’ but argument is of 
type ‘PCIBus *’ {aka ‘struct PCIBus *’}
                    void *irq_opaque, int nirq);
                                      ~~~~^~~~
../hw/remote/vfio-user-obj.c:668:5: error: too many arguments to 
function ‘pci_bus_irqs’
      pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus,
      ^~~~~~~~~~~~
In file included from include/hw/pci/pci_device.h:4,
                  from include/hw/remote/iohub.h:14,
                  from include/hw/remote/machine.h:18,
                  from ../hw/remote/vfio-user-obj.c:43:
include/hw/pci/pci.h:285:6: note: declared here
  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
       ^~~~~~~~~~~~
Bernhard Beschow Jan. 13, 2023, 5:37 p.m. UTC | #2
Am 13. Januar 2023 10:13:29 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 9/1/23 18:23, Bernhard Beschow wrote:
>> pci_bus_irqs() coupled together the assignment of pci_set_irq_fn and
>> pci_map_irq_fn to a PCI bus. This coupling gets in the way when the
>> pci_map_irq_fn is board-specific while the pci_set_irq_fn is device-
>> specific.
>> 
>> For example, both of QEMU's PIIX south bridge models have different
>> pci_map_irq_fn implementations which are board-specific rather than
>> device-specific. These implementations should therefore reside in board
>> code. The pci_set_irq_fn's, however, should stay in the device models
>> because they access memory internal to the model.
>> 
>> Factoring out pci_bus_map_irqs() from pci_bus_irqs() allows the
>> assignments to be decoupled, resolving the problem described above.
>> 
>> Note also how pci_vpb_realize() which gets touched in this commit
>> assigns different pci_map_irq_fn's depending on the board.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/pci/pci.h    |  3 ++-
>>   hw/i386/pc_q35.c        |  4 ++--
>>   hw/isa/piix3.c          |  8 ++++----
>>   hw/isa/piix4.c          |  3 ++-
>>   hw/pci-host/raven.c     |  3 ++-
>>   hw/pci-host/versatile.c |  3 ++-
>>   hw/pci/pci.c            | 12 +++++++++---
>>   hw/remote/machine.c     |  3 ++-
>>   8 files changed, 25 insertions(+), 14 deletions(-)
>> 
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 7048a373d1..85ee458cd2 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -282,8 +282,9 @@ PCIBus *pci_root_bus_new(DeviceState *parent, const char *name,
>>                            MemoryRegion *address_space_io,
>>                            uint8_t devfn_min, const char *typename);
>>   void pci_root_bus_cleanup(PCIBus *bus);
>> -void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>> +void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
>>                     void *irq_opaque, int nirq);
>> +void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq);
>
>I'm squashing:
>
>-- >8 --
>diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>index fe1fdfb5f7..46171f22f7 100644
>--- a/hw/remote/vfio-user-obj.c
>+++ b/hw/remote/vfio-user-obj.c
>@@ -667,4 +667,4 @@ void vfu_object_set_bus_irq(PCIBus *pci_bus)
>
>-    pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus,
>-                 max_bdf);
>+    pci_bus_irqs(pci_bus, vfu_object_set_irq, , pci_bus, max_bdf);
>+    pci_bus_map_irqs(pci_bus, vfu_object_map_irq);
> }
>---
>
>to fix:
>
>../hw/remote/vfio-user-obj.c: In function ‘vfu_object_set_bus_irq’:
>../hw/remote/vfio-user-obj.c:668:67: error: passing argument 4 of ‘pci_bus_irqs’ makes integer from pointer without a cast [-Werror=int-conversion]
>     pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus,
>                                                                   ^~~~~~~
>In file included from include/hw/pci/pci_device.h:4,
>                 from include/hw/remote/iohub.h:14,
>                 from include/hw/remote/machine.h:18,
>                 from ../hw/remote/vfio-user-obj.c:43:
>include/hw/pci/pci.h:286:41: note: expected ‘int’ but argument is of type ‘PCIBus *’ {aka ‘struct PCIBus *’}
>                   void *irq_opaque, int nirq);
>                                     ~~~~^~~~
>../hw/remote/vfio-user-obj.c:668:5: error: too many arguments to function ‘pci_bus_irqs’
>     pci_bus_irqs(pci_bus, vfu_object_set_irq, vfu_object_map_irq, pci_bus,
>     ^~~~~~~~~~~~
>In file included from include/hw/pci/pci_device.h:4,
>                 from include/hw/remote/iohub.h:14,
>                 from include/hw/remote/machine.h:18,
>                 from ../hw/remote/vfio-user-obj.c:43:
>include/hw/pci/pci.h:285:6: note: declared here
> void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
>      ^~~~~~~~~~~~
>

Thanks!

I've missed enabling vfio-user-server for my builds. Fixed now.

Best regards,
Bernhard
diff mbox series

Patch

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 7048a373d1..85ee458cd2 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -282,8 +282,9 @@  PCIBus *pci_root_bus_new(DeviceState *parent, const char *name,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min, const char *typename);
 void pci_root_bus_cleanup(PCIBus *bus);
-void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
                   void *irq_opaque, int nirq);
+void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq);
 void pci_bus_irqs_cleanup(PCIBus *bus);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 67ceb04bcc..65ea226211 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -268,8 +268,8 @@  static void pc_q35_init(MachineState *machine)
     for (i = 0; i < GSI_NUM_PINS; i++) {
         qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
     }
-    pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc_map_irq, ich9_lpc,
-                 ICH9_LPC_NB_PIRQS);
+    pci_bus_irqs(host_bus, ich9_lpc_set_irq, ich9_lpc, ICH9_LPC_NB_PIRQS);
+    pci_bus_map_irqs(host_bus, ich9_lpc_map_irq);
     pci_bus_set_route_irq_fn(host_bus, ich9_route_intx_pin_to_irq);
     isa_bus = ich9_lpc->isa_bus;
 
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index eabad7ba58..666e794f77 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -384,8 +384,8 @@  static void piix3_realize(PCIDevice *dev, Error **errp)
         return;
     }
 
-    pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
-                 piix3, PIIX_NUM_PIRQS);
+    pci_bus_irqs(pci_bus, piix3_set_irq, piix3, PIIX_NUM_PIRQS);
+    pci_bus_map_irqs(pci_bus, pci_slot_get_pirq);
     pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
 }
 
@@ -420,8 +420,8 @@  static void piix3_xen_realize(PCIDevice *dev, Error **errp)
      * connected to the IOAPIC directly.
      * These additional routes can be discovered through ACPI.
      */
-    pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
-                 piix3, XEN_PIIX_NUM_PIRQS);
+    pci_bus_irqs(pci_bus, xen_piix3_set_irq, piix3, XEN_PIIX_NUM_PIRQS);
+    pci_bus_map_irqs(pci_bus, xen_pci_slot_get_pirq);
 }
 
 static void piix3_xen_class_init(ObjectClass *klass, void *data)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0d23e11a39..9c79c9677b 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -271,7 +271,8 @@  static void piix4_realize(PCIDevice *dev, Error **errp)
     }
     qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
 
-    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
+    pci_bus_irqs(pci_bus, piix4_set_irq, s, PIIX_NUM_PIRQS);
+    pci_bus_map_irqs(pci_bus, pci_slot_get_pirq);
 }
 
 static void piix4_init(Object *obj)
diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 2c96ddf8fe..5b00b4e462 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -258,7 +258,8 @@  static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 
     qdev_init_gpio_in(d, raven_change_gpio, 1);
 
-    pci_bus_irqs(&s->pci_bus, raven_set_irq, raven_map_irq, s, PCI_NUM_PINS);
+    pci_bus_irqs(&s->pci_bus, raven_set_irq, s, PCI_NUM_PINS);
+    pci_bus_map_irqs(&s->pci_bus, raven_map_irq);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
                           "pci-conf-idx", 4);
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 0d50ea4cc0..60d4e7cd92 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -422,7 +422,8 @@  static void pci_vpb_realize(DeviceState *dev, Error **errp)
         mapfn = pci_vpb_map_irq;
     }
 
-    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
+    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, s->irq, 4);
+    pci_bus_map_irqs(&s->pci_bus, mapfn);
 
     /* Our memory regions are:
      * 0 : our control registers
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c2fb88f9a3..39a7bb32aa 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -280,6 +280,7 @@  static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
     PCIBus *bus;
     for (;;) {
         bus = pci_get_bus(pci_dev);
+        assert(bus->map_irq);
         irq_num = bus->map_irq(pci_dev, irq_num);
         if (bus->set_irq)
             break;
@@ -518,16 +519,20 @@  void pci_root_bus_cleanup(PCIBus *bus)
     qbus_unrealize(BUS(bus));
 }
 
-void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
                   void *irq_opaque, int nirq)
 {
     bus->set_irq = set_irq;
-    bus->map_irq = map_irq;
     bus->irq_opaque = irq_opaque;
     bus->nirq = nirq;
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
+void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq)
+{
+    bus->map_irq = map_irq;
+}
+
 void pci_bus_irqs_cleanup(PCIBus *bus)
 {
     bus->set_irq = NULL;
@@ -549,7 +554,8 @@  PCIBus *pci_register_root_bus(DeviceState *parent, const char *name,
 
     bus = pci_root_bus_new(parent, name, address_space_mem,
                            address_space_io, devfn_min, typename);
-    pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
+    pci_bus_irqs(bus, set_irq, irq_opaque, nirq);
+    pci_bus_map_irqs(bus, map_irq);
     return bus;
 }
 
diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index 75d550daae..519f855ec1 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -63,8 +63,9 @@  static void remote_machine_init(MachineState *machine)
     } else {
         remote_iohub_init(&s->iohub);
 
-        pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
+        pci_bus_irqs(pci_host->bus, remote_iohub_set_irq,
                      &s->iohub, REMOTE_IOHUB_NB_PIRQS);
+        pci_bus_map_irqs(pci_host->bus, remote_iohub_map_irq);
     }
 
     qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));