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" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
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);
- 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;
}
--
2.11.0.259.g40922b1
On Tue, Apr 18, 2017 at 07:17:20PM -0300, 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" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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);
>
> - 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;
> }
>
--
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 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" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> 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;
> }
>
>
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" <mst@redhat.com>
> > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > 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
© 2016 - 2026 Red Hat, Inc.