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

[Eduardo Habkost]

On Wed, Apr 19, 2017 at 09:31:57PM +0300, Marcel Apfelbaum wrote:
> 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?

qbus_create_inplace()/object_initialize() will zero the allocated
memory. I agree that it looks too low-level, I will probably drop
this patch and keep the 3 functions, for the sake of simplicity.

> 
> 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;
> >  }
> > 
> > 
> 

-- 
Eduardo