Re: [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace()

[Marcel Apfelbaum]

On 04/19/2017 01:17 AM, Eduardo Habkost wrote:
> Instead of having 3 separate functions, just make pci_bus_new() a
> wrapper that allocates the object and calls pci_bus_new_inplace().
>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Cc: Marcel Apfelbaum <address@hidden>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  hw/pci/pci.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 328f36cd21..0d28ee4e3f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -357,24 +357,6 @@ const char *pci_root_bus_path(PCIDevice *dev)
>      return rootbus->qbus.name;
>  }
>
> -static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> -                         MemoryRegion *address_space_mem,
> -                         MemoryRegion *address_space_io,
> -                         uint8_t devfn_min)
> -{
> -    PCIHostState *phb = PCI_HOST_BRIDGE(parent);
> -
> -    assert(PCI_FUNC(devfn_min) == 0);
> -    bus->devfn_min = devfn_min;
> -    bus->address_space_mem = address_space_mem;
> -    bus->address_space_io = address_space_io;
> -
> -    /* host bridge */
> -    QLIST_INIT(&bus->child);
> -
> -    QLIST_INSERT_HEAD(&pci_host_bridges, phb, next);
> -}
> -
>  bool pci_bus_is_express(PCIBus *bus)
>  {
>      return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
> @@ -391,8 +373,20 @@ void pci_bus_new_inplace(PCIBus *bus, size_t bus_size, 
> DeviceState *parent,
>                           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);
> -    pci_bus_init(bus, parent, address_space_mem, address_space_io, devfn_min);
> +
> +    assert(PCI_FUNC(devfn_min) == 0);
> +    bus->devfn_min = devfn_min;
> +    bus->address_space_mem = address_space_mem;
> +    bus->address_space_io = address_space_io;
> +
> +    /* host bridge */
> +    QLIST_INIT(&bus->child);
> +
> +    QLIST_INSERT_HEAD(&pci_host_bridges, phb, next);
> +
>  }
>
>  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> @@ -400,10 +394,11 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>                      MemoryRegion *address_space_io,
>                      uint8_t devfn_min, const char *typename)
>  {
> -    PCIBus *bus;
> +    size_t bus_size = object_type_get_instance_size(typename);
> +    PCIBus *bus = g_malloc(bus_size);

I am not sure what is the "win" here. Maybe because I don't personally
like the 2 "low level" lines above. Maybe we have some QOM primitive to do that 
for us?
At least we should use g_malloc0?

Thanks,
Marcel

> -    bus = PCI_BUS(qbus_create(typename, parent, name));
> -    pci_bus_init(bus, parent, address_space_mem, address_space_io, devfn_min);
> +    pci_bus_new_inplace(bus, bus_size, parent, name, address_space_mem,
> +                        address_space_io, devfn_min, typename);
>      return bus;
>  }