| Message ID | 20230109172347.1830-5-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Consolidate PIIX south bridges | expand |
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, ^~~~~~~~~~~~
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 --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));