* [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init()
2017-04-18 22:17 [Qemu-devel] [RFC v2 0/6] pci: Refactor PCI root bus creation code Eduardo Habkost
@ 2017-04-18 22:17 ` Eduardo Habkost
2017-04-19 0:22 ` David Gibson
2017-04-19 18:09 ` Marcel Apfelbaum
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace() Eduardo Habkost
` (4 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-18 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek, Marcel Apfelbaum
There's no need for a separate function just to append an item to
pci_host_bridges.
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 | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 259483b1c0..328f36cd21 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -312,13 +312,6 @@ static void pcibus_reset(BusState *qbus)
}
}
-static void pci_host_bus_register(DeviceState *host)
-{
- PCIHostState *host_bridge = PCI_HOST_BRIDGE(host);
-
- QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
-}
-
PCIBus *pci_find_primary_bus(void)
{
PCIBus *primary_bus = NULL;
@@ -369,6 +362,8 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
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;
@@ -377,7 +372,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
/* host bridge */
QLIST_INIT(&bus->child);
- pci_host_bus_register(parent);
+ QLIST_INSERT_HEAD(&pci_host_bridges, phb, next);
}
bool pci_bus_is_express(PCIBus *bus)
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init()
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init() Eduardo Habkost
@ 2017-04-19 0:22 ` David Gibson
2017-04-19 18:09 ` Marcel Apfelbaum
1 sibling, 0 replies; 22+ messages in thread
From: David Gibson @ 2017-04-19 0:22 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum
[-- Attachment #1: Type: text/plain, Size: 1844 bytes --]
On Tue, Apr 18, 2017 at 07:17:19PM -0300, Eduardo Habkost wrote:
1;4601;0c> There's no need for a separate function just to append an item to
> pci_host_bridges.
>
> 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 | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 259483b1c0..328f36cd21 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -312,13 +312,6 @@ static void pcibus_reset(BusState *qbus)
> }
> }
>
> -static void pci_host_bus_register(DeviceState *host)
> -{
> - PCIHostState *host_bridge = PCI_HOST_BRIDGE(host);
> -
> - QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
> -}
> -
> PCIBus *pci_find_primary_bus(void)
> {
> PCIBus *primary_bus = NULL;
> @@ -369,6 +362,8 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> 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;
> @@ -377,7 +372,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> /* host bridge */
> QLIST_INIT(&bus->child);
>
> - pci_host_bus_register(parent);
> + QLIST_INSERT_HEAD(&pci_host_bridges, phb, next);
> }
>
> bool pci_bus_is_express(PCIBus *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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init()
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init() Eduardo Habkost
2017-04-19 0:22 ` David Gibson
@ 2017-04-19 18:09 ` Marcel Apfelbaum
1 sibling, 0 replies; 22+ messages in thread
From: Marcel Apfelbaum @ 2017-04-19 18:09 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek
On 04/19/2017 01:17 AM, Eduardo Habkost wrote:
> There's no need for a separate function just to append an item to
> pci_host_bridges.
>
> 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 | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 259483b1c0..328f36cd21 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -312,13 +312,6 @@ static void pcibus_reset(BusState *qbus)
> }
> }
>
> -static void pci_host_bus_register(DeviceState *host)
> -{
> - PCIHostState *host_bridge = PCI_HOST_BRIDGE(host);
> -
> - QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
> -}
> -
> PCIBus *pci_find_primary_bus(void)
> {
> PCIBus *primary_bus = NULL;
> @@ -369,6 +362,8 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> 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;
> @@ -377,7 +372,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> /* host bridge */
> QLIST_INIT(&bus->child);
>
> - pci_host_bus_register(parent);
> + QLIST_INSERT_HEAD(&pci_host_bridges, phb, next);
> }
>
> bool pci_bus_is_express(PCIBus *bus)
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace()
2017-04-18 22:17 [Qemu-devel] [RFC v2 0/6] pci: Refactor PCI root bus creation code Eduardo Habkost
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init() Eduardo Habkost
@ 2017-04-18 22:17 ` Eduardo Habkost
2017-04-19 0:23 ` David Gibson
2017-04-19 18:31 ` Marcel Apfelbaum
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions Eduardo Habkost
` (3 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-18 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek, Marcel Apfelbaum
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace()
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace() Eduardo Habkost
@ 2017-04-19 0:23 ` David Gibson
2017-04-19 18:31 ` Marcel Apfelbaum
1 sibling, 0 replies; 22+ messages in thread
From: David Gibson @ 2017-04-19 0:23 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum
[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace()
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace() Eduardo Habkost
2017-04-19 0:23 ` David Gibson
@ 2017-04-19 18:31 ` Marcel Apfelbaum
2017-04-25 19:24 ` Eduardo Habkost
1 sibling, 1 reply; 22+ messages in thread
From: Marcel Apfelbaum @ 2017-04-19 18:31 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek
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;
> }
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace()
2017-04-19 18:31 ` Marcel Apfelbaum
@ 2017-04-25 19:24 ` Eduardo Habkost
0 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-25 19:24 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-18 22:17 [Qemu-devel] [RFC v2 0/6] pci: Refactor PCI root bus creation code Eduardo Habkost
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 1/6] pci: Inline pci_host_bus_register() inside pci_bus_init() Eduardo Habkost
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 2/6] pci: Move pci_bus_init() logic to pci_bus_new_inplace() Eduardo Habkost
@ 2017-04-18 22:17 ` Eduardo Habkost
2017-04-19 0:29 ` David Gibson
2017-04-19 8:41 ` Peter Maydell
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 4/6] pci: Manually simplify QOM casts at pci_host_bus_init*() calls Eduardo Habkost
` (2 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-18 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Cornelia Huck, David Gibson,
Hervé Poussineau, Paul Burton, Peter Maydell,
Richard Henderson, Scott Wood, Yongbok Kim, qemu-arm, qemu-ppc
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions Eduardo Habkost
@ 2017-04-19 0:29 ` David Gibson
2017-04-19 1:42 ` Eduardo Habkost
2017-04-19 8:41 ` Peter Maydell
1 sibling, 1 reply; 22+ messages in thread
From: David Gibson @ 2017-04-19 0:29 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Cornelia Huck, Hervé Poussineau,
Paul Burton, Peter Maydell, Richard Henderson, Scott Wood,
Yongbok Kim, qemu-arm, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 31426 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-19 0:29 ` David Gibson
@ 2017-04-19 1:42 ` Eduardo Habkost
2017-04-19 12:05 ` Cornelia Huck
0 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-19 1:42 UTC (permalink / raw)
To: David Gibson
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Cornelia Huck, Hervé Poussineau,
Paul Burton, Peter Maydell, Richard Henderson, Scott Wood,
Yongbok Kim, qemu-arm, qemu-ppc
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-19 1:42 ` Eduardo Habkost
@ 2017-04-19 12:05 ` Cornelia Huck
2017-04-19 18:41 ` Marcel Apfelbaum
0 siblings, 1 reply; 22+ messages in thread
From: Cornelia Huck @ 2017-04-19 12:05 UTC (permalink / raw)
To: Eduardo Habkost
Cc: David Gibson, qemu-devel, aik, David Gibson, Michael S. Tsirkin,
Laszlo Ersek, Marcel Apfelbaum, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Hervé Poussineau, Paul Burton,
Peter Maydell, Richard Henderson, Scott Wood, Yongbok Kim,
qemu-arm, qemu-ppc
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)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-19 12:05 ` Cornelia Huck
@ 2017-04-19 18:41 ` Marcel Apfelbaum
2017-04-19 21:19 ` Eduardo Habkost
0 siblings, 1 reply; 22+ messages in thread
From: Marcel Apfelbaum @ 2017-04-19 18:41 UTC (permalink / raw)
To: Cornelia Huck, Eduardo Habkost
Cc: David Gibson, qemu-devel, aik, David Gibson, Michael S. Tsirkin,
Laszlo Ersek, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Hervé Poussineau, Paul Burton,
Peter Maydell, Richard Henderson, Scott Wood, Yongbok Kim,
qemu-arm, qemu-ppc
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)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-19 18:41 ` Marcel Apfelbaum
@ 2017-04-19 21:19 ` Eduardo Habkost
0 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-19 21:19 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: Cornelia Huck, David Gibson, qemu-devel, aik, David Gibson,
Michael S. Tsirkin, Laszlo Ersek, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Hervé Poussineau, Paul Burton,
Peter Maydell, Richard Henderson, Scott Wood, Yongbok Kim,
qemu-arm, qemu-ppc
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions Eduardo Habkost
2017-04-19 0:29 ` David Gibson
@ 2017-04-19 8:41 ` Peter Maydell
2017-04-19 12:50 ` Eduardo Habkost
1 sibling, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2017-04-19 8:41 UTC (permalink / raw)
To: Eduardo Habkost
Cc: QEMU Developers, Alexey Kardashevskiy, David Gibson,
Michael S. Tsirkin, Laszlo Ersek, Marcel Apfelbaum,
Alexander Graf, Aurelien Jarno, Christian Borntraeger,
Cornelia Huck, David Gibson, Hervé Poussineau, Paul Burton,
Richard Henderson, Scott Wood, Yongbok Kim, qemu-arm, qemu-ppc
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-19 8:41 ` Peter Maydell
@ 2017-04-19 12:50 ` Eduardo Habkost
2017-04-20 5:04 ` David Gibson
0 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-19 12:50 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Alexey Kardashevskiy, David Gibson,
Michael S. Tsirkin, Laszlo Ersek, Marcel Apfelbaum,
Alexander Graf, Aurelien Jarno, Christian Borntraeger,
Cornelia Huck, David Gibson, Hervé Poussineau, Paul Burton,
Richard Henderson, Scott Wood, Yongbok Kim, qemu-arm, qemu-ppc
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions
2017-04-19 12:50 ` Eduardo Habkost
@ 2017-04-20 5:04 ` David Gibson
0 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2017-04-20 5:04 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Peter Maydell, QEMU Developers, Alexey Kardashevskiy,
David Gibson, Michael S. Tsirkin, Laszlo Ersek, Marcel Apfelbaum,
Alexander Graf, Aurelien Jarno, Christian Borntraeger,
Cornelia Huck, Hervé Poussineau, Paul Burton,
Richard Henderson, Scott Wood, Yongbok Kim, qemu-arm, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC v2 4/6] pci: Manually simplify QOM casts at pci_host_bus_init*() calls
2017-04-18 22:17 [Qemu-devel] [RFC v2 0/6] pci: Refactor PCI root bus creation code Eduardo Habkost
` (2 preceding siblings ...)
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 3/6] pci: Rename and change signatures of pci_bus_new() & related functions Eduardo Habkost
@ 2017-04-18 22:17 ` Eduardo Habkost
2017-04-19 0:30 ` David Gibson
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 5/6] pci: Set phb->bus inside pci_host_bus_init_inplace() Eduardo Habkost
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 6/6] pci: Remove unnecessary PCIBus variables Eduardo Habkost
5 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-18 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Alexander Graf, David Gibson, Aurelien Jarno,
qemu-ppc
Those redundant casts were not detected by the Coccinelle patch because
there are multiple variables and casts involved. Fix them manually.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/pci-host/q35.c | 2 +-
hw/pci-host/uninorth.c | 2 +-
hw/ppc/spapr_pci.c | 2 +-
hw/sh4/sh_pci.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 947dc3f124..4258076979 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -49,7 +49,7 @@ 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_host_bus_init(PCI_HOST_BRIDGE(s), "pcie.0",
+ pci->bus = pci_host_bus_init(pci, "pcie.0",
s->mch.pci_address_space,
s->mch.address_space_io, 0, TYPE_PCIE_BUS);
PC_MACHINE(qdev_get_machine())->bus = pci->bus;
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 079faad6ff..d9fb5fdc93 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -233,7 +233,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- h->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
+ 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);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7f29cc77b0..ce132c5eea 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1697,7 +1697,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
&sphb->iowindow);
- bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
+ bus = pci_host_bus_init_irqs(phb, NULL,
pci_spapr_set_irq, pci_spapr_map_irq, sphb,
&sphb->memspace, &sphb->iospace,
PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index f589b1628e..8be2e830e9 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -131,7 +131,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
for (i = 0; i < 4; i++) {
sysbus_init_irq(dev, &s->irq[i]);
}
- phb->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), "pci",
+ phb->bus = pci_host_bus_init_irqs(phb, "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);
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 4/6] pci: Manually simplify QOM casts at pci_host_bus_init*() calls
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 4/6] pci: Manually simplify QOM casts at pci_host_bus_init*() calls Eduardo Habkost
@ 2017-04-19 0:30 ` David Gibson
0 siblings, 0 replies; 22+ messages in thread
From: David Gibson @ 2017-04-19 0:30 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Alexander Graf, Aurelien Jarno, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 3710 bytes --]
On Tue, Apr 18, 2017 at 07:17:22PM -0300, Eduardo Habkost wrote:
> Those redundant casts were not detected by the Coccinelle patch because
> there are multiple variables and casts involved. Fix them manually.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/pci-host/q35.c | 2 +-
> hw/pci-host/uninorth.c | 2 +-
> hw/ppc/spapr_pci.c | 2 +-
> hw/sh4/sh_pci.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 947dc3f124..4258076979 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -49,7 +49,7 @@ 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_host_bus_init(PCI_HOST_BRIDGE(s), "pcie.0",
> + pci->bus = pci_host_bus_init(pci, "pcie.0",
> s->mch.pci_address_space,
> s->mch.address_space_io, 0, TYPE_PCIE_BUS);
> PC_MACHINE(qdev_get_machine())->bus = pci->bus;
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index 079faad6ff..d9fb5fdc93 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -233,7 +233,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
> memory_region_add_subregion(address_space_mem, 0x80000000ULL,
> &d->pci_hole);
>
> - h->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
> + 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);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7f29cc77b0..ce132c5eea 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1697,7 +1697,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
> &sphb->iowindow);
>
> - bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), NULL,
> + bus = pci_host_bus_init_irqs(phb, NULL,
> pci_spapr_set_irq, pci_spapr_map_irq, sphb,
> &sphb->memspace, &sphb->iospace,
> PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index f589b1628e..8be2e830e9 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -131,7 +131,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
> for (i = 0; i < 4; i++) {
> sysbus_init_irq(dev, &s->irq[i]);
> }
> - phb->bus = pci_host_bus_init_irqs(PCI_HOST_BRIDGE(dev), "pci",
> + phb->bus = pci_host_bus_init_irqs(phb, "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);
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC v2 5/6] pci: Set phb->bus inside pci_host_bus_init_inplace()
2017-04-18 22:17 [Qemu-devel] [RFC v2 0/6] pci: Refactor PCI root bus creation code Eduardo Habkost
` (3 preceding siblings ...)
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 4/6] pci: Manually simplify QOM casts at pci_host_bus_init*() calls Eduardo Habkost
@ 2017-04-18 22:17 ` Eduardo Habkost
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 6/6] pci: Remove unnecessary PCIBus variables Eduardo Habkost
5 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-18 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Alexander Graf, Aurelien Jarno,
Christian Borntraeger, Cornelia Huck, David Gibson,
Hervé Poussineau, Paul Burton, Peter Maydell,
Richard Henderson, Scott Wood, Yongbok Kim, qemu-arm, qemu-ppc
Every single caller of pci_host_bus_init*() sets phb->bus to point to
'bus'. Do that inside pci_bus_new_inplace() to avoid code duplication
and make it harder to break.
Generated using the following Coccinelle patch:
@@
identifier b;
expression phb;
expression list ARGS;
@@
-b = pci_host_bus_init(phb, ARGS);
+pci_host_bus_init(phb, ARGS);
+b = phb->bus;
...
-phb->bus = b;
@@
expression phb;
expression list ARGS;
@@
-phb->bus = pci_host_bus_init(phb, ARGS);
+pci_host_bus_init(phb, ARGS);
@@
expression PHB, BUS;
expression list ARGS;
@@
pci_host_bus_init_inplace(PHB, BUS, ARGS);
...
-PHB->bus = BUS;
@@
typedef PCIHostState;
typedef PCIBus;
identifier phb, BUS;
@@
void pci_host_bus_init_inplace(PCIHostState *phb, PCIBus *BUS, ...)
{
...
+ phb->bus = BUS;
}
@@
identifier b;
expression phb;
expression list ARGS;
@@
-b = pci_host_bus_init_irqs(phb, ARGS);
+pci_host_bus_init_irqs(phb, ARGS);
+b = phb->bus;
...
-phb->bus = b;
@@
expression phb;
expression list ARGS;
@@
-phb->bus = pci_host_bus_init_irqs(phb, ARGS);
+pci_host_bus_init_irqs(phb, ARGS);
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>
---
hw/alpha/typhoon.c | 8 ++++----
hw/mips/gt64xxx_pci.c | 8 +++-----
hw/pci-bridge/pci_expander_bridge.c | 12 ++++++------
hw/pci-host/apb.c | 6 ++----
hw/pci-host/bonito.c | 7 +++----
hw/pci-host/gpex.c | 7 +++----
hw/pci-host/grackle.c | 7 +++----
hw/pci-host/piix.c | 6 +++---
hw/pci-host/ppce500.c | 9 ++++-----
hw/pci-host/prep.c | 2 --
hw/pci-host/q35.c | 5 ++---
hw/pci-host/uninorth.c | 14 ++++++--------
hw/pci-host/versatile.c | 1 -
hw/pci-host/xilinx-pcie.c | 7 +++----
hw/pci/pci.c | 2 +-
hw/ppc/ppc4xx_pci.c | 9 ++++-----
hw/ppc/spapr_pci.c | 9 ++++-----
hw/s390x/s390-pci-bus.c | 9 ++++-----
hw/sh4/sh_pci.c | 7 +++----
19 files changed, 58 insertions(+), 77 deletions(-)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 24f1959ab8..a35fac5381 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -883,10 +883,10 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
memory_region_add_subregion(addr_space, 0x801fc000000ULL,
&s->pchip.reg_io);
- 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;
+ 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);
+ b = phb->bus;
qdev_init_nofail(dev);
/* Host memory as seen from the PCI side, via the IOMMU. */
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 5ca9f07031..428d5e2dd9 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1171,11 +1171,9 @@ 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_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);
+ 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 853448ef70..e88b0aa682 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -230,11 +230,13 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
ds = qdev_create(NULL, TYPE_PXB_HOST);
if (pcie) {
- bus = pci_host_bus_init(PCI_HOST_BRIDGE(ds), dev_name, NULL, NULL, 0,
- TYPE_PXB_PCIE_BUS);
+ pci_host_bus_init(PCI_HOST_BRIDGE(ds), dev_name, NULL, NULL, 0,
+ TYPE_PXB_PCIE_BUS);
+ bus = PCI_HOST_BRIDGE(ds)->bus;
} else {
- bus = pci_host_bus_init(PCI_HOST_BRIDGE(ds), "pxb-internal", NULL,
- NULL, 0, TYPE_PXB_BUS);
+ pci_host_bus_init(PCI_HOST_BRIDGE(ds), "pxb-internal", NULL, NULL, 0,
+ TYPE_PXB_BUS);
+ bus = PCI_HOST_BRIDGE(ds)->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);
@@ -246,8 +248,6 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp)
bus->address_space_io = dev->bus->address_space_io;
bus->map_irq = pxb_map_irq_fn;
- PCI_HOST_BRIDGE(ds)->bus = bus;
-
pxb_register_bus(dev, bus, &local_err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 38d08c661c..ebcb93d9ac 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -671,10 +671,8 @@ 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_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);
+ 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 8b6e80aa82..ff3db73900 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -714,10 +714,9 @@ static int bonito_pcihost_initfn(SysBusDevice *dev)
{
PCIHostState *phb = PCI_HOST_BRIDGE(dev);
- 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);
+ 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 9ea9927cf8..9e5d2e2dc4 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -62,10 +62,9 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
sysbus_init_irq(sbd, &s->irq[i]);
}
- 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);
+ 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 0844c30d9a..b3126852de 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -82,10 +82,9 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- 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_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 364d57039b..ec5a39706e 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -340,9 +340,9 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
dev = qdev_create(NULL, host_type);
s = PCI_HOST_BRIDGE(dev);
- b = pci_host_bus_init(s, NULL, pci_address_space,
- address_space_io, 0, TYPE_PCI_BUS);
- s->bus = b;
+ pci_host_bus_init(s, NULL, pci_address_space, address_space_io, 0,
+ TYPE_PCI_BUS);
+ b = s->bus;
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 babb12af31..d7091709d2 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -465,11 +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_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;
+ 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);
+ b = h->bus;
/* Set up PCI view of memory */
memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX);
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 65add936c7..da151fc051 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -285,8 +285,6 @@ static void raven_pcihost_initfn(Object *obj)
address_space_init(&s->bm_as, &s->bm, "raven-bm");
pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
- h->bus = &s->pci_bus;
-
object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_RAVEN_PCI_DEVICE);
pci_dev = DEVICE(&s->pci_dev);
qdev_set_parent_bus(pci_dev, BUS(&s->pci_bus));
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 4258076979..c285d2caca 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -49,9 +49,8 @@ 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_host_bus_init(pci, "pcie.0",
- s->mch.pci_address_space,
- s->mch.address_space_io, 0, TYPE_PCIE_BUS);
+ pci_host_bus_init(pci, "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 d9fb5fdc93..febabeaec7 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -233,10 +233,9 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- 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);
+ 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);
#if 0
pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north");
@@ -297,10 +296,9 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- 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);
+ 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 cad8848723..caf7f6407a 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -390,7 +390,6 @@ static void pci_vpb_init(Object *obj)
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);
qdev_set_parent_bus(DEVICE(&s->pci_dev), BUS(&s->pci_bus));
diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 0a3d19b22c..f988c7e58e 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -129,10 +129,9 @@ 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_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);
+ 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 37d65eaf7e..dd065a0881 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -384,7 +384,7 @@ void pci_host_bus_init_inplace(PCIHostState *phb, PCIBus *bus,
QLIST_INIT(&bus->child);
QLIST_INSERT_HEAD(&pci_host_bridges, phb, next);
-
+ phb->bus = bus;
}
PCIBus *pci_host_bus_init(PCIHostState *phb, const char *name,
diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
index 0d767a1a23..992be1bce9 100644
--- a/hw/ppc/ppc4xx_pci.c
+++ b/hw/ppc/ppc4xx_pci.c
@@ -314,11 +314,10 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
sysbus_init_irq(dev, &s->irq[i]);
}
- 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_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);
+ b = h->bus;
pci_create_simple(b, 0, "ppc4xx-host-bridge");
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ce132c5eea..06e8a8740b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1697,11 +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_host_bus_init_irqs(phb, 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;
+ pci_host_bus_init_irqs(phb, 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 = phb->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 267e1de510..5e174e90f4 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -560,15 +560,14 @@ static int s390_pcihost_init(SysBusDevice *dev)
DPRINTF("host_init\n");
- 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_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);
+ b = phb->bus;
pci_setup_iommu(b, s390_pci_dma_iommu, s);
bus = BUS(b);
qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
- phb->bus = b;
s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, DEVICE(s), NULL));
qbus_set_hotplug_handler(BUS(s->bus), DEVICE(s), NULL);
diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index 8be2e830e9..5745738fb9 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -131,10 +131,9 @@ static int sh_pci_device_init(SysBusDevice *dev)
for (i = 0; i < 4; i++) {
sysbus_init_irq(dev, &s->irq[i]);
}
- phb->bus = pci_host_bus_init_irqs(phb, "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);
+ pci_host_bus_init_irqs(phb, "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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [RFC v2 6/6] pci: Remove unnecessary PCIBus variables
2017-04-18 22:17 [Qemu-devel] [RFC v2 0/6] pci: Refactor PCI root bus creation code Eduardo Habkost
` (4 preceding siblings ...)
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 5/6] pci: Set phb->bus inside pci_host_bus_init_inplace() Eduardo Habkost
@ 2017-04-18 22:17 ` Eduardo Habkost
2017-04-19 12:20 ` Cornelia Huck
5 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2017-04-18 22:17 UTC (permalink / raw)
To: qemu-devel
Cc: aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Richard Henderson, Alexander Graf, Scott Wood,
David Gibson, Cornelia Huck, Christian Borntraeger, qemu-ppc
"phb->bus" is a short expression, there's no need for an extra variable
just to hold its value.
Generated using the following Coccinelle patch:
@@
identifier b;
identifier phb;
typedef PCIBus;
@@
-PCIBus *b;
... when != b
-b = phb->bus;
<...
-b
+phb->bus
...>
Cc: Richard Henderson <rth@twiddle.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Scott Wood <scottwood@freescale.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/alpha/typhoon.c | 10 ++++------
hw/pci-bridge/pci_expander_bridge.c | 4 +---
hw/pci-host/piix.c | 26 +++++++++++++-------------
hw/pci-host/ppce500.c | 8 +++-----
hw/ppc/ppc4xx_pci.c | 4 +---
hw/ppc/spapr_pci.c | 6 ++----
hw/s390x/s390-pci-bus.c | 6 ++----
7 files changed, 26 insertions(+), 38 deletions(-)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index a35fac5381..051de095e0 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -820,7 +820,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
DeviceState *dev;
TyphoonState *s;
PCIHostState *phb;
- PCIBus *b;
int i;
dev = qdev_create(NULL, TYPE_TYPHOON_PCI_HOST_BRIDGE);
@@ -886,24 +885,23 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
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);
- b = phb->bus;
qdev_init_nofail(dev);
/* Host memory as seen from the PCI side, via the IOMMU. */
memory_region_init_iommu(&s->pchip.iommu, OBJECT(s), &typhoon_iommu_ops,
"iommu-typhoon", UINT64_MAX);
address_space_init(&s->pchip.iommu_as, &s->pchip.iommu, "pchip0-pci");
- pci_setup_iommu(b, typhoon_pci_dma_iommu, s);
+ pci_setup_iommu(phb->bus, typhoon_pci_dma_iommu, s);
/* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */
memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), &alpha_pci_iack_ops,
- b, "pci0-iack", 64*MB);
+ phb->bus, "pci0-iack", 64 * MB);
memory_region_add_subregion(addr_space, 0x801f8000000ULL,
&s->pchip.reg_iack);
/* Pchip0 PCI configuration, 0x801.FE00.0000, 16MB. */
memory_region_init_io(&s->pchip.reg_conf, OBJECT(s), &alpha_pci_conf1_ops,
- b, "pci0-conf", 16*MB);
+ phb->bus, "pci0-conf", 16 * MB);
memory_region_add_subregion(addr_space, 0x801fe000000ULL,
&s->pchip.reg_conf);
@@ -928,7 +926,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
isa_bus_irqs(*isa_bus, isa_irqs);
}
- return b;
+ return phb->bus;
}
static int typhoon_pcihost_init(SysBusDevice *dev)
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index e88b0aa682..bdd03f661a 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -114,7 +114,6 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
{
const PCIHostState *pxb_host;
- const PCIBus *pxb_bus;
const PXBDev *pxb_dev;
int position;
const DeviceState *pxb_dev_base;
@@ -122,8 +121,7 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
const SysBusDevice *main_host_sbd;
pxb_host = PCI_HOST_BRIDGE(dev);
- pxb_bus = pxb_host->bus;
- pxb_dev = convert_to_pxb(pxb_bus->parent_dev);
+ pxb_dev = convert_to_pxb(pxb_host->bus->parent_dev);
position = g_list_index(pxb_dev_list, pxb_dev);
assert(position >= 0);
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index ec5a39706e..7b623f5d1b 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -330,7 +330,6 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
MemoryRegion *ram_memory)
{
DeviceState *dev;
- PCIBus *b;
PCIDevice *d;
PCIHostState *s;
PIIX3State *piix3;
@@ -342,11 +341,10 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
s = PCI_HOST_BRIDGE(dev);
pci_host_bus_init(s, NULL, pci_address_space, address_space_io, 0,
TYPE_PCI_BUS);
- b = s->bus;
object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
qdev_init_nofail(dev);
- d = pci_create_simple(b, 0, pci_type);
+ d = pci_create_simple(s->bus, 0, pci_type);
*pi440fx_state = I440FX_PCI_DEVICE(d);
f = *pi440fx_state;
f->system_memory = address_space_mem;
@@ -391,18 +389,20 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
* connected to the IOAPIC directly.
* These additional routes can be discovered through ACPI. */
if (xen_enabled()) {
- PCIDevice *pci_dev = pci_create_simple_multifunction(b,
- -1, true, "PIIX3-xen");
+ PCIDevice *pci_dev = pci_create_simple_multifunction(s->bus,
+ -1, true,
+ "PIIX3-xen");
piix3 = PIIX3_PCI_DEVICE(pci_dev);
- pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
- piix3, XEN_PIIX_NUM_PIRQS);
+ pci_bus_irqs(s->bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+ piix3, XEN_PIIX_NUM_PIRQS);
} else {
- PCIDevice *pci_dev = pci_create_simple_multifunction(b,
- -1, true, "PIIX3");
+ PCIDevice *pci_dev = pci_create_simple_multifunction(s->bus,
+ -1, true,
+ "PIIX3");
piix3 = PIIX3_PCI_DEVICE(pci_dev);
- pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
- PIIX_NUM_PIRQS);
- pci_bus_set_route_irq_fn(b, piix3_route_intx_pin_to_irq);
+ pci_bus_irqs(s->bus, piix3_set_irq, pci_slot_get_pirq, piix3,
+ PIIX_NUM_PIRQS);
+ pci_bus_set_route_irq_fn(s->bus, piix3_route_intx_pin_to_irq);
}
piix3->pic = pic;
*isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
@@ -417,7 +417,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
i440fx_update_memory_mappings(f);
- return b;
+ return s->bus;
}
PCIBus *find_i440fx(void)
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index d7091709d2..24decdbbbc 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -445,7 +445,6 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
{
PCIHostState *h;
PPCE500PCIState *s;
- PCIBus *b;
int i;
h = PCI_HOST_BRIDGE(dev);
@@ -468,15 +467,14 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
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);
- b = h->bus;
/* Set up PCI view of memory */
memory_region_init(&s->bm, OBJECT(s), "bm-e500", UINT64_MAX);
memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
address_space_init(&s->bm_as, &s->bm, "pci-bm");
- pci_setup_iommu(b, e500_pcihost_set_iommu, s);
+ pci_setup_iommu(h->bus, e500_pcihost_set_iommu, s);
- pci_create_simple(b, 0, "e500-host-bridge");
+ pci_create_simple(h->bus, 0, "e500-host-bridge");
memory_region_init(&s->container, OBJECT(h), "pci-container", PCIE500_ALL_SIZE);
memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, h,
@@ -489,7 +487,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
memory_region_add_subregion(&s->container, PCIE500_CFGDATA, &h->data_mem);
memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
sysbus_init_mmio(dev, &s->container);
- pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
+ pci_bus_set_route_irq_fn(h->bus, e500_route_intx_pin_to_irq);
return 0;
}
diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
index 992be1bce9..80516a387c 100644
--- a/hw/ppc/ppc4xx_pci.c
+++ b/hw/ppc/ppc4xx_pci.c
@@ -304,7 +304,6 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
{
PPC4xxPCIState *s;
PCIHostState *h;
- PCIBus *b;
int i;
h = PCI_HOST_BRIDGE(dev);
@@ -317,9 +316,8 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
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);
- b = h->bus;
- pci_create_simple(b, 0, "ppc4xx-host-bridge");
+ pci_create_simple(h->bus, 0, "ppc4xx-host-bridge");
/* XXX split into 2 memory regions, one for config space, one for regs */
memory_region_init(&s->container, OBJECT(s), "pci-container", PCI_ALL_SIZE);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 06e8a8740b..b1ce50427c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1570,7 +1570,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
PCIHostState *phb = PCI_HOST_BRIDGE(s);
char *namebuf;
int i;
- PCIBus *bus;
uint64_t msi_window_size = 4096;
sPAPRTCETable *tcet;
const unsigned windows_supported =
@@ -1700,7 +1699,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
pci_host_bus_init_irqs(phb, 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 = phb->bus;
qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL);
/*
@@ -1739,9 +1737,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(&sphb->iommu_root, SPAPR_PCI_MSI_WINDOW,
&sphb->msiwindow);
- pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
+ pci_setup_iommu(phb->bus, spapr_pci_dma_iommu, sphb);
- pci_bus_set_route_irq_fn(bus, spapr_route_intx_pin_to_irq);
+ pci_bus_set_route_irq_fn(phb->bus, spapr_route_intx_pin_to_irq);
QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 5e174e90f4..c2776ba221 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -553,7 +553,6 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn)
static int s390_pcihost_init(SysBusDevice *dev)
{
- PCIBus *b;
BusState *bus;
PCIHostState *phb = PCI_HOST_BRIDGE(dev);
S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
@@ -563,10 +562,9 @@ static int s390_pcihost_init(SysBusDevice *dev)
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);
- b = phb->bus;
- pci_setup_iommu(b, s390_pci_dma_iommu, s);
+ pci_setup_iommu(phb->bus, s390_pci_dma_iommu, s);
- bus = BUS(b);
+ bus = BUS(phb->bus);
qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, DEVICE(s), NULL));
--
2.11.0.259.g40922b1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [RFC v2 6/6] pci: Remove unnecessary PCIBus variables
2017-04-18 22:17 ` [Qemu-devel] [RFC v2 6/6] pci: Remove unnecessary PCIBus variables Eduardo Habkost
@ 2017-04-19 12:20 ` Cornelia Huck
0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2017-04-19 12:20 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, aik, David Gibson, Michael S. Tsirkin, Laszlo Ersek,
Marcel Apfelbaum, Richard Henderson, Alexander Graf, Scott Wood,
David Gibson, Christian Borntraeger, qemu-ppc
On Tue, 18 Apr 2017 19:17:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> "phb->bus" is a short expression, there's no need for an extra variable
> just to hold its value.
>
> Generated using the following Coccinelle patch:
>
> @@
> identifier b;
> identifier phb;
> typedef PCIBus;
> @@
> -PCIBus *b;
> ... when != b
> -b = phb->bus;
> <...
> -b
> +phb->bus
> ...>
>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/alpha/typhoon.c | 10 ++++------
> hw/pci-bridge/pci_expander_bridge.c | 4 +---
> hw/pci-host/piix.c | 26 +++++++++++++-------------
> hw/pci-host/ppce500.c | 8 +++-----
> hw/ppc/ppc4xx_pci.c | 4 +---
> hw/ppc/spapr_pci.c | 6 ++----
> hw/s390x/s390-pci-bus.c | 6 ++----
> 7 files changed, 26 insertions(+), 38 deletions(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 5e174e90f4..c2776ba221 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -553,7 +553,6 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn)
>
> static int s390_pcihost_init(SysBusDevice *dev)
> {
> - PCIBus *b;
> BusState *bus;
> PCIHostState *phb = PCI_HOST_BRIDGE(dev);
> S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
> @@ -563,10 +562,9 @@ static int s390_pcihost_init(SysBusDevice *dev)
> 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);
> - b = phb->bus;
> - pci_setup_iommu(b, s390_pci_dma_iommu, s);
> + pci_setup_iommu(phb->bus, s390_pci_dma_iommu, s);
>
> - bus = BUS(b);
> + bus = BUS(phb->bus);
This BusState variable is equally unnecessary, as is it just used in
the line below. But that is outside the scope of that simple coccinelle
patch, so
> qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
>
> s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, DEVICE(s), NULL));
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
for the s390 part.
^ permalink raw reply [flat|nested] 22+ messages in thread