pci_bus_new*() and pci_register_bus() work only when the 'parent'
argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
are meant to initialize a bus that's in a PCI host bridge.

The new function names are:
* pci_host_bus_init() (replacing pci_bus_new())
* pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
* pci_host_bus_init_irqs() (replacing pci_register_bus())

This also replaces the DeviceState *parent parameter on those functions
with a PCIHostState *phb parameter.

This was implemented using the following Coccinelle patch:

    // 1) Rename pci_bus_new():
    @@
    typedef PCIBus;
    typedef PCIHostState;
    typedef DeviceState;
    identifier parent;
    parameter list ARGS;
    @@
    -PCIBus *pci_bus_new(DeviceState *parent, ARGS);
    +PCIBus *pci_host_bus_init(PCIHostState *phb, ARGS);

    @@
    typedef PCIBus;
    typedef PCIHostState;
    identifier parent;
    parameter list ARGS;
    @@
    -PCIBus *pci_bus_new(DeviceState *parent, ARGS)
    +PCIBus *pci_host_bus_init(PCIHostState *phb, ARGS)
     {
     <...
    -parent
    +DEVICE(phb)
     ...>
     }

    @@
    expression parent;
    expression list ARGS;
    @@
    -pci_bus_new(parent, ARGS)
    +pci_host_bus_init(PCI_HOST_BRIDGE(parent), ARGS)

    // 2) Rename pci_bus_new_inplace():
    @@
    typedef PCIBus;
    typedef PCIHostState;
    typedef DeviceState;
    identifier bus, bus_size, parent;
    parameter list ARGS;
    @@
    -void pci_bus_new_inplace(PCIBus *bus, size_t bus_size,
    -                         DeviceState *parent, ARGS);
    +void pci_host_bus_init_inplace(PCIHostState *phb,
    +                               PCIBus *bus, size_t bus_size, ARGS);

    @@
    typedef PCIBus;
    typedef PCIHostState;
    typedef DeviceState;
    identifier bus, bus_size, parent;
    parameter list ARGS;
    @@
    -void pci_bus_new_inplace(PCIBus *bus, size_t bus_size,
    -                         DeviceState *parent, ARGS)
    +void pci_host_bus_init_inplace(PCIHostState *phb,
    +                               PCIBus *bus, size_t bus_size, ARGS)
     {
    -PCIHostState *phb = PCI_HOST_BRIDGE(parent);
     <...
    -parent
    +DEVICE(phb)
     ...>
     }

    @@
    expression bus, bus_size, parent;
    expression list ARGS;
    @@
    -pci_bus_new_inplace(bus, bus_size, parent, ARGS)
    +pci_host_bus_init_inplace(PCI_HOST_BRIDGE(parent), bus, bus_size, ARGS)

    // 3) Rename pci_register_bus():
    @@
    typedef PCIBus;
    typedef PCIHostState;
    identifier parent;
    parameter list ARGS;
    @@
    -PCIBus *pci_register_bus(DeviceState *parent, ARGS);
    +PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, ARGS);

    @@
    typedef PCIBus;
    typedef PCIHostState;
    identifier parent;
    parameter list ARGS;
    @@
    -PCIBus *pci_register_bus(DeviceState *parent, ARGS)
    +PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, ARGS)
     {
     <...
    -parent
    +DEVICE(phb)
     ...>
     }

    @@
    expression parent;
    expression list ARGS;
    @@
    -pci_register_bus(parent, ARGS)
    +pci_host_bus_init_irqs(PCI_HOST_BRIDGE(parent), ARGS)

    // 5) fix redundant casts on the resulting code:
    @@
    expression o;
    @@
    -PCI_HOST_BRIDGE(DEVICE(o))
    +PCI_HOST_BRIDGE(o)

    @@
    expression o;
    @@
    -DEVICE(PCI_HOST_BRIDGE(o))
    +DEVICE(o)

    @@
    idexpression PCIHostState *phb;
    @@
    -PCI_HOST_BRIDGE(phb)
    +phb

    @@
    idexpression PCIHostState *phb;
    expression dev;
    @@
     phb = PCI_HOST_BRIDGE(dev);
     <...
    -PCI_HOST_BRIDGE(dev)
    +phb
     ...>

    @@
    identifier phb;
    identifier dev;
    @@
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
     <...
    -PCI_HOST_BRIDGE(dev)
    +phb
     ...>

Cc: Alexander Graf <agraf@suse.de>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: "Hervé Poussineau" <hpoussin@reactos.org>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Yongbok Kim <yongbok.kim@imgtec.com>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/pci/pci.h                | 31 ++++++++++++-------------
 hw/alpha/typhoon.c                  |  7 +++---
 hw/mips/gt64xxx_pci.c               | 11 +++++----
 hw/pci-bridge/pci_expander_bridge.c |  6 +++--
 hw/pci-host/apb.c                   |  9 ++++----
 hw/pci-host/bonito.c                |  8 +++----
 hw/pci-host/gpex.c                  |  7 +++---
 hw/pci-host/grackle.c               | 11 ++++-----
 hw/pci-host/piix.c                  |  4 ++--
 hw/pci-host/ppce500.c               |  7 +++---
 hw/pci-host/prep.c                  |  5 +++--
 hw/pci-host/q35.c                   |  6 ++---
 hw/pci-host/uninorth.c              | 20 +++++++----------
 hw/pci-host/versatile.c             |  7 +++---
 hw/pci-host/xilinx-pcie.c           |  7 +++---
 hw/pci/pci.c                        | 45 +++++++++++++++++++------------------
 hw/ppc/ppc4xx_pci.c                 |  7 +++---
 hw/ppc/spapr_pci.c                  |  8 +++----
 hw/s390x/s390-pci-bus.c             |  8 +++----
 hw/sh4/sh_pci.c                     | 10 ++++-----
 20 files changed, 111 insertions(+), 113 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5cb6..85fe3ae743 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -393,26 +393,27 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 
 bool pci_bus_is_express(PCIBus *bus);
 bool pci_bus_is_root(PCIBus *bus);
-void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
-                         const char *name,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
-                         uint8_t devfn_min, const char *typename);
-PCIBus *pci_bus_new(DeviceState *parent, const char *name,
-                    MemoryRegion *address_space_mem,
-                    MemoryRegion *address_space_io,
-                    uint8_t devfn_min, const char *typename);
+void pci_host_bus_init_inplace(PCIHostState *phb, PCIBus *bus,
+                               size_t bus_size, const char *name,
+                               MemoryRegion *address_space_mem,
+                               MemoryRegion *address_space_io,
+                               uint8_t devfn_min, const char *typename);
+PCIBus *pci_host_bus_init(PCIHostState *phb, const char *name,
+                          MemoryRegion *address_space_mem,
+                          MemoryRegion *address_space_io, uint8_t devfn_min,
+                          const char *typename);
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
-PCIBus *pci_register_bus(DeviceState *parent, const char *name,
-                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
-                         uint8_t devfn_min, int nirq, const char *typename);
+PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, const char *name,
+                               pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+                               void *irq_opaque,
+                               MemoryRegion *address_space_mem,
+                               MemoryRegion *address_space_io,
+                               uint8_t devfn_min, int nirq,
+                               const char *typename);
 void pci_bus_set_route_irq_fn(PCIBus *, pci_route_irq_fn);
 PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin);
 bool pci_intx_route_changed(PCIINTxRoute *old, PCIINTxRoute *new);
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index f50f5cf186..24f1959ab8 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -883,10 +883,9 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
     memory_region_add_subregion(addr_space, 0x801fc000000ULL,
                                 &s->pchip.reg_io);
 
-    b = pci_register_bus(dev, "pci",
-                         typhoon_set_irq, sys_map_irq, s,
-                         &s->pchip.reg_mem, &s->pchip.reg_io,
-                         0, 64, TYPE_PCI_BUS);
+    b = pci_host_bus_init_irqs(phb, "pci", typhoon_set_irq,
+                               sys_map_irq, s, &s->pchip.reg_mem,
+                               &s->pchip.reg_io, 0, 64, TYPE_PCI_BUS);
     phb->bus = b;
     qdev_init_nofail(dev);
 
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 4811843ab6..5ca9f07031 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1171,12 +1171,11 @@ PCIBus *gt64120_register(qemu_irq *pic)
     phb = PCI_HOST_BRIDGE(dev);
     memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", UINT32_MAX);
     address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");
-    phb->bus = pci_register_bus(dev, "pci",
-                                gt64120_pci_set_irq, gt64120_pci_map_irq,
-                                pic,
-                                &d->pci0_mem,
-                                get_system_io(),
-                                PCI_DEVFN(18, 0), 4, TYPE_PCI_BUS);
+    phb->bus = pci_host_bus_init_irqs(phb, "pci",
+                                      gt64120_pci_set_irq,
+                                      gt64120_pci_map_irq, pic, &d->pci0_mem,
+                                      get_system_io(), PCI_DEVFN(18, 0), 4,
+                                      TYPE_PCI_BUS);
     qdev_init_nofail(dev);
     memory_region_init_io(&d->ISD_mem, OBJECT(dev), &isd_mem_ops, d, "isd-mem", 0x1000);
 
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 6ac187fa32..853448ef70 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -230,9 +230,11 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
 
     ds = qdev_create(NULL, TYPE_PXB_HOST);
     if (pcie) {
-        bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
+        bus = pci_host_bus_init(PCI_HOST_BRIDGE(ds), dev_name, NULL, NULL, 0,
+                                TYPE_PXB_PCIE_BUS);
     } else {
-        bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
+        bus = pci_host_bus_init(PCI_HOST_BRIDGE(ds), "pxb-internal", NULL,
+                                NULL, 0, TYPE_PXB_BUS);
         bds = qdev_create(BUS(bus), "pci-bridge");
         bds->id = dev_name;
         qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 653e711121..38d08c661c 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -671,11 +671,10 @@ PCIBus *pci_apb_init(hwaddr special_base,
     dev = qdev_create(NULL, TYPE_APB);
     d = APB_DEVICE(dev);
     phb = PCI_HOST_BRIDGE(dev);
-    phb->bus = pci_register_bus(DEVICE(phb), "pci",
-                                pci_apb_set_irq, pci_pbm_map_irq, d,
-                                &d->pci_mmio,
-                                get_system_io(),
-                                0, 32, TYPE_PCI_BUS);
+    phb->bus = pci_host_bus_init_irqs(phb, "pci",
+                                      pci_apb_set_irq, pci_pbm_map_irq, d,
+                                      &d->pci_mmio, get_system_io(), 0, 32,
+                                      TYPE_PCI_BUS);
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     /* apb_config */
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 1999ece590..8b6e80aa82 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -714,10 +714,10 @@ static int bonito_pcihost_initfn(SysBusDevice *dev)
 {
     PCIHostState *phb = PCI_HOST_BRIDGE(dev);
 
-    phb->bus = pci_register_bus(DEVICE(dev), "pci",
-                                pci_bonito_set_irq, pci_bonito_map_irq, dev,
-                                get_system_memory(), get_system_io(),
-                                0x28, 32, TYPE_PCI_BUS);
+    phb->bus = pci_host_bus_init_irqs(phb, "pci",
+                                      pci_bonito_set_irq, pci_bonito_map_irq,
+                                      dev, get_system_memory(),
+                                      get_system_io(), 0x28, 32, TYPE_PCI_BUS);
 
     return 0;
 }
diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
index 66055ee5cc..9ea9927cf8 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -62,9 +62,10 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(sbd, &s->irq[i]);
     }
 
-    pci->bus = pci_register_bus(dev, "pcie.0", gpex_set_irq,
-                                pci_swizzle_map_irq_fn, s, &s->io_mmio,
-                                &s->io_ioport, 0, 4, TYPE_PCIE_BUS);
+    pci->bus = pci_host_bus_init_irqs(pci, "pcie.0",
+                                      gpex_set_irq, pci_swizzle_map_irq_fn, s,
+                                      &s->io_mmio, &s->io_ioport, 0, 4,
+                                      TYPE_PCIE_BUS);
 
     qdev_set_parent_bus(DEVICE(&s->gpex_root), BUS(pci->bus));
     qdev_init_nofail(DEVICE(&s->gpex_root));
diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 2c8acdaaca..0844c30d9a 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -82,13 +82,10 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
     memory_region_add_subregion(address_space_mem, 0x80000000ULL,
                                 &d->pci_hole);
 
-    phb->bus = pci_register_bus(dev, NULL,
-                                pci_grackle_set_irq,
-                                pci_grackle_map_irq,
-                                pic,
-                                &d->pci_mmio,
-                                address_space_io,
-                                0, 4, TYPE_PCI_BUS);
+    phb->bus = pci_host_bus_init_irqs(phb, NULL,
+                                      pci_grackle_set_irq,
+                                      pci_grackle_map_irq, pic, &d->pci_mmio,
+                                      address_space_io, 0, 4, TYPE_PCI_BUS);
 
     pci_create_simple(phb->bus, 0, "grackle");
     qdev_init_nofail(dev);
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index f9218aa952..364d57039b 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -340,8 +340,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
 
     dev = qdev_create(NULL, host_type);
     s = PCI_HOST_BRIDGE(dev);
-    b = pci_bus_new(dev, NULL, pci_address_space,
-                    address_space_io, 0, TYPE_PCI_BUS);
+    b = pci_host_bus_init(s, NULL, pci_address_space,
+                          address_space_io, 0, TYPE_PCI_BUS);
     s->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
     qdev_init_nofail(dev);
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index e502bc0505..babb12af31 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -465,9 +465,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
     /* PIO lives at the bottom of our bus space */
     memory_region_add_subregion_overlap(&s->busmem, 0, &s->pio, -2);
 
-    b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
-                         mpc85xx_pci_map_irq, s, &s->busmem, &s->pio,
-                         PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
+    b = pci_host_bus_init_irqs(h, NULL,
+                               mpc85xx_pci_set_irq, mpc85xx_pci_map_irq, s,
+                               &s->busmem, &s->pio,
+                               PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
     h->bus = b;
 
     /* Set up PCI view of memory */
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 260a119a9e..65add936c7 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -269,8 +269,9 @@ static void raven_pcihost_initfn(Object *obj)
     memory_region_add_subregion_overlap(address_space_mem, 0x80000000,
                                         &s->pci_io_non_contiguous, 1);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
-                        &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
+    pci_host_bus_init_inplace(h, &s->pci_bus,
+                              sizeof(s->pci_bus), NULL, &s->pci_memory,
+                              &s->pci_io, 0, TYPE_PCI_BUS);
 
     /* Bus master address space */
     memory_region_init(&s->bm, obj, "bm-raven", UINT32_MAX);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 344f77b10c..947dc3f124 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -49,9 +49,9 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
     sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
     sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
 
-    pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
-                           s->mch.pci_address_space, s->mch.address_space_io,
-                           0, TYPE_PCIE_BUS);
+    pci->bus = pci_host_bus_init(PCI_HOST_BRIDGE(s), "pcie.0",
+                                 s->mch.pci_address_space,
+                                 s->mch.address_space_io, 0, TYPE_PCIE_BUS);
     PC_MACHINE(qdev_get_machine())->bus = pci->bus;
     qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));
     qdev_init_nofail(DEVICE(&s->mch));
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index df342ac3cb..079faad6ff 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -233,12 +233,10 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
     memory_region_add_subregion(address_space_mem, 0x80000000ULL,
                                 &d->pci_hole);
 
-    h->bus = pci_register_bus(dev, NULL,
-                              pci_unin_set_irq, pci_unin_map_irq,
-                              pic,
-                              &d->pci_mmio,
-                              address_space_io,
-                              PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
+    h->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
+                                    pci_unin_set_irq, pci_unin_map_irq, pic,
+                                    &d->pci_mmio, address_space_io,
+                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
 
 #if 0
     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north");
@@ -299,12 +297,10 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
     memory_region_add_subregion(address_space_mem, 0x80000000ULL,
                                 &d->pci_hole);
 
-    h->bus = pci_register_bus(dev, NULL,
-                              pci_unin_set_irq, pci_unin_map_irq,
-                              pic,
-                              &d->pci_mmio,
-                              address_space_io,
-                              PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
+    h->bus = pci_host_bus_init_irqs(h, NULL,
+                                    pci_unin_set_irq, pci_unin_map_irq, pic,
+                                    &d->pci_mmio, address_space_io,
+                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
 
     sysbus_mmio_map(s, 0, 0xf0800000);
     sysbus_mmio_map(s, 1, 0xf0c00000);
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 467cbb9cb8..cad8848723 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -386,9 +386,10 @@ static void pci_vpb_init(Object *obj)
     memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
     memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
 
-    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
-                        &s->pci_mem_space, &s->pci_io_space,
-                        PCI_DEVFN(11, 0), TYPE_PCI_BUS);
+    pci_host_bus_init_inplace(h, &s->pci_bus,
+                              sizeof(s->pci_bus), "pci", &s->pci_mem_space,
+                              &s->pci_io_space, PCI_DEVFN(11, 0),
+                              TYPE_PCI_BUS);
     h->bus = &s->pci_bus;
 
     object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_VERSATILE_PCI_HOST);
diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 8b71e2d950..0a3d19b22c 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -129,9 +129,10 @@ static void xilinx_pcie_host_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &pex->mmio);
     sysbus_init_mmio(sbd, &s->mmio);
 
-    pci->bus = pci_register_bus(dev, s->name, xilinx_pcie_set_irq,
-                                pci_swizzle_map_irq_fn, s, &s->mmio,
-                                &s->io, 0, 4, TYPE_PCIE_BUS);
+    pci->bus = pci_host_bus_init_irqs(pci, s->name,
+                                      xilinx_pcie_set_irq,
+                                      pci_swizzle_map_irq_fn, s, &s->mmio,
+                                      &s->io, 0, 4, TYPE_PCIE_BUS);
 
     qdev_set_parent_bus(DEVICE(&s->root), BUS(pci->bus));
     qdev_init_nofail(DEVICE(&s->root));
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0d28ee4e3f..37d65eaf7e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -367,15 +367,13 @@ bool pci_bus_is_root(PCIBus *bus)
     return PCI_BUS_GET_CLASS(bus)->is_root(bus);
 }
 
-void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
-                         const char *name,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
-                         uint8_t devfn_min, const char *typename)
+void pci_host_bus_init_inplace(PCIHostState *phb, PCIBus *bus,
+                               size_t bus_size, const char *name,
+                               MemoryRegion *address_space_mem,
+                               MemoryRegion *address_space_io,
+                               uint8_t devfn_min, const char *typename)
 {
-    PCIHostState *phb = PCI_HOST_BRIDGE(parent);
-
-    qbus_create_inplace(bus, bus_size, typename, parent, name);
+    qbus_create_inplace(bus, bus_size, typename, DEVICE(phb), name);
 
     assert(PCI_FUNC(devfn_min) == 0);
     bus->devfn_min = devfn_min;
@@ -389,16 +387,17 @@ void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
 
 }
 
-PCIBus *pci_bus_new(DeviceState *parent, const char *name,
-                    MemoryRegion *address_space_mem,
-                    MemoryRegion *address_space_io,
-                    uint8_t devfn_min, const char *typename)
+PCIBus *pci_host_bus_init(PCIHostState *phb, const char *name,
+                           MemoryRegion *address_space_mem,
+                           MemoryRegion *address_space_io, uint8_t devfn_min,
+                           const char *typename)
 {
     size_t bus_size = object_type_get_instance_size(typename);
     PCIBus *bus = g_malloc(bus_size);
 
-    pci_bus_new_inplace(bus, bus_size, parent, name, address_space_mem,
-                        address_space_io, devfn_min, typename);
+    pci_host_bus_init_inplace(phb, bus, bus_size,
+                              name, address_space_mem, address_space_io,
+                              devfn_min, typename);
     return bus;
 }
 
@@ -412,17 +411,19 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
-PCIBus *pci_register_bus(DeviceState *parent, const char *name,
-                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
-                         uint8_t devfn_min, int nirq, const char *typename)
+PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, const char *name,
+                                pci_set_irq_fn set_irq,
+                                pci_map_irq_fn map_irq, void *irq_opaque,
+                                MemoryRegion *address_space_mem,
+                                MemoryRegion *address_space_io,
+                                uint8_t devfn_min, int nirq,
+                                const char *typename)
 {
     PCIBus *bus;
 
-    bus = pci_bus_new(parent, name, address_space_mem,
-                      address_space_io, devfn_min, typename);
+    bus = pci_host_bus_init(phb, name,
+                            address_space_mem,
+                            address_space_io, devfn_min, typename);
     pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
     return bus;
 }
diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
index dc19682970..0d767a1a23 100644
--- a/hw/ppc/ppc4xx_pci.c
+++ b/hw/ppc/ppc4xx_pci.c
@@ -314,9 +314,10 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
         sysbus_init_irq(dev, &s->irq[i]);
     }
 
-    b = pci_register_bus(DEVICE(dev), NULL, ppc4xx_pci_set_irq,
-                         ppc4xx_pci_map_irq, s->irq, get_system_memory(),
-                         get_system_io(), 0, 4, TYPE_PCI_BUS);
+    b = pci_host_bus_init_irqs(h, NULL,
+                               ppc4xx_pci_set_irq, ppc4xx_pci_map_irq, s->irq,
+                               get_system_memory(), get_system_io(), 0, 4,
+                               TYPE_PCI_BUS);
     h->bus = b;
 
     pci_create_simple(b, 0, "ppc4xx-host-bridge");
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 98c52e411f..7f29cc77b0 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1697,10 +1697,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
 
-    bus = pci_register_bus(dev, NULL,
-                           pci_spapr_set_irq, pci_spapr_map_irq, sphb,
-                           &sphb->memspace, &sphb->iospace,
-                           PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
+    bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
+                                 pci_spapr_set_irq, pci_spapr_map_irq, sphb,
+                                 &sphb->memspace, &sphb->iospace,
+                                 PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
     phb->bus = bus;
     qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL);
 
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 69b0291e8a..267e1de510 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -560,10 +560,10 @@ static int s390_pcihost_init(SysBusDevice *dev)
 
     DPRINTF("host_init\n");
 
-    b = pci_register_bus(DEVICE(dev), NULL,
-                         s390_pci_set_irq, s390_pci_map_irq, NULL,
-                         get_system_memory(), get_system_io(), 0, 64,
-                         TYPE_PCI_BUS);
+    b = pci_host_bus_init_irqs(phb, NULL,
+                               s390_pci_set_irq, s390_pci_map_irq, NULL,
+                               get_system_memory(), get_system_io(), 0, 64,
+                               TYPE_PCI_BUS);
     pci_setup_iommu(b, s390_pci_dma_iommu, s);
 
     bus = BUS(b);
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index 1747628f3d..f589b1628e 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -131,12 +131,10 @@ static int sh_pci_device_init(SysBusDevice *dev)
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(dev, &s->irq[i]);
     }
-    phb->bus = pci_register_bus(DEVICE(dev), "pci",
-                                sh_pci_set_irq, sh_pci_map_irq,
-                                s->irq,
-                                get_system_memory(),
-                                get_system_io(),
-                                PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
+    phb->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), "pci",
+                                      sh_pci_set_irq, sh_pci_map_irq, s->irq,
+                                      get_system_memory(), get_system_io(),
+                                      PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
     memory_region_init_io(&s->memconfig_p4, OBJECT(s), &sh_pci_reg_ops, s,
                           "sh_pci", 0x224);
     memory_region_init_alias(&s->memconfig_a7, OBJECT(s), "sh_pci.2",
-- 
2.11.0.259.g40922b1

On Tue, Apr 18, 2017 at 07:17:21PM -0300, Eduardo Habkost wrote:
> pci_bus_new*() and pci_register_bus() work only when the 'parent'
> argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> are meant to initialize a bus that's in a PCI host bridge.
> 
> The new function names are:
> * pci_host_bus_init() (replacing pci_bus_new())
> * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> * pci_host_bus_init_irqs() (replacing pci_register_bus())

I like the idea, but I'm not terribly convinced by these names.
Aren't functions which allocate objects usually called whatever_new()
rather than whatever_init()?  And pci_register_bus() appears to do
more than just initialize irqs.

> This also replaces the DeviceState *parent parameter on those functions
> with a PCIHostState *phb parameter.
> 
> This was implemented using the following Coccinelle patch:
> 
>     // 1) Rename pci_bus_new():
>     @@
>     typedef PCIBus;
>     typedef PCIHostState;
>     typedef DeviceState;
>     identifier parent;
>     parameter list ARGS;
>     @@
>     -PCIBus *pci_bus_new(DeviceState *parent, ARGS);
>     +PCIBus *pci_host_bus_init(PCIHostState *phb, ARGS);
> 
>     @@
>     typedef PCIBus;
>     typedef PCIHostState;
>     identifier parent;
>     parameter list ARGS;
>     @@
>     -PCIBus *pci_bus_new(DeviceState *parent, ARGS)
>     +PCIBus *pci_host_bus_init(PCIHostState *phb, ARGS)
>      {
>      <...
>     -parent
>     +DEVICE(phb)
>      ...>
>      }
> 
>     @@
>     expression parent;
>     expression list ARGS;
>     @@
>     -pci_bus_new(parent, ARGS)
>     +pci_host_bus_init(PCI_HOST_BRIDGE(parent), ARGS)
> 
>     // 2) Rename pci_bus_new_inplace():
>     @@
>     typedef PCIBus;
>     typedef PCIHostState;
>     typedef DeviceState;
>     identifier bus, bus_size, parent;
>     parameter list ARGS;
>     @@
>     -void pci_bus_new_inplace(PCIBus *bus, size_t bus_size,
>     -                         DeviceState *parent, ARGS);
>     +void pci_host_bus_init_inplace(PCIHostState *phb,
>     +                               PCIBus *bus, size_t bus_size, ARGS);
> 
>     @@
>     typedef PCIBus;
>     typedef PCIHostState;
>     typedef DeviceState;
>     identifier bus, bus_size, parent;
>     parameter list ARGS;
>     @@
>     -void pci_bus_new_inplace(PCIBus *bus, size_t bus_size,
>     -                         DeviceState *parent, ARGS)
>     +void pci_host_bus_init_inplace(PCIHostState *phb,
>     +                               PCIBus *bus, size_t bus_size, ARGS)
>      {
>     -PCIHostState *phb = PCI_HOST_BRIDGE(parent);
>      <...
>     -parent
>     +DEVICE(phb)
>      ...>
>      }
> 
>     @@
>     expression bus, bus_size, parent;
>     expression list ARGS;
>     @@
>     -pci_bus_new_inplace(bus, bus_size, parent, ARGS)
>     +pci_host_bus_init_inplace(PCI_HOST_BRIDGE(parent), bus, bus_size, ARGS)
> 
>     // 3) Rename pci_register_bus():
>     @@
>     typedef PCIBus;
>     typedef PCIHostState;
>     identifier parent;
>     parameter list ARGS;
>     @@
>     -PCIBus *pci_register_bus(DeviceState *parent, ARGS);
>     +PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, ARGS);
> 
>     @@
>     typedef PCIBus;
>     typedef PCIHostState;
>     identifier parent;
>     parameter list ARGS;
>     @@
>     -PCIBus *pci_register_bus(DeviceState *parent, ARGS)
>     +PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, ARGS)
>      {
>      <...
>     -parent
>     +DEVICE(phb)
>      ...>
>      }
> 
>     @@
>     expression parent;
>     expression list ARGS;
>     @@
>     -pci_register_bus(parent, ARGS)
>     +pci_host_bus_init_irqs(PCI_HOST_BRIDGE(parent), ARGS)
> 
>     // 5) fix redundant casts on the resulting code:
>     @@
>     expression o;
>     @@
>     -PCI_HOST_BRIDGE(DEVICE(o))
>     +PCI_HOST_BRIDGE(o)
> 
>     @@
>     expression o;
>     @@
>     -DEVICE(PCI_HOST_BRIDGE(o))
>     +DEVICE(o)
> 
>     @@
>     idexpression PCIHostState *phb;
>     @@
>     -PCI_HOST_BRIDGE(phb)
>     +phb
> 
>     @@
>     idexpression PCIHostState *phb;
>     expression dev;
>     @@
>      phb = PCI_HOST_BRIDGE(dev);
>      <...
>     -PCI_HOST_BRIDGE(dev)
>     +phb
>      ...>
> 
>     @@
>     identifier phb;
>     identifier dev;
>     @@
>      PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>      <...
>     -PCI_HOST_BRIDGE(dev)
>     +phb
>      ...>
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  include/hw/pci/pci.h                | 31 ++++++++++++-------------
>  hw/alpha/typhoon.c                  |  7 +++---
>  hw/mips/gt64xxx_pci.c               | 11 +++++----
>  hw/pci-bridge/pci_expander_bridge.c |  6 +++--
>  hw/pci-host/apb.c                   |  9 ++++----
>  hw/pci-host/bonito.c                |  8 +++----
>  hw/pci-host/gpex.c                  |  7 +++---
>  hw/pci-host/grackle.c               | 11 ++++-----
>  hw/pci-host/piix.c                  |  4 ++--
>  hw/pci-host/ppce500.c               |  7 +++---
>  hw/pci-host/prep.c                  |  5 +++--
>  hw/pci-host/q35.c                   |  6 ++---
>  hw/pci-host/uninorth.c              | 20 +++++++----------
>  hw/pci-host/versatile.c             |  7 +++---
>  hw/pci-host/xilinx-pcie.c           |  7 +++---
>  hw/pci/pci.c                        | 45 +++++++++++++++++++------------------
>  hw/ppc/ppc4xx_pci.c                 |  7 +++---
>  hw/ppc/spapr_pci.c                  |  8 +++----
>  hw/s390x/s390-pci-bus.c             |  8 +++----
>  hw/sh4/sh_pci.c                     | 10 ++++-----
>  20 files changed, 111 insertions(+), 113 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5cb6..85fe3ae743 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -393,26 +393,27 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
>  
>  bool pci_bus_is_express(PCIBus *bus);
>  bool pci_bus_is_root(PCIBus *bus);
> -void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> -                         const char *name,
> -                         MemoryRegion *address_space_mem,
> -                         MemoryRegion *address_space_io,
> -                         uint8_t devfn_min, const char *typename);
> -PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> -                    MemoryRegion *address_space_mem,
> -                    MemoryRegion *address_space_io,
> -                    uint8_t devfn_min, const char *typename);
> +void pci_host_bus_init_inplace(PCIHostState *phb, PCIBus *bus,
> +                               size_t bus_size, const char *name,
> +                               MemoryRegion *address_space_mem,
> +                               MemoryRegion *address_space_io,
> +                               uint8_t devfn_min, const char *typename);
> +PCIBus *pci_host_bus_init(PCIHostState *phb, const char *name,
> +                          MemoryRegion *address_space_mem,
> +                          MemoryRegion *address_space_io, uint8_t devfn_min,
> +                          const char *typename);
>  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                    void *irq_opaque, int nirq);
>  int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
>  /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
>  int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
> -PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> -                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                         void *irq_opaque,
> -                         MemoryRegion *address_space_mem,
> -                         MemoryRegion *address_space_io,
> -                         uint8_t devfn_min, int nirq, const char *typename);
> +PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, const char *name,
> +                               pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> +                               void *irq_opaque,
> +                               MemoryRegion *address_space_mem,
> +                               MemoryRegion *address_space_io,
> +                               uint8_t devfn_min, int nirq,
> +                               const char *typename);
>  void pci_bus_set_route_irq_fn(PCIBus *, pci_route_irq_fn);
>  PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin);
>  bool pci_intx_route_changed(PCIINTxRoute *old, PCIINTxRoute *new);
> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
> index f50f5cf186..24f1959ab8 100644
> --- a/hw/alpha/typhoon.c
> +++ b/hw/alpha/typhoon.c
> @@ -883,10 +883,9 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
>      memory_region_add_subregion(addr_space, 0x801fc000000ULL,
>                                  &s->pchip.reg_io);
>  
> -    b = pci_register_bus(dev, "pci",
> -                         typhoon_set_irq, sys_map_irq, s,
> -                         &s->pchip.reg_mem, &s->pchip.reg_io,
> -                         0, 64, TYPE_PCI_BUS);
> +    b = pci_host_bus_init_irqs(phb, "pci", typhoon_set_irq,
> +                               sys_map_irq, s, &s->pchip.reg_mem,
> +                               &s->pchip.reg_io, 0, 64, TYPE_PCI_BUS);
>      phb->bus = b;
>      qdev_init_nofail(dev);
>  
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index 4811843ab6..5ca9f07031 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1171,12 +1171,11 @@ PCIBus *gt64120_register(qemu_irq *pic)
>      phb = PCI_HOST_BRIDGE(dev);
>      memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", UINT32_MAX);
>      address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");
> -    phb->bus = pci_register_bus(dev, "pci",
> -                                gt64120_pci_set_irq, gt64120_pci_map_irq,
> -                                pic,
> -                                &d->pci0_mem,
> -                                get_system_io(),
> -                                PCI_DEVFN(18, 0), 4, TYPE_PCI_BUS);
> +    phb->bus = pci_host_bus_init_irqs(phb, "pci",
> +                                      gt64120_pci_set_irq,
> +                                      gt64120_pci_map_irq, pic, &d->pci0_mem,
> +                                      get_system_io(), PCI_DEVFN(18, 0), 4,
> +                                      TYPE_PCI_BUS);
>      qdev_init_nofail(dev);
>      memory_region_init_io(&d->ISD_mem, OBJECT(dev), &isd_mem_ops, d, "isd-mem", 0x1000);
>  
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 6ac187fa32..853448ef70 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -230,9 +230,11 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
>  
>      ds = qdev_create(NULL, TYPE_PXB_HOST);
>      if (pcie) {
> -        bus = pci_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
> +        bus = pci_host_bus_init(PCI_HOST_BRIDGE(ds), dev_name, NULL, NULL, 0,
> +                                TYPE_PXB_PCIE_BUS);
>      } else {
> -        bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
> +        bus = pci_host_bus_init(PCI_HOST_BRIDGE(ds), "pxb-internal", NULL,
> +                                NULL, 0, TYPE_PXB_BUS);
>          bds = qdev_create(BUS(bus), "pci-bridge");
>          bds->id = dev_name;
>          qdev_prop_set_uint8(bds, PCI_BRIDGE_DEV_PROP_CHASSIS_NR, pxb->bus_nr);
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 653e711121..38d08c661c 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -671,11 +671,10 @@ PCIBus *pci_apb_init(hwaddr special_base,
>      dev = qdev_create(NULL, TYPE_APB);
>      d = APB_DEVICE(dev);
>      phb = PCI_HOST_BRIDGE(dev);
> -    phb->bus = pci_register_bus(DEVICE(phb), "pci",
> -                                pci_apb_set_irq, pci_pbm_map_irq, d,
> -                                &d->pci_mmio,
> -                                get_system_io(),
> -                                0, 32, TYPE_PCI_BUS);
> +    phb->bus = pci_host_bus_init_irqs(phb, "pci",
> +                                      pci_apb_set_irq, pci_pbm_map_irq, d,
> +                                      &d->pci_mmio, get_system_io(), 0, 32,
> +                                      TYPE_PCI_BUS);
>      qdev_init_nofail(dev);
>      s = SYS_BUS_DEVICE(dev);
>      /* apb_config */
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index 1999ece590..8b6e80aa82 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -714,10 +714,10 @@ static int bonito_pcihost_initfn(SysBusDevice *dev)
>  {
>      PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>  
> -    phb->bus = pci_register_bus(DEVICE(dev), "pci",
> -                                pci_bonito_set_irq, pci_bonito_map_irq, dev,
> -                                get_system_memory(), get_system_io(),
> -                                0x28, 32, TYPE_PCI_BUS);
> +    phb->bus = pci_host_bus_init_irqs(phb, "pci",
> +                                      pci_bonito_set_irq, pci_bonito_map_irq,
> +                                      dev, get_system_memory(),
> +                                      get_system_io(), 0x28, 32, TYPE_PCI_BUS);
>  
>      return 0;
>  }
> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> index 66055ee5cc..9ea9927cf8 100644
> --- a/hw/pci-host/gpex.c
> +++ b/hw/pci-host/gpex.c
> @@ -62,9 +62,10 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
>          sysbus_init_irq(sbd, &s->irq[i]);
>      }
>  
> -    pci->bus = pci_register_bus(dev, "pcie.0", gpex_set_irq,
> -                                pci_swizzle_map_irq_fn, s, &s->io_mmio,
> -                                &s->io_ioport, 0, 4, TYPE_PCIE_BUS);
> +    pci->bus = pci_host_bus_init_irqs(pci, "pcie.0",
> +                                      gpex_set_irq, pci_swizzle_map_irq_fn, s,
> +                                      &s->io_mmio, &s->io_ioport, 0, 4,
> +                                      TYPE_PCIE_BUS);
>  
>      qdev_set_parent_bus(DEVICE(&s->gpex_root), BUS(pci->bus));
>      qdev_init_nofail(DEVICE(&s->gpex_root));
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index 2c8acdaaca..0844c30d9a 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -82,13 +82,10 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
>      memory_region_add_subregion(address_space_mem, 0x80000000ULL,
>                                  &d->pci_hole);
>  
> -    phb->bus = pci_register_bus(dev, NULL,
> -                                pci_grackle_set_irq,
> -                                pci_grackle_map_irq,
> -                                pic,
> -                                &d->pci_mmio,
> -                                address_space_io,
> -                                0, 4, TYPE_PCI_BUS);
> +    phb->bus = pci_host_bus_init_irqs(phb, NULL,
> +                                      pci_grackle_set_irq,
> +                                      pci_grackle_map_irq, pic, &d->pci_mmio,
> +                                      address_space_io, 0, 4, TYPE_PCI_BUS);
>  
>      pci_create_simple(phb->bus, 0, "grackle");
>      qdev_init_nofail(dev);
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9218aa952..364d57039b 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -340,8 +340,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>  
>      dev = qdev_create(NULL, host_type);
>      s = PCI_HOST_BRIDGE(dev);
> -    b = pci_bus_new(dev, NULL, pci_address_space,
> -                    address_space_io, 0, TYPE_PCI_BUS);
> +    b = pci_host_bus_init(s, NULL, pci_address_space,
> +                          address_space_io, 0, TYPE_PCI_BUS);
>      s->bus = b;
>      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index e502bc0505..babb12af31 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -465,9 +465,10 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>      /* PIO lives at the bottom of our bus space */
>      memory_region_add_subregion_overlap(&s->busmem, 0, &s->pio, -2);
>  
> -    b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> -                         mpc85xx_pci_map_irq, s, &s->busmem, &s->pio,
> -                         PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
> +    b = pci_host_bus_init_irqs(h, NULL,
> +                               mpc85xx_pci_set_irq, mpc85xx_pci_map_irq, s,
> +                               &s->busmem, &s->pio,
> +                               PCI_DEVFN(s->first_slot, 0), 4, TYPE_PCI_BUS);
>      h->bus = b;
>  
>      /* Set up PCI view of memory */
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 260a119a9e..65add936c7 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -269,8 +269,9 @@ static void raven_pcihost_initfn(Object *obj)
>      memory_region_add_subregion_overlap(address_space_mem, 0x80000000,
>                                          &s->pci_io_non_contiguous, 1);
>      memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> -    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
> -                        &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
> +    pci_host_bus_init_inplace(h, &s->pci_bus,
> +                              sizeof(s->pci_bus), NULL, &s->pci_memory,
> +                              &s->pci_io, 0, TYPE_PCI_BUS);
>  
>      /* Bus master address space */
>      memory_region_init(&s->bm, obj, "bm-raven", UINT32_MAX);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 344f77b10c..947dc3f124 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -49,9 +49,9 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
>      sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem);
>      sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
>  
> -    pci->bus = pci_bus_new(DEVICE(s), "pcie.0",
> -                           s->mch.pci_address_space, s->mch.address_space_io,
> -                           0, TYPE_PCIE_BUS);
> +    pci->bus = pci_host_bus_init(PCI_HOST_BRIDGE(s), "pcie.0",
> +                                 s->mch.pci_address_space,
> +                                 s->mch.address_space_io, 0, TYPE_PCIE_BUS);
>      PC_MACHINE(qdev_get_machine())->bus = pci->bus;
>      qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));
>      qdev_init_nofail(DEVICE(&s->mch));
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index df342ac3cb..079faad6ff 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -233,12 +233,10 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
>      memory_region_add_subregion(address_space_mem, 0x80000000ULL,
>                                  &d->pci_hole);
>  
> -    h->bus = pci_register_bus(dev, NULL,
> -                              pci_unin_set_irq, pci_unin_map_irq,
> -                              pic,
> -                              &d->pci_mmio,
> -                              address_space_io,
> -                              PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
> +    h->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
> +                                    pci_unin_set_irq, pci_unin_map_irq, pic,
> +                                    &d->pci_mmio, address_space_io,
> +                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>  
>  #if 0
>      pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north");
> @@ -299,12 +297,10 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
>      memory_region_add_subregion(address_space_mem, 0x80000000ULL,
>                                  &d->pci_hole);
>  
> -    h->bus = pci_register_bus(dev, NULL,
> -                              pci_unin_set_irq, pci_unin_map_irq,
> -                              pic,
> -                              &d->pci_mmio,
> -                              address_space_io,
> -                              PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
> +    h->bus = pci_host_bus_init_irqs(h, NULL,
> +                                    pci_unin_set_irq, pci_unin_map_irq, pic,
> +                                    &d->pci_mmio, address_space_io,
> +                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>  
>      sysbus_mmio_map(s, 0, 0xf0800000);
>      sysbus_mmio_map(s, 1, 0xf0c00000);
> diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
> index 467cbb9cb8..cad8848723 100644
> --- a/hw/pci-host/versatile.c
> +++ b/hw/pci-host/versatile.c
> @@ -386,9 +386,10 @@ static void pci_vpb_init(Object *obj)
>      memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
>      memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
>  
> -    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
> -                        &s->pci_mem_space, &s->pci_io_space,
> -                        PCI_DEVFN(11, 0), TYPE_PCI_BUS);
> +    pci_host_bus_init_inplace(h, &s->pci_bus,
> +                              sizeof(s->pci_bus), "pci", &s->pci_mem_space,
> +                              &s->pci_io_space, PCI_DEVFN(11, 0),
> +                              TYPE_PCI_BUS);
>      h->bus = &s->pci_bus;
>  
>      object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_VERSATILE_PCI_HOST);
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 8b71e2d950..0a3d19b22c 100644
> --- a/hw/pci-host/xilinx-pcie.c
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -129,9 +129,10 @@ static void xilinx_pcie_host_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(sbd, &pex->mmio);
>      sysbus_init_mmio(sbd, &s->mmio);
>  
> -    pci->bus = pci_register_bus(dev, s->name, xilinx_pcie_set_irq,
> -                                pci_swizzle_map_irq_fn, s, &s->mmio,
> -                                &s->io, 0, 4, TYPE_PCIE_BUS);
> +    pci->bus = pci_host_bus_init_irqs(pci, s->name,
> +                                      xilinx_pcie_set_irq,
> +                                      pci_swizzle_map_irq_fn, s, &s->mmio,
> +                                      &s->io, 0, 4, TYPE_PCIE_BUS);
>  
>      qdev_set_parent_bus(DEVICE(&s->root), BUS(pci->bus));
>      qdev_init_nofail(DEVICE(&s->root));
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0d28ee4e3f..37d65eaf7e 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -367,15 +367,13 @@ bool pci_bus_is_root(PCIBus *bus)
>      return PCI_BUS_GET_CLASS(bus)->is_root(bus);
>  }
>  
> -void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
> -                         const char *name,
> -                         MemoryRegion *address_space_mem,
> -                         MemoryRegion *address_space_io,
> -                         uint8_t devfn_min, const char *typename)
> +void pci_host_bus_init_inplace(PCIHostState *phb, PCIBus *bus,
> +                               size_t bus_size, const char *name,
> +                               MemoryRegion *address_space_mem,
> +                               MemoryRegion *address_space_io,
> +                               uint8_t devfn_min, const char *typename)
>  {
> -    PCIHostState *phb = PCI_HOST_BRIDGE(parent);
> -
> -    qbus_create_inplace(bus, bus_size, typename, parent, name);
> +    qbus_create_inplace(bus, bus_size, typename, DEVICE(phb), name);
>  
>      assert(PCI_FUNC(devfn_min) == 0);
>      bus->devfn_min = devfn_min;
> @@ -389,16 +387,17 @@ void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState *parent,
>  
>  }
>  
> -PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> -                    MemoryRegion *address_space_mem,
> -                    MemoryRegion *address_space_io,
> -                    uint8_t devfn_min, const char *typename)
> +PCIBus *pci_host_bus_init(PCIHostState *phb, const char *name,
> +                           MemoryRegion *address_space_mem,
> +                           MemoryRegion *address_space_io, uint8_t devfn_min,
> +                           const char *typename)
>  {
>      size_t bus_size = object_type_get_instance_size(typename);
>      PCIBus *bus = g_malloc(bus_size);
>  
> -    pci_bus_new_inplace(bus, bus_size, parent, name, address_space_mem,
> -                        address_space_io, devfn_min, typename);
> +    pci_host_bus_init_inplace(phb, bus, bus_size,
> +                              name, address_space_mem, address_space_io,
> +                              devfn_min, typename);
>      return bus;
>  }
>  
> @@ -412,17 +411,19 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>      bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
>  }
>  
> -PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> -                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                         void *irq_opaque,
> -                         MemoryRegion *address_space_mem,
> -                         MemoryRegion *address_space_io,
> -                         uint8_t devfn_min, int nirq, const char *typename)
> +PCIBus *pci_host_bus_init_irqs(PCIHostState *phb, const char *name,
> +                                pci_set_irq_fn set_irq,
> +                                pci_map_irq_fn map_irq, void *irq_opaque,
> +                                MemoryRegion *address_space_mem,
> +                                MemoryRegion *address_space_io,
> +                                uint8_t devfn_min, int nirq,
> +                                const char *typename)
>  {
>      PCIBus *bus;
>  
> -    bus = pci_bus_new(parent, name, address_space_mem,
> -                      address_space_io, devfn_min, typename);
> +    bus = pci_host_bus_init(phb, name,
> +                            address_space_mem,
> +                            address_space_io, devfn_min, typename);
>      pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
>      return bus;
>  }
> diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
> index dc19682970..0d767a1a23 100644
> --- a/hw/ppc/ppc4xx_pci.c
> +++ b/hw/ppc/ppc4xx_pci.c
> @@ -314,9 +314,10 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
>          sysbus_init_irq(dev, &s->irq[i]);
>      }
>  
> -    b = pci_register_bus(DEVICE(dev), NULL, ppc4xx_pci_set_irq,
> -                         ppc4xx_pci_map_irq, s->irq, get_system_memory(),
> -                         get_system_io(), 0, 4, TYPE_PCI_BUS);
> +    b = pci_host_bus_init_irqs(h, NULL,
> +                               ppc4xx_pci_set_irq, ppc4xx_pci_map_irq, s->irq,
> +                               get_system_memory(), get_system_io(), 0, 4,
> +                               TYPE_PCI_BUS);
>      h->bus = b;
>  
>      pci_create_simple(b, 0, "ppc4xx-host-bridge");
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 98c52e411f..7f29cc77b0 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1697,10 +1697,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
>                                  &sphb->iowindow);
>  
> -    bus = pci_register_bus(dev, NULL,
> -                           pci_spapr_set_irq, pci_spapr_map_irq, sphb,
> -                           &sphb->memspace, &sphb->iospace,
> -                           PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
> +    bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
> +                                 pci_spapr_set_irq, pci_spapr_map_irq, sphb,
> +                                 &sphb->memspace, &sphb->iospace,
> +                                 PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
>      phb->bus = bus;
>      qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL);
>  
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 69b0291e8a..267e1de510 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -560,10 +560,10 @@ static int s390_pcihost_init(SysBusDevice *dev)
>  
>      DPRINTF("host_init\n");
>  
> -    b = pci_register_bus(DEVICE(dev), NULL,
> -                         s390_pci_set_irq, s390_pci_map_irq, NULL,
> -                         get_system_memory(), get_system_io(), 0, 64,
> -                         TYPE_PCI_BUS);
> +    b = pci_host_bus_init_irqs(phb, NULL,
> +                               s390_pci_set_irq, s390_pci_map_irq, NULL,
> +                               get_system_memory(), get_system_io(), 0, 64,
> +                               TYPE_PCI_BUS);
>      pci_setup_iommu(b, s390_pci_dma_iommu, s);
>  
>      bus = BUS(b);
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index 1747628f3d..f589b1628e 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -131,12 +131,10 @@ static int sh_pci_device_init(SysBusDevice *dev)
>      for (i = 0; i < 4; i++) {
>          sysbus_init_irq(dev, &s->irq[i]);
>      }
> -    phb->bus = pci_register_bus(DEVICE(dev), "pci",
> -                                sh_pci_set_irq, sh_pci_map_irq,
> -                                s->irq,
> -                                get_system_memory(),
> -                                get_system_io(),
> -                                PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
> +    phb->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), "pci",
> +                                      sh_pci_set_irq, sh_pci_map_irq, s->irq,
> +                                      get_system_memory(), get_system_io(),
> +                                      PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
>      memory_region_init_io(&s->memconfig_p4, OBJECT(s), &sh_pci_reg_ops, s,
>                            "sh_pci", 0x224);
>      memory_region_init_alias(&s->memconfig_a7, OBJECT(s), "sh_pci.2",

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Wed, Apr 19, 2017 at 10:29:26AM +1000, David Gibson wrote:
> On Tue, Apr 18, 2017 at 07:17:21PM -0300, Eduardo Habkost wrote:
> > pci_bus_new*() and pci_register_bus() work only when the 'parent'
> > argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> > are meant to initialize a bus that's in a PCI host bridge.
> > 
> > The new function names are:
> > * pci_host_bus_init() (replacing pci_bus_new())
> > * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> > * pci_host_bus_init_irqs() (replacing pci_register_bus())
> 
> I like the idea, but I'm not terribly convinced by these names.
> Aren't functions which allocate objects usually called whatever_new()
> rather than whatever_init()?  And pci_register_bus() appears to do
> more than just initialize irqs.

I agree the names aren't terribly clear. This is what they are
supposed to mean:

* pci_host_bus_init(phb) initializes phb->bus.
* pci_host_bus_init(phb) initializes phb->bus using an
  already-allocated object.
* pci_host_bus_init_irqs() does the same as pci_host_bus_init(),
  but also calls pci_bus_irqs().

I plan to submit API documentation comments later. I am open to
alternative name suggestions.

-- 
Eduardo

On Tue, 18 Apr 2017 22:42:07 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 19, 2017 at 10:29:26AM +1000, David Gibson wrote:
> > On Tue, Apr 18, 2017 at 07:17:21PM -0300, Eduardo Habkost wrote:
> > > pci_bus_new*() and pci_register_bus() work only when the 'parent'
> > > argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> > > are meant to initialize a bus that's in a PCI host bridge.
> > > 
> > > The new function names are:
> > > * pci_host_bus_init() (replacing pci_bus_new())
> > > * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> > > * pci_host_bus_init_irqs() (replacing pci_register_bus())
> > 
> > I like the idea, but I'm not terribly convinced by these names.
> > Aren't functions which allocate objects usually called whatever_new()
> > rather than whatever_init()?  And pci_register_bus() appears to do
> > more than just initialize irqs.
> 
> I agree the names aren't terribly clear. This is what they are
> supposed to mean:
> 
> * pci_host_bus_init(phb) initializes phb->bus.
> * pci_host_bus_init(phb) initializes phb->bus using an
>   already-allocated object.
> * pci_host_bus_init_irqs() does the same as pci_host_bus_init(),
>   but also calls pci_bus_irqs().
> 
> I plan to submit API documentation comments later. I am open to
> alternative name suggestions.
> 

pci_host_bus_init_irqs() sounds as if it would only init irqs. What
about:

pci_host_bus_new()
pci_host_bus_new_inplace()
pci_host_bus_new_with_irqs()

(the last one might be a bit long, though, especially as it takes so
many arguments already)

On 04/19/2017 03:05 PM, Cornelia Huck wrote:
> On Tue, 18 Apr 2017 22:42:07 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Wed, Apr 19, 2017 at 10:29:26AM +1000, David Gibson wrote:
>>> On Tue, Apr 18, 2017 at 07:17:21PM -0300, Eduardo Habkost wrote:
>>>> pci_bus_new*() and pci_register_bus() work only when the 'parent'
>>>> argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
>>>> are meant to initialize a bus that's in a PCI host bridge.
>>>>
>>>> The new function names are:
>>>> * pci_host_bus_init() (replacing pci_bus_new())
>>>> * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
>>>> * pci_host_bus_init_irqs() (replacing pci_register_bus())
>>>
>>> I like the idea, but I'm not terribly convinced by these names.
>>> Aren't functions which allocate objects usually called whatever_new()
>>> rather than whatever_init()?  And pci_register_bus() appears to do
>>> more than just initialize irqs.
>>
>> I agree the names aren't terribly clear. This is what they are
>> supposed to mean:
>>
>> * pci_host_bus_init(phb) initializes phb->bus.
>> * pci_host_bus_init(phb) initializes phb->bus using an
>>   already-allocated object.
>> * pci_host_bus_init_irqs() does the same as pci_host_bus_init(),
>>   but also calls pci_bus_irqs().
>>
>> I plan to submit API documentation comments later. I am open to
>> alternative name suggestions.
>>
>
> pci_host_bus_init_irqs() sounds as if it would only init irqs. What

Right! This is what I thought too.

> about:
>
> pci_host_bus_new()
> pci_host_bus_new_inplace()
> pci_host_bus_new_with_irqs()

I like the names, but a following patch (5/6) modifies
the above functions to return void and create the bus
as a side effect.
So now we have a pci_host_bus_new that returns nothing?

Thanks,
Marcel


>
> (the last one might be a bit long, though, especially as it takes so
> many arguments already)
>

On Wed, Apr 19, 2017 at 09:41:19PM +0300, Marcel Apfelbaum wrote:
> On 04/19/2017 03:05 PM, Cornelia Huck wrote:
> > On Tue, 18 Apr 2017 22:42:07 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, Apr 19, 2017 at 10:29:26AM +1000, David Gibson wrote:
> > > > On Tue, Apr 18, 2017 at 07:17:21PM -0300, Eduardo Habkost wrote:
> > > > > pci_bus_new*() and pci_register_bus() work only when the 'parent'
> > > > > argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> > > > > are meant to initialize a bus that's in a PCI host bridge.
> > > > > 
> > > > > The new function names are:
> > > > > * pci_host_bus_init() (replacing pci_bus_new())
> > > > > * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> > > > > * pci_host_bus_init_irqs() (replacing pci_register_bus())
> > > > 
> > > > I like the idea, but I'm not terribly convinced by these names.
> > > > Aren't functions which allocate objects usually called whatever_new()
> > > > rather than whatever_init()?  And pci_register_bus() appears to do
> > > > more than just initialize irqs.
> > > 
> > > I agree the names aren't terribly clear. This is what they are
> > > supposed to mean:
> > > 
> > > * pci_host_bus_init(phb) initializes phb->bus.
> > > * pci_host_bus_init(phb) initializes phb->bus using an
> > >   already-allocated object.
> > > * pci_host_bus_init_irqs() does the same as pci_host_bus_init(),
> > >   but also calls pci_bus_irqs().
> > > 
> > > I plan to submit API documentation comments later. I am open to
> > > alternative name suggestions.
> > > 
> > 
> > pci_host_bus_init_irqs() sounds as if it would only init irqs. What
> 
> Right! This is what I thought too.
> 
> > about:
> > 
> > pci_host_bus_new()
> > pci_host_bus_new_inplace()
> > pci_host_bus_new_with_irqs()
> 
> I like the names, but a following patch (5/6) modifies
> the above functions to return void and create the bus
> as a side effect.
> So now we have a pci_host_bus_new that returns nothing?

I plan to change the approach, and not get rid of pci_bus_new().
I will probably just move the PCI_HOST_BRIDGE-specific code to a
separate function (probably I will name it pci_host_init_bus()),
that will call pci_bus_new(), append the bridge to
pci_host_bridges, and set PCIHostState::bus.

After that, we can change pci_bridge_initfn() to use
pci_bus_new() instead of reimplementing it.

-- 
Eduardo

On 18 April 2017 at 23:17, Eduardo Habkost <ehabkost@redhat.com> wrote:
> pci_bus_new*() and pci_register_bus() work only when the 'parent'
> argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> are meant to initialize a bus that's in a PCI host bridge.
>
> The new function names are:
> * pci_host_bus_init() (replacing pci_bus_new())
> * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> * pci_host_bus_init_irqs() (replacing pci_register_bus())

This is moving these functions away from the convention that
we seem to mostly follow for buses (eg ISA, SCSI) of
foo_bus_new() to allocate and return a new bus, and
foo_bus_new_inplace() to initialize a bus that's inline in
some other struct.

I'm not sure this is a good idea unless we have a plan to
convert all the other QOM buses and document that this is
the right way to implement init for a bus.

thanks
-- PMM

On Wed, Apr 19, 2017 at 09:41:42AM +0100, Peter Maydell wrote:
> On 18 April 2017 at 23:17, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > pci_bus_new*() and pci_register_bus() work only when the 'parent'
> > argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> > are meant to initialize a bus that's in a PCI host bridge.
> >
> > The new function names are:
> > * pci_host_bus_init() (replacing pci_bus_new())
> > * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> > * pci_host_bus_init_irqs() (replacing pci_register_bus())
> 
> This is moving these functions away from the convention that
> we seem to mostly follow for buses (eg ISA, SCSI) of
> foo_bus_new() to allocate and return a new bus, and
> foo_bus_new_inplace() to initialize a bus that's inline in
> some other struct.
> 
> I'm not sure this is a good idea unless we have a plan to
> convert all the other QOM buses and document that this is
> the right way to implement init for a bus.

The point of the rename is that those functions are doing more
than just allocating a PCIBus. They do some initialization of
PCIHostState as well. That's why non-root PCI buses can't be
created by pci_bus_new*() today.

Maybe the answer here is to move the PCI_HOST_BRIDGE-specific
code somewhere else, and make pci_bus_new*() more generic. This
would allow us to reuse pci_bus_new*() when creating non-root PCI
buses, later.

-- 
Eduardo

On Wed, Apr 19, 2017 at 09:50:01AM -0300, Eduardo Habkost wrote:
> On Wed, Apr 19, 2017 at 09:41:42AM +0100, Peter Maydell wrote:
> > On 18 April 2017 at 23:17, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > pci_bus_new*() and pci_register_bus() work only when the 'parent'
> > > argument is a PCI_HOST_BRIDGE object. Rename them to reflect that they
> > > are meant to initialize a bus that's in a PCI host bridge.
> > >
> > > The new function names are:
> > > * pci_host_bus_init() (replacing pci_bus_new())
> > > * pci_host_bus_init_inplace() (replacing pci_bus_new_inplace())
> > > * pci_host_bus_init_irqs() (replacing pci_register_bus())
> > 
> > This is moving these functions away from the convention that
> > we seem to mostly follow for buses (eg ISA, SCSI) of
> > foo_bus_new() to allocate and return a new bus, and
> > foo_bus_new_inplace() to initialize a bus that's inline in
> > some other struct.
> > 
> > I'm not sure this is a good idea unless we have a plan to
> > convert all the other QOM buses and document that this is
> > the right way to implement init for a bus.
> 
> The point of the rename is that those functions are doing more
> than just allocating a PCIBus. They do some initialization of
> PCIHostState as well. That's why non-root PCI buses can't be
> created by pci_bus_new*() today.

Hm, yeah.

So.. for what's now pci_register_bus() is it even worth keeping a
helper?  Could we just require that callers call the other init
function then call pci_bus_irqs().  That would be one less name to
come up with.

For the others, what about
	pci_common_root_bus_new()
	pci_common_root_bus_init_inplace()
?

> Maybe the answer here is to move the PCI_HOST_BRIDGE-specific
> code somewhere else, and make pci_bus_new*() more generic. This
> would allow us to reuse pci_bus_new*() when creating non-root PCI
> buses, later.

That might work even better.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson