[Qemu-devel] [PATCH 0/2] pci: split out bridge code into pci_bridge

[Qemu-devel] [PATCH 0/2] pci: split out bridge code into pci_bridge

[Isaku Yamahata]

Now pci.c has grown. So split bridge related code into dedicated file
for further extension to pci bridge. Further clean up and pcie port emulator.
This make patch conflict less possible in future.

Isaku Yamahata (2):
  pci/bridge: allocate PCIBus dynamically for PCIBridge.
  pci/bridge: split out pci bridge code into pci_bridge.c from pci.c

 Makefile.objs   |    2 +-
 hw/apb_pci.c    |    1 +
 hw/dec_pci.c    |    1 +
 hw/pci.c        |  177 ++++-------------------------------------------------
 hw/pci.h        |    9 ++-
 hw/pci_bridge.c |  184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci_bridge.h |   41 ++++++++++++
 7 files changed, 247 insertions(+), 168 deletions(-)
 create mode 100644 hw/pci_bridge.c
 create mode 100644 hw/pci_bridge.h

[Qemu-devel] [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Isaku Yamahata]

allocate PCIBus dynamically for PCIBridge and bug fix of
pci_unregister_secondary_bus().
This is a preparation for splitting out pci_bridge functions.
Since PCIBus is private to pci.c, PCIBridge won't be able to
contain PCIBus in its structure.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 08652e8..fdf02d0 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -286,23 +286,27 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     return bus;
 }
 
-static void pci_register_secondary_bus(PCIBus *parent,
-                                       PCIBus *bus,
-                                       PCIDevice *dev,
-                                       pci_map_irq_fn map_irq,
-                                       const char *name)
+static PCIBus *pci_register_secondary_bus(PCIBus *parent,
+                                          PCIDevice *dev,
+                                          pci_map_irq_fn map_irq,
+                                          const char *name)
 {
-    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
+    PCIBus *bus;
+    bus = pci_bus_new(&dev->qdev, name, 0);
+
     bus->map_irq = map_irq;
     bus->parent_dev = dev;
 
     QLIST_INSERT_HEAD(&parent->child, bus, sibling);
+
+    return bus;
 }
 
 static void pci_unregister_secondary_bus(PCIBus *bus)
 {
     assert(QLIST_EMPTY(&bus->child));
     QLIST_REMOVE(bus, sibling);
+    qbus_free(&bus->qbus);
 }
 
 int pci_bus_num(PCIBus *s)
@@ -1527,7 +1531,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
 
 typedef struct {
     PCIDevice dev;
-    PCIBus bus;
+    PCIBus *bus;
     uint32_t vid;
     uint32_t did;
 } PCIBridge;
@@ -1628,8 +1632,7 @@ static int pci_bridge_initfn(PCIDevice *dev)
 static int pci_bridge_exitfn(PCIDevice *pci_dev)
 {
     PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
-    PCIBus *bus = &s->bus;
-    pci_unregister_secondary_bus(bus);
+    pci_unregister_secondary_bus(s->bus);
     return 0;
 }
 
@@ -1646,8 +1649,8 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
     qdev_init_nofail(&dev->qdev);
 
     s = DO_UPCAST(PCIBridge, dev, dev);
-    pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name);
-    return &s->bus;
+    s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
+    return s->bus;
 }
 
 PCIDevice *pci_bridge_get_device(PCIBus *bus)
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Michael S. Tsirkin]

On Fri, Jul 02, 2010 at 11:30:11AM +0900, Isaku Yamahata wrote:
> allocate PCIBus dynamically for PCIBridge and bug fix of
> pci_unregister_secondary_bus().

could you make the bugfix a separate patch please?

> This is a preparation for splitting out pci_bridge functions.
> Since PCIBus is private to pci.c, PCIBridge won't be able to
> contain PCIBus in its structure.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

I think this becomes too complex: as bridge configuration affects
the bus operation, you might end up sticking a pointer to the device
in the bus. A similar arrangement is in place in with piix_pci, and I would
love to get rid of it, too.

Let's just put PCIBus in a header? It could be a new header
named pci_internals.h or something like this.

> ---
>  hw/pci.c |   25 ++++++++++++++-----------
>  1 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 08652e8..fdf02d0 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -286,23 +286,27 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>      return bus;
>  }
>  
> -static void pci_register_secondary_bus(PCIBus *parent,
> -                                       PCIBus *bus,
> -                                       PCIDevice *dev,
> -                                       pci_map_irq_fn map_irq,
> -                                       const char *name)
> +static PCIBus *pci_register_secondary_bus(PCIBus *parent,
> +                                          PCIDevice *dev,
> +                                          pci_map_irq_fn map_irq,
> +                                          const char *name)
>  {
> -    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
> +    PCIBus *bus;
> +    bus = pci_bus_new(&dev->qdev, name, 0);
> +
>      bus->map_irq = map_irq;
>      bus->parent_dev = dev;
>  
>      QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> +
> +    return bus;

This does more than we need: pci_bus_new
was created for host bus so it will also register in
reset and vmstate lists.

>  }
>  
>  static void pci_unregister_secondary_bus(PCIBus *bus)
>  {
>      assert(QLIST_EMPTY(&bus->child));
>      QLIST_REMOVE(bus, sibling);
> +    qbus_free(&bus->qbus);
>  }
>  
>  int pci_bus_num(PCIBus *s)
> @@ -1527,7 +1531,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
>  
>  typedef struct {
>      PCIDevice dev;
> -    PCIBus bus;
> +    PCIBus *bus;
>      uint32_t vid;
>      uint32_t did;
>  } PCIBridge;
> @@ -1628,8 +1632,7 @@ static int pci_bridge_initfn(PCIDevice *dev)
>  static int pci_bridge_exitfn(PCIDevice *pci_dev)
>  {
>      PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
> -    PCIBus *bus = &s->bus;
> -    pci_unregister_secondary_bus(bus);
> +    pci_unregister_secondary_bus(s->bus);
>      return 0;
>  }
>  
> @@ -1646,8 +1649,8 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
>      qdev_init_nofail(&dev->qdev);
>  
>      s = DO_UPCAST(PCIBridge, dev, dev);
> -    pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name);
> -    return &s->bus;
> +    s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
> +    return s->bus;
>  }
>  
>  PCIDevice *pci_bridge_get_device(PCIBus *bus)
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Isaku Yamahata]

On Tue, Jul 06, 2010 at 03:18:52PM +0300, Michael S. Tsirkin wrote:
> On Fri, Jul 02, 2010 at 11:30:11AM +0900, Isaku Yamahata wrote:
> > allocate PCIBus dynamically for PCIBridge and bug fix of
> > pci_unregister_secondary_bus().
> 
> could you make the bugfix a separate patch please?

Will do.


> > This is a preparation for splitting out pci_bridge functions.
> > Since PCIBus is private to pci.c, PCIBridge won't be able to
> > contain PCIBus in its structure.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> I think this becomes too complex: as bridge configuration affects
> the bus operation, you might end up sticking a pointer to the device
> in the bus. A similar arrangement is in place in with piix_pci, and I would
> love to get rid of it, too.

I'd glad to look into it, but I'd like to make it sure before digging
into it.
Do you mean i440fx_init() and I440FXState::bus = PCIHostState::bus?
Please a bit more concrete explanation.


> Let's just put PCIBus in a header? It could be a new header
> named pci_internals.h or something like this.

Sounds a good idea. In fact I had thought the same idea.
I'll go for that way.


> > ---
> >  hw/pci.c |   25 ++++++++++++++-----------
> >  1 files changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 08652e8..fdf02d0 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -286,23 +286,27 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> >      return bus;
> >  }
> >  
> > -static void pci_register_secondary_bus(PCIBus *parent,
> > -                                       PCIBus *bus,
> > -                                       PCIDevice *dev,
> > -                                       pci_map_irq_fn map_irq,
> > -                                       const char *name)
> > +static PCIBus *pci_register_secondary_bus(PCIBus *parent,
> > +                                          PCIDevice *dev,
> > +                                          pci_map_irq_fn map_irq,
> > +                                          const char *name)
> >  {
> > -    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
> > +    PCIBus *bus;
> > +    bus = pci_bus_new(&dev->qdev, name, 0);
> > +
> >      bus->map_irq = map_irq;
> >      bus->parent_dev = dev;
> >  
> >      QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> > +
> > +    return bus;
> 
> This does more than we need: pci_bus_new
> was created for host bus so it will also register in
> reset and vmstate lists.

I'm bit confused. I've thought that pci_bus_new() was for both root bus
and secondary bus. So I've tried to move out root bus specific stuff
from pci_bus_new().

But you claim it's only for root bus, not for secondary bus.
Now I realized why you've rejected such patches so far.
Then, you also mean the current pci_register_secondary_bus() is broken.
I also think it's broken. So how do we want to fix it?
My idea is as follows.

- introduce something like pci_secondary_bus_new()
  (pci_sec_bus_new() for short?) for secondary bus. 
  fix pci_register_secondary_bus() with it.

- introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
  for pci host bus which is more generic than pci_bus_new().
  It's for
  - to avoid confusion.
  - to eliminate assumption of pci_bus_new().
    pci_bus_new() assumes that its pci segment is 0.
    keep pci_bus_new() as a convenience wrapper of
    pci_host_bus_new(segment = 0). Thus we can avoid fixing up
    all the caller.

> >  }
> >  
> >  static void pci_unregister_secondary_bus(PCIBus *bus)
> >  {
> >      assert(QLIST_EMPTY(&bus->child));
> >      QLIST_REMOVE(bus, sibling);
> > +    qbus_free(&bus->qbus);
> >  }
> >  
> >  int pci_bus_num(PCIBus *s)
> > @@ -1527,7 +1531,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
> >  
> >  typedef struct {
> >      PCIDevice dev;
> > -    PCIBus bus;
> > +    PCIBus *bus;
> >      uint32_t vid;
> >      uint32_t did;
> >  } PCIBridge;
> > @@ -1628,8 +1632,7 @@ static int pci_bridge_initfn(PCIDevice *dev)
> >  static int pci_bridge_exitfn(PCIDevice *pci_dev)
> >  {
> >      PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
> > -    PCIBus *bus = &s->bus;
> > -    pci_unregister_secondary_bus(bus);
> > +    pci_unregister_secondary_bus(s->bus);
> >      return 0;
> >  }
> >  
> > @@ -1646,8 +1649,8 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> >      qdev_init_nofail(&dev->qdev);
> >  
> >      s = DO_UPCAST(PCIBridge, dev, dev);
> > -    pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name);
> > -    return &s->bus;
> > +    s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
> > +    return s->bus;
> >  }
> >  
> >  PCIDevice *pci_bridge_get_device(PCIBus *bus)
> > -- 
> > 1.7.1.1
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Michael S. Tsirkin]

On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> On Tue, Jul 06, 2010 at 03:18:52PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Jul 02, 2010 at 11:30:11AM +0900, Isaku Yamahata wrote:
> > > allocate PCIBus dynamically for PCIBridge and bug fix of
> > > pci_unregister_secondary_bus().
> > 
> > could you make the bugfix a separate patch please?
> 
> Will do.
> 
> 
> > > This is a preparation for splitting out pci_bridge functions.
> > > Since PCIBus is private to pci.c, PCIBridge won't be able to
> > > contain PCIBus in its structure.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > I think this becomes too complex: as bridge configuration affects
> > the bus operation, you might end up sticking a pointer to the device
> > in the bus. A similar arrangement is in place in with piix_pci, and I would
> > love to get rid of it, too.
> 
> I'd glad to look into it, but I'd like to make it sure before digging
> into it.
> Do you mean i440fx_init() and I440FXState::bus = PCIHostState::bus?
> Please a bit more concrete explanation.

I am not sure myself yet. Generally I'm not very happy with how
interrupts are handled.

Specifically:
	- lots of indirect calls through qemu_irq
          not type-safe, hard to debug and can not be good for performance
	  need to find a way to chase these pointers at setup time
	- lots of loops over irq pins and over buses
	  need to precompute and store at setup time, and use bits for booleans
	- information is duplicated, e.g. piix duplicates irq states
	  need to use from a single place
	  with the last issue, be careful not to break migration:
	  we need to compute and store old data on migration

In case of piix_pci interrupts are controlled through PIIX3 device, so
we create the host bus, the device on it, and finally make another call
to make interrupts on the bus get device as the opaque pointer.
All this looks very convoluted.

> 
> > Let's just put PCIBus in a header? It could be a new header
> > named pci_internals.h or something like this.
> 
> Sounds a good idea. In fact I had thought the same idea.
> I'll go for that way.
> 
> > > ---
> > >  hw/pci.c |   25 ++++++++++++++-----------
> > >  1 files changed, 14 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 08652e8..fdf02d0 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -286,23 +286,27 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > >      return bus;
> > >  }
> > >  
> > > -static void pci_register_secondary_bus(PCIBus *parent,
> > > -                                       PCIBus *bus,
> > > -                                       PCIDevice *dev,
> > > -                                       pci_map_irq_fn map_irq,
> > > -                                       const char *name)
> > > +static PCIBus *pci_register_secondary_bus(PCIBus *parent,
> > > +                                          PCIDevice *dev,
> > > +                                          pci_map_irq_fn map_irq,
> > > +                                          const char *name)
> > >  {
> > > -    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
> > > +    PCIBus *bus;
> > > +    bus = pci_bus_new(&dev->qdev, name, 0);
> > > +
> > >      bus->map_irq = map_irq;
> > >      bus->parent_dev = dev;
> > >  
> > >      QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> > > +
> > > +    return bus;
> > 
> > This does more than we need: pci_bus_new
> > was created for host bus so it will also register in
> > reset and vmstate lists.
> 
> I'm bit confused. I've thought that pci_bus_new() was for both root bus
> and secondary bus. So I've tried to move out root bus specific stuff
> from pci_bus_new().
> 
> But you claim it's only for root bus, not for secondary bus.
> Now I realized why you've rejected such patches so far.
> Then, you also mean the current pci_register_secondary_bus() is broken.
> I also think it's broken. So how do we want to fix it?
> My idea is as follows.
> 
> - introduce something like pci_secondary_bus_new()
>   (pci_sec_bus_new() for short?) for secondary bus. 
>   fix pci_register_secondary_bus() with it.
> 
> - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
>   for pci host bus which is more generic than pci_bus_new().
>   It's for
>   - to avoid confusion.
>   - to eliminate assumption of pci_bus_new().
>     pci_bus_new() assumes that its pci segment is 0.
>     keep pci_bus_new() as a convenience wrapper of
>     pci_host_bus_new(segment = 0). Thus we can avoid fixing up
>     all the caller.
> 
> > >  }
> > >  
> > >  static void pci_unregister_secondary_bus(PCIBus *bus)
> > >  {
> > >      assert(QLIST_EMPTY(&bus->child));
> > >      QLIST_REMOVE(bus, sibling);
> > > +    qbus_free(&bus->qbus);
> > >  }
> > >  
> > >  int pci_bus_num(PCIBus *s)
> > > @@ -1527,7 +1531,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
> > >  
> > >  typedef struct {
> > >      PCIDevice dev;
> > > -    PCIBus bus;
> > > +    PCIBus *bus;
> > >      uint32_t vid;
> > >      uint32_t did;
> > >  } PCIBridge;
> > > @@ -1628,8 +1632,7 @@ static int pci_bridge_initfn(PCIDevice *dev)
> > >  static int pci_bridge_exitfn(PCIDevice *pci_dev)
> > >  {
> > >      PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
> > > -    PCIBus *bus = &s->bus;
> > > -    pci_unregister_secondary_bus(bus);
> > > +    pci_unregister_secondary_bus(s->bus);
> > >      return 0;
> > >  }
> > >  
> > > @@ -1646,8 +1649,8 @@ PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> > >      qdev_init_nofail(&dev->qdev);
> > >  
> > >      s = DO_UPCAST(PCIBridge, dev, dev);
> > > -    pci_register_secondary_bus(bus, &s->bus, &s->dev, map_irq, name);
> > > -    return &s->bus;
> > > +    s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
> > > +    return s->bus;
> > >  }
> > >  
> > >  PCIDevice *pci_bridge_get_device(PCIBus *bus)
> > > -- 
> > > 1.7.1.1
> > 
> 
> -- 
> yamahata

Re: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Isaku Yamahata]

On Wed, Jul 07, 2010 at 02:47:10PM +0300, Michael S. Tsirkin wrote:
> > > I think this becomes too complex: as bridge configuration affects
> > > the bus operation, you might end up sticking a pointer to the device
> > > in the bus. A similar arrangement is in place in with piix_pci, and I would
> > > love to get rid of it, too.
> > 
> > I'd glad to look into it, but I'd like to make it sure before digging
> > into it.
> > Do you mean i440fx_init() and I440FXState::bus = PCIHostState::bus?
> > Please a bit more concrete explanation.
> 
> I am not sure myself yet. Generally I'm not very happy with how
> interrupts are handled.
> 
> Specifically:
> 	- lots of indirect calls through qemu_irq
>           not type-safe, hard to debug and can not be good for performance
> 	  need to find a way to chase these pointers at setup time
> 	- lots of loops over irq pins and over buses
> 	  need to precompute and store at setup time, and use bits for booleans
> 	- information is duplicated, e.g. piix duplicates irq states
> 	  need to use from a single place
> 	  with the last issue, be careful not to break migration:
> 	  we need to compute and store old data on migration
> 
> In case of piix_pci interrupts are controlled through PIIX3 device, so
> we create the host bus, the device on it, and finally make another call
> to make interrupts on the bus get device as the opaque pointer.
> All this looks very convoluted.

I see, it's concern about over all piix_pci.


Can you please comment on pci_bus_new() issue below?
I'm afraid that you missed it.

> > > > ---
> > > >  hw/pci.c |   25 ++++++++++++++-----------
> > > >  1 files changed, 14 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > index 08652e8..fdf02d0 100644
> > > > --- a/hw/pci.c
> > > > +++ b/hw/pci.c
> > > > @@ -286,23 +286,27 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > > >      return bus;
> > > >  }
> > > >  
> > > > -static void pci_register_secondary_bus(PCIBus *parent,
> > > > -                                       PCIBus *bus,
> > > > -                                       PCIDevice *dev,
> > > > -                                       pci_map_irq_fn map_irq,
> > > > -                                       const char *name)
> > > > +static PCIBus *pci_register_secondary_bus(PCIBus *parent,
> > > > +                                          PCIDevice *dev,
> > > > +                                          pci_map_irq_fn map_irq,
> > > > +                                          const char *name)
> > > >  {
> > > > -    qbus_create_inplace(&bus->qbus, &pci_bus_info, &dev->qdev, name);
> > > > +    PCIBus *bus;
> > > > +    bus = pci_bus_new(&dev->qdev, name, 0);
> > > > +
> > > >      bus->map_irq = map_irq;
> > > >      bus->parent_dev = dev;
> > > >  
> > > >      QLIST_INSERT_HEAD(&parent->child, bus, sibling);
> > > > +
> > > > +    return bus;
> > > 
> > > This does more than we need: pci_bus_new
> > > was created for host bus so it will also register in
> > > reset and vmstate lists.
> > 
> > I'm bit confused. I've thought that pci_bus_new() was for both root bus
> > and secondary bus. So I've tried to move out root bus specific stuff
> > from pci_bus_new().
> > 
> > But you claim it's only for root bus, not for secondary bus.
> > Now I realized why you've rejected such patches so far.
> > Then, you also mean the current pci_register_secondary_bus() is broken.
> > I also think it's broken. So how do we want to fix it?
> > My idea is as follows.
> > 
> > - introduce something like pci_secondary_bus_new()
> >   (pci_sec_bus_new() for short?) for secondary bus. 
> >   fix pci_register_secondary_bus() with it.
> > 
> > - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
> >   for pci host bus which is more generic than pci_bus_new().
> >   It's for
> >   - to avoid confusion.
> >   - to eliminate assumption of pci_bus_new().
> >     pci_bus_new() assumes that its pci segment is 0.
> >     keep pci_bus_new() as a convenience wrapper of
> >     pci_host_bus_new(segment = 0). Thus we can avoid fixing up
> >     all the caller.

-- 
yamahata

[Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Michael S. Tsirkin]

On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> But you claim it's only for root bus, not for secondary bus.

It is currently, isn't it?

> Now I realized why you've rejected such patches so far.
> Then, you also mean the current pci_register_secondary_bus() is broken.

Sorry about being dense, what is broken?

> I also think it's broken. So how do we want to fix it?
> My idea is as follows.
> 
> - introduce something like pci_secondary_bus_new()
>   (pci_sec_bus_new() for short?) for secondary bus. 
>   fix pci_register_secondary_bus() with it.
> 
> - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
>   for pci host bus which is more generic than pci_bus_new().
>   It's for
>   - to avoid confusion.

IMHO the confusion comes from the fact we have too
many functions that do almost, but not quite, the same
thing, and the function names do not say anything.

We have a ton of 5 line functions with names like
_allocate_inplace, _new, _register, _simple

>   - to eliminate assumption of pci_bus_new().
>     pci_bus_new() assumes that its pci segment is 0.
>     keep pci_bus_new() as a convenience wrapper of
>     pci_host_bus_new(segment = 0). Thus we can avoid fixing up
>     all the caller.

We have a single caller, right? I think you mean pci_register_bus?
So IIUC, you propose that we add pci_register_host_bus,
and make pci_register_bus a compatibility wrapper?
Sure, let's just add a comment this is deprecated.

I am not sure why do we need an API to deal with secondary bus:
it is always a part of the bridge, so all users can and should call
pci_bridge_init?

-- 
MST

Re: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Isaku Yamahata]

On Thu, Jul 08, 2010 at 05:04:32PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> > But you claim it's only for root bus, not for secondary bus.
> 
> It is currently, isn't it?
> 
> > Now I realized why you've rejected such patches so far.
> > Then, you also mean the current pci_register_secondary_bus() is broken.
> 
> Sorry about being dense, what is broken?

I've regarded pci_bus_new() (or _inplace) as new qdev style API.
And pci_register_bus() (or pci_register_secondary_bus()) as old 
(so deprecated) API.
So pci_reguster_bus() would be replaced with pci_bus_new() gradually
like the changeset of 7cd9eee0f6fd6953114068dd98d91fca1237880b
I've thought that pci_bus_new() is for both root and secondary bus.
However, according to your comment, the situation seems different.


> > I also think it's broken. So how do we want to fix it?
> > My idea is as follows.
> > 
> > - introduce something like pci_secondary_bus_new()
> >   (pci_sec_bus_new() for short?) for secondary bus. 
> >   fix pci_register_secondary_bus() with it.
> > 
> > - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
> >   for pci host bus which is more generic than pci_bus_new().
> >   It's for
> >   - to avoid confusion.
> 
> IMHO the confusion comes from the fact we have too
> many functions that do almost, but not quite, the same
> thing, and the function names do not say anything.
> 
> We have a ton of 5 line functions with names like
> _allocate_inplace, _new, _register, _simple

Fully Agreed. Some clean up is necessary.


> >   - to eliminate assumption of pci_bus_new().
> >     pci_bus_new() assumes that its pci segment is 0.
> >     keep pci_bus_new() as a convenience wrapper of
> >     pci_host_bus_new(segment = 0). Thus we can avoid fixing up
> >     all the caller.
> 
> We have a single caller, right? I think you mean pci_register_bus?
> So IIUC, you propose that we add pci_register_host_bus,
> and make pci_register_bus a compatibility wrapper?
> Sure, let's just add a comment this is deprecated.
> 
> I am not sure why do we need an API to deal with secondary bus:
> it is always a part of the bridge, so all users can and should call
> pci_bridge_init?

Okay, then how about the following?

For root bus:
- pci_host_bus_new()/pci_host_bus_new_inplace()
  qbus style api. pci segment must be specified.
  New code should use this.

- pci_bus_new()
  qbus style API.
  convenience wrapper for compatibility of
  pci_host_bus_new(pci segment = 0)
  In fact, the only current user piix_pci.c. It's easy to remove it.

- pci_register_bus()
  old style API. deprecated.
  It has been kept for compatibility so far.
  This will be gradually replaced with pci_host_bus_new()

For secondary bus:
- pci_bridge_init()
  qdev style API.
  New code should use this.

- pci_{register, unregister}_secondary_bus():
  old stype API. deprecated. 
  Keep them only for internal use in pci.c
  or they can be easily removed or renamed for qdev style.

For pci device:
- pci_create()
  qdev style API.
  The transitional function until completion of qdev conversion.
  If the creation of a device tree from config file is implemented,
  this function will be unnecessary.

- pci_create_simple()
  qdev style API.
  convenience function = pci_create() + qdev_init_nofail()

-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Michael S. Tsirkin]

On Fri, Jul 09, 2010 at 12:43:18AM +0900, Isaku Yamahata wrote:
> On Thu, Jul 08, 2010 at 05:04:32PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> > > But you claim it's only for root bus, not for secondary bus.
> > 
> > It is currently, isn't it?
> > 
> > > Now I realized why you've rejected such patches so far.
> > > Then, you also mean the current pci_register_secondary_bus() is broken.
> > 
> > Sorry about being dense, what is broken?
> 
> I've regarded pci_bus_new() (or _inplace) as new qdev style API.

The names are pretty bad btw, aren't they?  Would
pci_bus_init/pci_bus_cleanup be better?  And whoever wants to allocate
memory, can do it with malloc, right?

> And pci_register_bus() (or pci_register_secondary_bus()) as old 
> (so deprecated) API.
> So pci_reguster_bus() would be replaced with pci_bus_new() gradually
> like the changeset of 7cd9eee0f6fd6953114068dd98d91fca1237880b
> I've thought that pci_bus_new() is for both root and secondary bus.
> However, according to your comment, the situation seems different.

Forget my comment, it's different according to the code, isn't it?

> > > I also think it's broken. So how do we want to fix it?
> > > My idea is as follows.
> > > 
> > > - introduce something like pci_secondary_bus_new()
> > >   (pci_sec_bus_new() for short?) for secondary bus. 
> > >   fix pci_register_secondary_bus() with it.
> > > 
> > > - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
> > >   for pci host bus which is more generic than pci_bus_new().
> > >   It's for
> > >   - to avoid confusion.
> > 
> > IMHO the confusion comes from the fact we have too
> > many functions that do almost, but not quite, the same
> > thing, and the function names do not say anything.
> > 
> > We have a ton of 5 line functions with names like
> > _allocate_inplace, _new, _register, _simple
> 
> Fully Agreed. Some clean up is necessary.
> 
> 
> > >   - to eliminate assumption of pci_bus_new().
> > >     pci_bus_new() assumes that its pci segment is 0.
> > >     keep pci_bus_new() as a convenience wrapper of
> > >     pci_host_bus_new(segment = 0). Thus we can avoid fixing up
> > >     all the caller.
> > 
> > We have a single caller, right? I think you mean pci_register_bus?
> > So IIUC, you propose that we add pci_register_host_bus,
> > and make pci_register_bus a compatibility wrapper?
> > Sure, let's just add a comment this is deprecated.
> > 
> > I am not sure why do we need an API to deal with secondary bus:
> > it is always a part of the bridge, so all users can and should call
> > pci_bridge_init?
> 
> Okay, then how about the following?
> 
> For root bus:
> - pci_host_bus_new()/pci_host_bus_new_inplace()
>   qbus style api. pci segment must be specified.
>   New code should use this.

I'd prefer a simple _init which works like _inplace.
Allocating memory is simple enough for users.

> - pci_bus_new()
>   qbus style API.
>   convenience wrapper for compatibility of
>   pci_host_bus_new(pci segment = 0)
>   In fact, the only current user piix_pci.c. It's easy to remove it.
> - pci_register_bus()
>   old style API. deprecated.
>   It has been kept for compatibility so far.
>   This will be gradually replaced with pci_host_bus_new()

Also, let's make these helpers inline: will make it possible
to check code by comparing binary after changes.

> For secondary bus:
> - pci_bridge_init()
>   qdev style API.
>   New code should use this.

Well - isn't the way we do this a bit backwards?
I thought the idea was that each device has its own
PCIDeviceInfo qdev structure, instead of the common pci-bridge.

And then pci_bridge_init (or _setup to avoid reusing existing names)
would be a common function that devices can reuse in their init
functions..

> - pci_{register, unregister}_secondary_bus():
>   old stype API. deprecated. 
>   Keep them only for internal use in pci.c
>   or they can be easily removed or renamed for qdev style.
> 
> For pci device:
> - pci_create()
>   qdev style API.
>   The transitional function until completion of qdev conversion.
>   If the creation of a device tree from config file is implemented,
>   this function will be unnecessary.
> 
> - pci_create_simple()
>   qdev style API.
>   convenience function = pci_create() + qdev_init_nofail()
> 
> -- 
> yamahata

Re: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Isaku Yamahata]

On Thu, Jul 08, 2010 at 07:49:35PM +0300, Michael S. Tsirkin wrote:

> > > >   - to eliminate assumption of pci_bus_new().
> > > >     pci_bus_new() assumes that its pci segment is 0.
> > > >     keep pci_bus_new() as a convenience wrapper of
> > > >     pci_host_bus_new(segment = 0). Thus we can avoid fixing up
> > > >     all the caller.
> > > 
> > > We have a single caller, right? I think you mean pci_register_bus?
> > > So IIUC, you propose that we add pci_register_host_bus,
> > > and make pci_register_bus a compatibility wrapper?
> > > Sure, let's just add a comment this is deprecated.
> > > 
> > > I am not sure why do we need an API to deal with secondary bus:
> > > it is always a part of the bridge, so all users can and should call
> > > pci_bridge_init?
> > 
> > Okay, then how about the following?
> > 
> > For root bus:
> > - pci_host_bus_new()/pci_host_bus_new_inplace()
> >   qbus style api. pci segment must be specified.
> >   New code should use this.
> 
> I'd prefer a simple _init which works like _inplace.
> Allocating memory is simple enough for users.

I see. Then
- export PCIBus. This is necessary to tell its size.
- pci_host_bus_init()/pci_host_bus_cleanup()
  which don't allocate memory.

> > - pci_bus_new()
> >   qbus style API.
> >   convenience wrapper for compatibility of
> >   pci_host_bus_new(pci segment = 0)
> >   In fact, the only current user piix_pci.c. It's easy to remove it.
> > - pci_register_bus()
> >   old style API. deprecated.
> >   It has been kept for compatibility so far.
> >   This will be gradually replaced with pci_host_bus_new()
> 
> Also, let's make these helpers inline: will make it possible
> to check code by comparing binary after changes.

Agreed.

> 
> > For secondary bus:
> > - pci_bridge_init()
> >   qdev style API.
> >   New code should use this.
> 
> Well - isn't the way we do this a bit backwards?
> I thought the idea was that each device has its own
> PCIDeviceInfo qdev structure, instead of the common pci-bridge.
> 
> And then pci_bridge_init (or _setup to avoid reusing existing names)
> would be a common function that devices can reuse in their init
> functions..

Agreed and in fact I'm having such a patch.
Just I think it's next phase of clean up.
The patch series creates a library for pci bridge as you suggested before
and converts apb_pci.c and dec_pci.c into qdev.

> 
> > - pci_{register, unregister}_secondary_bus():
> >   old stype API. deprecated. 
> >   Keep them only for internal use in pci.c
> >   or they can be easily removed or renamed for qdev style.
> > 
> > For pci device:
> > - pci_create()
> >   qdev style API.
> >   The transitional function until completion of qdev conversion.
> >   If the creation of a device tree from config file is implemented,
> >   this function will be unnecessary.
> > 
> > - pci_create_simple()
> >   qdev style API.
> >   convenience function = pci_create() + qdev_init_nofail()
> > 
> > -- 
> > yamahata
> 

-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Isaku Yamahata]

On Thu, Jul 08, 2010 at 07:49:35PM +0300, Michael S. Tsirkin wrote:

> > For root bus:
> > - pci_host_bus_new()/pci_host_bus_new_inplace()
> >   qbus style api. pci segment must be specified.
> >   New code should use this.
> 
> I'd prefer a simple _init which works like _inplace.
> Allocating memory is simple enough for users.

How about the following patch?
  - replaced pci_bus_new(), pci_bus_new_inplace() with
    pci_host_bus_init()/pci_host_bus_cleanup()
  - eliminated memory allocation by embedding PCIBus into PCIHostState.

I've tested it with only piix4 yet.
If this direction is okay, I'll give it more tests and post it.


>From d54dffc884c93bc4f71c506a933891d404e5bf81 Mon Sep 17 00:00:00 2001
Message-Id: <d54dffc884c93bc4f71c506a933891d404e5bf81.1279244480.git.yamahata@valinux.co.jp>
In-Reply-To: <cover.1279244480.git.yamahata@valinux.co.jp>
References: <cover.1279244480.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Tue, 13 Jul 2010 17:35:00 +0900
Subject: [PATCH] pci/pci_host: pci host bus initialization clean up.

Embed PCIBus into PCIHostState and clean up of pci host bus initialization.
And Embed PCIHostState into each devices.
Especially pci host bus creation must be aware of pci segment, usually 0.
Although some boards doesn't use PCIHostState at the moment,
in long term enhance PCIHostState and convert them.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/apb_pci.c       |   21 ++++++++---------
 hw/bonito.c        |   25 +++++++++++----------
 hw/grackle_pci.c   |   11 ++++-----
 hw/gt64xxx.c       |   14 ++++++------
 hw/pci.c           |   61 ++++++++++++++++++++++++++++++++++------------------
 hw/pci.h           |   18 ++++++++++-----
 hw/pci_host.c      |   14 ++++-------
 hw/pci_host.h      |   13 ++++++++++-
 hw/pcie_host.c     |   12 +++++-----
 hw/piix_pci.c      |    5 ++-
 hw/ppc4xx_pci.c    |   11 ++++-----
 hw/ppce500_pci.c   |   12 ++++------
 hw/prep_pci.c      |   20 ++++++++--------
 hw/sh_pci.c        |   14 ++++++------
 hw/unin_pci.c      |   30 ++++++++++++------------
 hw/versatile_pci.c |    8 ++++--
 16 files changed, 160 insertions(+), 129 deletions(-)

diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index d446550..027160a 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -68,7 +68,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
 
 typedef struct APBState {
     SysBusDevice busdev;
-    PCIBus      *bus;
+    PCIHostState pci;
     ReadWriteHandler pci_config_handler;
     uint32_t iommu[4];
     uint32_t pci_control[16];
@@ -194,7 +194,7 @@ static void apb_pci_config_write(ReadWriteHandler *h, pcibus_t addr,
 
     val = qemu_bswap_len(val, size);
     APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %x\n", __func__, addr, val);
-    pci_data_write(s->bus, addr, val, size);
+    pci_data_write(&s->pci.bus, addr, val, size);
 }
 
 static uint32_t apb_pci_config_read(ReadWriteHandler *h, pcibus_t addr,
@@ -203,7 +203,7 @@ static uint32_t apb_pci_config_read(ReadWriteHandler *h, pcibus_t addr,
     uint32_t ret;
     APBState *s = container_of(h, APBState, pci_config_handler);
 
-    ret = pci_data_read(s->bus, addr, size);
+    ret = pci_data_read(&s->pci.bus, addr, size);
     ret = qemu_bswap_len(ret, size);
     APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, ret);
     return ret;
@@ -347,29 +347,28 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
     sysbus_mmio_map(s, 2, special_base + 0x2000000ULL);
     d = FROM_SYSBUS(APBState, s);
 
-    d->bus = pci_register_bus(&d->busdev.qdev, "pci",
-                                         pci_apb_set_irq, pci_pbm_map_irq, d,
-                                         0, 32);
-    pci_bus_set_mem_base(d->bus, mem_base);
+    pci_host_bus_init_simple(&d->pci, &d->busdev.qdev, 0, "pci",
+                             pci_apb_set_irq, pci_pbm_map_irq, d, 0, 32);
+    pci_bus_set_mem_base(&d->pci.bus, mem_base);
 
     for (i = 0; i < 32; i++) {
         sysbus_connect_irq(s, i, pic[i]);
     }
 
-    pci_create_simple(d->bus, 0, "pbm");
+    pci_create_simple(&d->pci.bus, 0, "pbm");
 
     /* APB secondary busses */
-    br = pci_bridge_create_simple(d->bus, PCI_DEVFN(1, 0), true,
+    br = pci_bridge_create_simple(&d->pci.bus, PCI_DEVFN(1, 0), true,
                                   pci_apb_map_irq, "pbm-bridge",
                                   "Advanced PCI Bus secondary bridge 1");
     *bus2 = pci_bridge_get_sec_bus(br);
 
-    br = pci_bridge_create_simple(d->bus, PCI_DEVFN(1, 1), true,
+    br = pci_bridge_create_simple(&d->pci.bus, PCI_DEVFN(1, 1), true,
                                   pci_apb_map_irq, "pbm-bridge",
                                   "Advanced PCI Bus secondary bridge 2");
     *bus3 = pci_bridge_get_sec_bus(br);
 
-    return d->bus;
+    return &d->pci.bus;
 }
 
 static void pci_pbm_reset(DeviceState *d)
diff --git a/hw/bonito.c b/hw/bonito.c
index 414e0aa..a328f3a 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -444,9 +444,9 @@ static uint32_t bonito_sbridge_pciaddr(void *opaque, target_phys_addr_t addr)
             ",pcimap_cfg=%x\n", addr, s->regs[BONITO_PCIMAP_CFG]);
         exit(1);
     }
-    pciaddr = PCI_ADDR(pci_bus_num(s->pcihost->bus), devno, funno, regno);
+    pciaddr = PCI_ADDR(pci_bus_num(&s->pcihost->bus), devno, funno, regno);
     DPRINTF("cfgaddr %x pciaddr %x busno %x devno %d funno %d regno %d \n",
-        cfgaddr, pciaddr, pci_bus_num(s->pcihost->bus), devno, funno, regno);
+        cfgaddr, pciaddr, pci_bus_num(&s->pcihost->bus), devno, funno, regno);
 
     return pciaddr;
 }
@@ -467,7 +467,7 @@ static void bonito_spciconf_writeb(void *opaque, target_phys_addr_t addr,
 
     /* set the pci address in s->config_reg */
     s->pcihost->config_reg = (pciaddr) | (1u << 31);
-    pci_data_write(s->pcihost->bus, s->pcihost->config_reg, val & 0xff, 1);
+    pci_data_write(&s->pcihost->bus, s->pcihost->config_reg, val & 0xff, 1);
 
     /* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
     status = pci_get_word(s->dev.config + PCI_STATUS);
@@ -493,7 +493,7 @@ static void bonito_spciconf_writew(void *opaque, target_phys_addr_t addr,
 
     /* set the pci address in s->config_reg */
     s->pcihost->config_reg = (pciaddr) | (1u << 31);
-    pci_data_write(s->pcihost->bus, s->pcihost->config_reg, val, 2);
+    pci_data_write(&s->pcihost->bus, s->pcihost->config_reg, val, 2);
 
     /* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
     status = pci_get_word(s->dev.config + PCI_STATUS);
@@ -519,7 +519,7 @@ static void bonito_spciconf_writel(void *opaque, target_phys_addr_t addr,
 
     /* set the pci address in s->config_reg */
     s->pcihost->config_reg = (pciaddr) | (1u << 31);
-    pci_data_write(s->pcihost->bus, s->pcihost->config_reg, val, 4);
+    pci_data_write(&s->pcihost->bus, s->pcihost->config_reg, val, 4);
 
     /* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
     status = pci_get_word(s->dev.config + PCI_STATUS);
@@ -548,7 +548,7 @@ static uint32_t bonito_spciconf_readb(void *opaque, target_phys_addr_t addr)
     status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
     pci_set_word(s->dev.config + PCI_STATUS, status);
 
-    return pci_data_read(s->pcihost->bus, s->pcihost->config_reg, 1);
+    return pci_data_read(&s->pcihost->bus, s->pcihost->config_reg, 1);
 }
 
 static uint32_t bonito_spciconf_readw(void *opaque, target_phys_addr_t addr)
@@ -574,7 +574,7 @@ static uint32_t bonito_spciconf_readw(void *opaque, target_phys_addr_t addr)
     status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
     pci_set_word(s->dev.config + PCI_STATUS, status);
 
-    return pci_data_read(s->pcihost->bus, s->pcihost->config_reg, 2);
+    return pci_data_read(&s->pcihost->bus, s->pcihost->config_reg, 2);
 }
 
 static uint32_t bonito_spciconf_readl(void *opaque, target_phys_addr_t addr)
@@ -600,7 +600,7 @@ static uint32_t bonito_spciconf_readl(void *opaque, target_phys_addr_t addr)
     status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
     pci_set_word(s->dev.config + PCI_STATUS, status);
 
-    return pci_data_read(s->pcihost->bus, s->pcihost->config_reg, 4);
+    return pci_data_read(&s->pcihost->bus, s->pcihost->config_reg, 4);
 }
 
 /* south bridge PCI configure space. 0x1fe8 0000 - 0x1fef ffff */
@@ -774,11 +774,12 @@ PCIBus *bonito_init(qemu_irq *pic)
 
     dev = qdev_create(NULL, "Bonito-pcihost");
     pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
-    b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
-                         pci_bonito_map_irq, pic, 0x28, 32);
-    pcihost->pci.bus = b;
+    pci_host_bus_init_simple(&pcihost->pci, &pcihost->busdev.qdev, 0, "pci",
+                             pci_bonito_set_irq, pci_bonito_map_irq,
+                             pic, 0x28, 32);
     qdev_init_nofail(dev);
-    pci_bus_set_mem_base(pcihost->pci.bus, 0x10000000);
+    b = &pcihost->pci.bus;
+    pci_bus_set_mem_base(b, 0x10000000);
 
     d = pci_create_simple(b, PCI_DEVFN(0, 0), "Bonito");
     s = DO_UPCAST(PCIBonitoState, dev, d);
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index 91c755f..1056ed3 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -88,17 +88,16 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
     qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     d = FROM_SYSBUS(GrackleState, s);
-    d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
-                                         pci_grackle_set_irq,
-                                         pci_grackle_map_irq,
-                                         pic, 0, 4);
+    pci_host_bus_init_simple(&d->host_state, &d->busdev.qdev, 0, "pci",
+                             pci_grackle_set_irq, pci_grackle_map_irq,
+                             pic, 0, 4);
 
-    pci_create_simple(d->host_state.bus, 0, "grackle");
+    pci_create_simple(&d->host_state.bus, 0, "grackle");
 
     sysbus_mmio_map(s, 0, base);
     sysbus_mmio_map(s, 1, base + 0x00200000);
 
-    return d->host_state.bus;
+    return &d->host_state.bus;
 }
 
 static int pci_grackle_init_device(SysBusDevice *dev)
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index cabf7ea..4e227ba 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -535,7 +535,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
         if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
             val = bswap32(val);
         if (s->pci->config_reg & (1u << 31))
-            pci_data_write(s->pci->bus, s->pci->config_reg, val, 4);
+            pci_data_write(&s->pci->bus, s->pci->config_reg, val, 4);
         break;
 
     /* Interrupts */
@@ -775,7 +775,7 @@ static uint32_t gt64120_readl (void *opaque,
         if (!(s->pci->config_reg & (1 << 31)))
             val = 0xffffffff;
         else
-            val = pci_data_read(s->pci->bus, s->pci->config_reg, 4);
+            val = pci_data_read(&s->pci->bus, s->pci->config_reg, 4);
         if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
             val = bswap32(val);
         break;
@@ -1113,11 +1113,11 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
     s = qemu_mallocz(sizeof(GT64120State));
     s->pci = qemu_mallocz(sizeof(GT64120PCIState));
 
-    s->pci->bus = pci_register_bus(NULL, "pci",
-                                   pci_gt64120_set_irq, pci_gt64120_map_irq,
-                                   pic, PCI_DEVFN(18, 0), 4);
+    pci_host_bus_init_simple(s->pci, NULL, 0, "pci",
+                             pci_gt64120_set_irq, pci_gt64120_map_irq,
+                             pic, PCI_DEVFN(18, 0), 4);
     s->ISD_handle = cpu_register_io_memory(gt64120_read, gt64120_write, s);
-    d = pci_register_device(s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice),
+    d = pci_register_device(&s->pci->bus, "GT64120 PCI Bus", sizeof(PCIDevice),
                             0, NULL, NULL);
 
     /* FIXME: Malta specific hw assumptions ahead */
@@ -1149,5 +1149,5 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
     register_savevm(&d->qdev, "GT64120 PCI Bus", 0, 1,
                     gt64120_save, gt64120_load, d);
 
-    return s->pci->bus;
+    return &s->pci->bus;
 }
diff --git a/hw/pci.c b/hw/pci.c
index 737fbd2..71430ad 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -25,6 +25,7 @@
 #include "pci.h"
 #include "pci_bridge.h"
 #include "pci_internals.h"
+#include "pci_host.h"
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
@@ -173,6 +174,19 @@ static void pci_host_bus_register(int domain, PCIBus *bus)
     QLIST_INSERT_HEAD(&host_buses, host, next);
 }
 
+static void pci_host_bus_unregister(PCIBus *bus)
+{
+    struct PCIHostBus *host;
+    QLIST_FOREACH(host, &host_buses, next) {
+        if (host->bus == bus) {
+            QLIST_REMOVE(host, next);
+            qemu_free(host);
+            return;
+        }
+    }
+    abort();/* should not be reached */
+}
+
 PCIBus *pci_find_root_bus(int domain)
 {
     struct PCIHostBus *host;
@@ -206,29 +220,45 @@ int pci_find_domain(const PCIBus *bus)
     return -1;
 }
 
-void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
-                         const char *name, int devfn_min)
+/* initialize pci host bus. To create secondary bus use pci_bridge.c */
+void pci_host_bus_init(PCIHostState *pci_host, DeviceState *parent,
+                       uint16_t segment, const char *name, int devfn_min)
 {
-    qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
+    PCIBus *bus = &pci_host->bus;
     assert(PCI_FUNC(devfn_min) == 0);
+
+    pci_host_init(pci_host);
+    qbus_create_inplace(&bus->qbus, &pci_bus_info, parent, name);
     bus->devfn_min = devfn_min;
 
     /* host bridge */
     QLIST_INIT(&bus->child);
-    pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */
+    pci_host_bus_register(segment, bus);
 
     vmstate_register(NULL, -1, &vmstate_pcibus, bus);
     qemu_register_reset(pci_bus_reset, bus);
 }
 
-PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min)
+void pci_host_bus_cleanup(PCIHostState *pci_host)
 {
-    PCIBus *bus;
+    PCIBus *bus = &pci_host->bus;
+    assert(bus->parent_dev == NULL);    /* host bus */
+    assert(QLIST_EMPTY(&bus->child));   /* no child bus */
 
-    bus = qemu_mallocz(sizeof(*bus));
-    bus->qbus.qdev_allocated = 1;
-    pci_bus_new_inplace(bus, parent, name, devfn_min);
-    return bus;
+    qemu_unregister_reset(pci_bus_reset, bus);
+    vmstate_unregister(NULL, &vmstate_pcibus, bus);
+    pci_host_bus_unregister(bus);
+    /* pci_host_cleanup(pci_host) */
+}
+
+/* convenience function to ease conversion from pci_register_bus(). */
+void pci_host_bus_init_simple(PCIHostState *pci_host, DeviceState *parent,
+                              uint16_t segment, const char *name,
+                              pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+                              void *irq_opaque, int devfn_min, int nirq)
+{
+    pci_host_bus_init(pci_host, parent, segment, name, devfn_min);
+    pci_bus_irqs(&pci_host->bus, set_irq, map_irq, irq_opaque, nirq);
 }
 
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
@@ -253,17 +283,6 @@ void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base)
     bus->mem_base = base;
 }
 
-PCIBus *pci_register_bus(DeviceState *parent, const char *name,
-                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque, int devfn_min, int nirq)
-{
-    PCIBus *bus;
-
-    bus = pci_bus_new(parent, name, devfn_min);
-    pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
-    return bus;
-}
-
 int pci_bus_num(PCIBus *s)
 {
     if (!s->parent_dev)
diff --git a/hw/pci.h b/hw/pci.h
index c551f96..a268f3c 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -204,15 +204,21 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
 typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, int state);
-void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
-                         const char *name, int devfn_min);
-PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
+
+void pci_host_bus_init(PCIHostState *pci_host, DeviceState *parent,
+                       uint16_t segment, const char *name, int devfn_min);
+void pci_host_bus_cleanup(PCIHostState *pci_host);
+
+/* convenience function to ease conversion from pci_register_bus().
+   = pci_host_bus_init() + pci_bus_irqs() */
+void pci_host_bus_init_simple(PCIHostState *pci_host, DeviceState *parent,
+                              uint16_t segment, const char *name,
+                              pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
+                              void *irq_opaque, int devfn_min, int nirq);
+
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
 void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
-PCIBus *pci_register_bus(DeviceState *parent, const char *name,
-                         pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque, int devfn_min, int nirq);
 
 void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
 
diff --git a/hw/pci_host.c b/hw/pci_host.c
index bc5b771..811454f 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -131,7 +131,7 @@ static void pci_host_data_write_swap(ReadWriteHandler *handler,
     PCI_DPRINTF("write addr %" FMT_PCIBUS " len %d val %x\n",
                 addr, len, val);
     if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+        pci_data_write(&s->bus, s->config_reg | (addr & 3), val, len);
 }
 
 static uint32_t pci_host_data_read_swap(ReadWriteHandler *handler,
@@ -141,7 +141,7 @@ static uint32_t pci_host_data_read_swap(ReadWriteHandler *handler,
     uint32_t val;
     if (!(s->config_reg & (1 << 31)))
         return 0xffffffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
+    val = pci_data_read(&s->bus, s->config_reg | (addr & 3), len);
     PCI_DPRINTF("read addr %" FMT_PCIBUS " len %d val %x\n",
                 addr, len, val);
     val = qemu_bswap_len(val, len);
@@ -155,7 +155,7 @@ static void pci_host_data_write_noswap(ReadWriteHandler *handler,
     PCI_DPRINTF("write addr %" FMT_PCIBUS " len %d val %x\n",
                 addr, len, val);
     if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
+        pci_data_write(&s->bus, s->config_reg | (addr & 3), val, len);
 }
 
 static uint32_t pci_host_data_read_noswap(ReadWriteHandler *handler,
@@ -165,13 +165,13 @@ static uint32_t pci_host_data_read_noswap(ReadWriteHandler *handler,
     uint32_t val;
     if (!(s->config_reg & (1 << 31)))
         return 0xffffffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
+    val = pci_data_read(&s->bus, s->config_reg | (addr & 3), len);
     PCI_DPRINTF("read addr %" FMT_PCIBUS " len %d val %x\n",
                 addr, len, val);
     return val;
 }
 
-static void pci_host_init(PCIHostState *s)
+void pci_host_init(PCIHostState *s)
 {
     s->conf_handler.write = pci_host_config_write_swap;
     s->conf_handler.read = pci_host_config_read_swap;
@@ -185,7 +185,6 @@ static void pci_host_init(PCIHostState *s)
 
 int pci_host_conf_register_mmio(PCIHostState *s, int swap)
 {
-    pci_host_init(s);
     if (swap) {
         return cpu_register_io_memory_simple(&s->conf_handler);
     } else {
@@ -195,13 +194,11 @@ int pci_host_conf_register_mmio(PCIHostState *s, int swap)
 
 void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
 {
-    pci_host_init(s);
     register_ioport_simple(&s->conf_noswap_handler, ioport, 4, 4);
 }
 
 int pci_host_data_register_mmio(PCIHostState *s, int swap)
 {
-    pci_host_init(s);
     if (swap) {
         return cpu_register_io_memory_simple(&s->data_handler);
     } else {
@@ -211,7 +208,6 @@ int pci_host_data_register_mmio(PCIHostState *s, int swap)
 
 void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
 {
-    pci_host_init(s);
     register_ioport_simple(&s->data_noswap_handler, ioport, 4, 1);
     register_ioport_simple(&s->data_noswap_handler, ioport, 4, 2);
     register_ioport_simple(&s->data_noswap_handler, ioport, 4, 4);
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 46d1379..4d9c10e 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -25,11 +25,20 @@
 /* Worker routines for a PCI host controller that uses an {address,data}
    register pair to access PCI configuration space.  */
 
+/*
+ * TODO: there remains some boards which doesn't use PCIHostState.
+ *       Enhance PCIHostState API and convert remaining boards.
+ *       - allow custom address decoder which doesn't match with
+ *         24:16 bus, 15:8 defn, 7:0 offset.
+ *       - allow MMIO mapped direct access to configuration space
+ */
+
 #ifndef PCI_HOST_H
 #define PCI_HOST_H
 
 #include "sysbus.h"
 #include "rwhandler.h"
+#include "pci_internals.h"
 
 struct PCIHostState {
     ReadWriteHandler conf_noswap_handler;
@@ -37,9 +46,11 @@ struct PCIHostState {
     ReadWriteHandler data_noswap_handler;
     ReadWriteHandler data_handler;
     uint32_t config_reg;
-    PCIBus *bus;
+    PCIBus bus;
 };
 
+void pci_host_init(PCIHostState *s);
+
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
 
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index c4feeca..0de90cd 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -80,39 +80,39 @@ static void pcie_mmcfg_data_writeb(void *opaque,
                                    target_phys_addr_t addr, uint32_t value)
 {
     PCIExpressHost *e = opaque;
-    pcie_mmcfg_data_write(e->pci.bus, addr - e->base_addr, value, 1);
+    pcie_mmcfg_data_write(&e->pci.bus, addr - e->base_addr, value, 1);
 }
 
 static void pcie_mmcfg_data_writew(void *opaque,
                                    target_phys_addr_t addr, uint32_t value)
 {
     PCIExpressHost *e = opaque;
-    pcie_mmcfg_data_write(e->pci.bus, addr - e->base_addr, value, 2);
+    pcie_mmcfg_data_write(&e->pci.bus, addr - e->base_addr, value, 2);
 }
 
 static void pcie_mmcfg_data_writel(void *opaque,
                                    target_phys_addr_t addr, uint32_t value)
 {
     PCIExpressHost *e = opaque;
-    pcie_mmcfg_data_write(e->pci.bus, addr - e->base_addr, value, 4);
+    pcie_mmcfg_data_write(&e->pci.bus, addr - e->base_addr, value, 4);
 }
 
 static uint32_t pcie_mmcfg_data_readb(void *opaque, target_phys_addr_t addr)
 {
     PCIExpressHost *e = opaque;
-    return pcie_mmcfg_data_read(e->pci.bus, addr - e->base_addr, 1);
+    return pcie_mmcfg_data_read(&e->pci.bus, addr - e->base_addr, 1);
 }
 
 static uint32_t pcie_mmcfg_data_readw(void *opaque, target_phys_addr_t addr)
 {
     PCIExpressHost *e = opaque;
-    return pcie_mmcfg_data_read(e->pci.bus, addr - e->base_addr, 2);
+    return pcie_mmcfg_data_read(&e->pci.bus, addr - e->base_addr, 2);
 }
 
 static uint32_t pcie_mmcfg_data_readl(void *opaque, target_phys_addr_t addr)
 {
     PCIExpressHost *e = opaque;
-    return pcie_mmcfg_data_read(e->pci.bus, addr - e->base_addr, 4);
+    return pcie_mmcfg_data_read(&e->pci.bus, addr - e->base_addr, 4);
 }
 
 
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 663afe2..3d03165 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -228,9 +228,10 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
 
     dev = qdev_create(NULL, "i440FX-pcihost");
     s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
-    b = pci_bus_new(&s->busdev.qdev, NULL, 0);
-    s->pci_host.bus = b;
+
+    pci_host_bus_init(&s->pci_host, &s->busdev.qdev, 0, NULL, 0);
     qdev_init_nofail(dev);
+    b = &s->pci_host.bus;
 
     d = pci_create_simple(b, 0, "i440FX");
     *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 6e437e7..357eb88 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -357,12 +357,11 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
 
     controller = qemu_mallocz(sizeof(PPC4xxPCIState));
 
-    controller->pci_state.bus = pci_register_bus(NULL, "pci",
-                                                 ppc4xx_pci_set_irq,
-                                                 ppc4xx_pci_map_irq,
-                                                 pci_irqs, 0, 4);
+    pci_host_bus_init_simple(&controller->pci_state, NULL, 0, "pci",
+                             ppc4xx_pci_set_irq, ppc4xx_pci_map_irq,
+                             pci_irqs, 0, 4);
 
-    controller->pci_dev = pci_register_device(controller->pci_state.bus,
+    controller->pci_dev = pci_register_device(&controller->pci_state.bus,
                                               "host bridge", sizeof(PCIDevice),
                                               0, NULL, NULL);
     pci_conf = controller->pci_dev->config;
@@ -395,7 +394,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
     register_savevm(&controller->pci_dev->qdev, "ppc4xx_pci", ppc4xx_pci_id++,
                     1, ppc4xx_pci_save, ppc4xx_pci_load, controller);
 
-    return controller->pci_state.bus;
+    return &controller->pci_state.bus;
 
 free:
     printf("%s error\n", __func__);
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 8ac99f2..8db470d 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -276,12 +276,10 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
 
     controller = qemu_mallocz(sizeof(PPCE500PCIState));
 
-    controller->pci_state.bus = pci_register_bus(NULL, "pci",
-                                                 mpc85xx_pci_set_irq,
-                                                 mpc85xx_pci_map_irq,
-                                                 pci_irqs, PCI_DEVFN(0x11, 0),
-                                                 4);
-    d = pci_register_device(controller->pci_state.bus,
+    pci_host_bus_init_simple(&controller->pci_state, NULL, 0, "pci",
+                             mpc85xx_pci_set_irq, mpc85xx_pci_map_irq,
+                             pci_irqs, PCI_DEVFN(0x11, 0), 4);
+    d = pci_register_device(&controller->pci_state.bus,
                             "host bridge", sizeof(PCIDevice),
                             0, NULL, NULL);
 
@@ -314,7 +312,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
     register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++,
                     1, ppce500_pci_save, ppce500_pci_load, controller);
 
-    return controller->pci_state.bus;
+    return &controller->pci_state.bus;
 
 free:
     printf("%s error\n", __func__);
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 0c2afe9..f3d0655 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -43,28 +43,28 @@ static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
 static void PPC_PCIIO_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     PREPPCIState *s = opaque;
-    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 1);
+    pci_data_write(&s->bus, PPC_PCIIO_config(addr), val, 1);
 }
 
 static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     PREPPCIState *s = opaque;
     val = bswap16(val);
-    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 2);
+    pci_data_write(&s->bus, PPC_PCIIO_config(addr), val, 2);
 }
 
 static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     PREPPCIState *s = opaque;
     val = bswap32(val);
-    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 4);
+    pci_data_write(&s->bus, PPC_PCIIO_config(addr), val, 4);
 }
 
 static uint32_t PPC_PCIIO_readb (void *opaque, target_phys_addr_t addr)
 {
     PREPPCIState *s = opaque;
     uint32_t val;
-    val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 1);
+    val = pci_data_read(&s->bus, PPC_PCIIO_config(addr), 1);
     return val;
 }
 
@@ -72,7 +72,7 @@ static uint32_t PPC_PCIIO_readw (void *opaque, target_phys_addr_t addr)
 {
     PREPPCIState *s = opaque;
     uint32_t val;
-    val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 2);
+    val = pci_data_read(&s->bus, PPC_PCIIO_config(addr), 2);
     val = bswap16(val);
     return val;
 }
@@ -81,7 +81,7 @@ static uint32_t PPC_PCIIO_readl (void *opaque, target_phys_addr_t addr)
 {
     PREPPCIState *s = opaque;
     uint32_t val;
-    val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 4);
+    val = pci_data_read(&s->bus, PPC_PCIIO_config(addr), 4);
     val = bswap32(val);
     return val;
 }
@@ -117,8 +117,8 @@ PCIBus *pci_prep_init(qemu_irq *pic)
     int PPC_io_memory;
 
     s = qemu_mallocz(sizeof(PREPPCIState));
-    s->bus = pci_register_bus(NULL, "pci",
-                              prep_set_irq, prep_map_irq, pic, 0, 4);
+    pci_host_bus_init_simple(s, NULL, 0, "pci",
+                             prep_set_irq, prep_map_irq, pic, 0, 4);
 
     pci_host_conf_register_ioport(0xcf8, s);
 
@@ -129,7 +129,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
     cpu_register_physical_memory(0x80800000, 0x00400000, PPC_io_memory);
 
     /* PCI host bridge */
-    d = pci_register_device(s->bus, "PREP Host Bridge - Motorola Raven",
+    d = pci_register_device(&s->bus, "PREP Host Bridge - Motorola Raven",
                             sizeof(PCIDevice), 0, NULL, NULL);
     pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_MOTOROLA);
     pci_config_set_device_id(d->config, PCI_DEVICE_ID_MOTOROLA_RAVEN);
@@ -139,5 +139,5 @@ PCIBus *pci_prep_init(qemu_irq *pic)
     d->config[0x0D] = 0x10; // latency_timer
     d->config[0x34] = 0x00; // capabilities_pointer
 
-    return s->bus;
+    return &s->bus;
 }
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index cc2f190..798501d 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -29,7 +29,7 @@
 #include "bswap.h"
 
 typedef struct {
-    PCIBus *bus;
+    PCIHostState pci;
     PCIDevice *dev;
     uint32_t par;
     uint32_t mbr;
@@ -58,7 +58,7 @@ static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
         }
         break;
     case 0x220:
-        pci_data_write(pcic->bus, pcic->par, val, 4);
+        pci_data_write(&pcic->pci.bus, pcic->par, val, 4);
         break;
     }
 }
@@ -76,7 +76,7 @@ static uint32_t sh_pci_reg_read (void *p, target_phys_addr_t addr)
     case 0x1c8:
         return pcic->iobr;
     case 0x220:
-        return pci_data_read(pcic->bus, pcic->par, 4);
+        return pci_data_read(&pcic->pci.bus, pcic->par, 4);
     }
     return 0;
 }
@@ -98,10 +98,10 @@ PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     int reg;
 
     p = qemu_mallocz(sizeof(SHPCIC));
-    p->bus = pci_register_bus(NULL, "pci",
-                              set_irq, map_irq, opaque, devfn_min, nirq);
+    pci_host_bus_init_simple(&p->pci, NULL, 0, "pci",
+                             set_irq, map_irq, opaque, devfn_min, nirq);
 
-    p->dev = pci_register_device(p->bus, "SH PCIC", sizeof(PCIDevice),
+    p->dev = pci_register_device(&p->pci.bus, "SH PCIC", sizeof(PCIDevice),
                                  -1, NULL, NULL);
     reg = cpu_register_io_memory(sh_pci_reg.r, sh_pci_reg.w, p);
     cpu_register_physical_memory(0x1e200000, 0x224, reg);
@@ -117,5 +117,5 @@ PCIBus *sh_pci_register_bus(pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     p->dev->config[0x06] = 0x90;
     p->dev->config[0x07] = 0x02;
 
-    return p->bus;
+    return &p->pci.bus;
 }
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 1310211..9439cf2 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -123,7 +123,7 @@ static void unin_data_write(ReadWriteHandler *handler,
     UNINState *s = container_of(handler, UNINState, data_handler);
     val = qemu_bswap_len(val, len);
     UNIN_DPRINTF("write addr %" FMT_PCIBUS " len %d val %x\n", addr, len, val);
-    pci_data_write(s->host_state.bus,
+    pci_data_write(&s->host_state.bus,
                    unin_get_config_reg(s->host_state.config_reg, addr),
                    val, len);
 }
@@ -134,7 +134,7 @@ static uint32_t unin_data_read(ReadWriteHandler *handler,
     UNINState *s = container_of(handler, UNINState, data_handler);
     uint32_t val;
 
-    val = pci_data_read(s->host_state.bus,
+    val = pci_data_read(&s->host_state.bus,
                         unin_get_config_reg(s->host_state.config_reg, addr),
                         len);
     UNIN_DPRINTF("read addr %" FMT_PCIBUS " len %d val %x\n", addr, len, val);
@@ -228,12 +228,12 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
     qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     d = FROM_SYSBUS(UNINState, s);
-    d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
-                                         pci_unin_set_irq, pci_unin_map_irq,
-                                         pic, PCI_DEVFN(11, 0), 4);
+    pci_host_bus_init_simple(&d->host_state, &d->busdev.qdev, 0, "pci",
+                             pci_unin_set_irq, pci_unin_map_irq,
+                             pic, PCI_DEVFN(11, 0), 4);
 
 #if 0
-    pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north");
+    pci_create_simple(&d->host_state.bus, PCI_DEVFN(11, 0), "uni-north");
 #endif
 
     sysbus_mmio_map(s, 0, 0xf2800000);
@@ -242,11 +242,11 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
     /* DEC 21154 bridge */
 #if 0
     /* XXX: not activated as PPC BIOS doesn't handle multiple buses properly */
-    pci_create_simple(d->host_state.bus, PCI_DEVFN(12, 0), "dec-21154");
+    pci_create_simple(&d->host_state.bus, PCI_DEVFN(12, 0), "dec-21154");
 #endif
 
     /* Uninorth AGP bus */
-    pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north-agp");
+    pci_create_simple(&d->host_state.bus, PCI_DEVFN(11, 0), "uni-north-agp");
     dev = qdev_create(NULL, "uni-north-agp");
     qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
@@ -256,7 +256,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
     /* Uninorth internal bus */
 #if 0
     /* XXX: not needed for now */
-    pci_create_simple(d->host_state.bus, PCI_DEVFN(14, 0), "uni-north-pci");
+    pci_create_simple(&d->host_state.bus, PCI_DEVFN(14, 0), "uni-north-pci");
     dev = qdev_create(NULL, "uni-north-pci");
     qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
@@ -264,7 +264,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
     sysbus_mmio_map(s, 1, 0xf4c00000);
 #endif
 
-    return d->host_state.bus;
+    return &d->host_state.bus;
 }
 
 PCIBus *pci_pmac_u3_init(qemu_irq *pic)
@@ -280,16 +280,16 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic)
     s = sysbus_from_qdev(dev);
     d = FROM_SYSBUS(UNINState, s);
 
-    d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
-                                         pci_unin_set_irq, pci_unin_map_irq,
-                                         pic, PCI_DEVFN(11, 0), 4);
+    pci_host_bus_init_simple(&d->host_state, &d->busdev.qdev, 0, "pci",
+                             pci_unin_set_irq, pci_unin_map_irq,
+                             pic, PCI_DEVFN(11, 0), 4);
 
     sysbus_mmio_map(s, 0, 0xf0800000);
     sysbus_mmio_map(s, 1, 0xf0c00000);
 
-    pci_create_simple(d->host_state.bus, 11 << 3, "u3-agp");
+    pci_create_simple(&d->host_state.bus, 11 << 3, "u3-agp");
 
-    return d->host_state.bus;
+    return &d->host_state.bus;
 }
 
 static int unin_main_pci_host_init(PCIDevice *d)
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index a76bdfa..4cc588b 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -16,6 +16,7 @@ typedef struct {
     qemu_irq irq[4];
     int realview;
     int mem_config;
+    PCIHostState pci;
 } PCIVPBState;
 
 static inline uint32_t vpb_pci_config_addr(target_phys_addr_t addr)
@@ -125,9 +126,10 @@ static int pci_vpb_init(SysBusDevice *dev)
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(dev, &s->irq[i]);
     }
-    bus = pci_register_bus(&dev->qdev, "pci",
-                           pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
-                           PCI_DEVFN(11, 0), 4);
+    pci_host_bus_init_simple(&s->pci, &dev->qdev, 0, "pci",
+                             pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
+                             PCI_DEVFN(11, 0), 4);
+    bus = &s->pci.bus;
 
     /* ??? Register memory space.  */
 
-- 
1.7.1.1


-- 
yamahata

Re: [Qemu-devel] Re: [PATCH 1/2] pci/bridge: allocate PCIBus dynamically for PCIBridge.

[Michael S. Tsirkin]

On Fri, Jul 16, 2010 at 10:46:52AM +0900, Isaku Yamahata wrote:
> On Thu, Jul 08, 2010 at 07:49:35PM +0300, Michael S. Tsirkin wrote:
> 
> > > For root bus:
> > > - pci_host_bus_new()/pci_host_bus_new_inplace()
> > >   qbus style api. pci segment must be specified.
> > >   New code should use this.
> > 
> > I'd prefer a simple _init which works like _inplace.
> > Allocating memory is simple enough for users.
> 
> How about the following patch?
>   - replaced pci_bus_new(), pci_bus_new_inplace() with
>     pci_host_bus_init()/pci_host_bus_cleanup()
>   - eliminated memory allocation by embedding PCIBus into PCIHostState.
> I've tested it with only piix4 yet.
> If this direction is okay, I'll give it more tests and post it.
> 

Looks good to me. What do others think?  Let's also make
pci_host_bus_init_simple inline.

-- 
MST

[Qemu-devel] [PATCH 2/2] pci/bridge: split out pci bridge code into pci_bridge.c from pci.c

[Isaku Yamahata]

Move pci bridge related code into pci_bridge.c from pci.c
for further enhancement. pci.c is big enough now, so split it out.
No code change but exporting some accesser functions.

In fact, some of pci bridge functions stays in pci.c because
it accesses to PCIBus member. Unstatic the accessor functions
and use them from pci_bridge.c

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 Makefile.objs   |    2 +-
 hw/apb_pci.c    |    1 +
 hw/dec_pci.c    |    1 +
 hw/pci.c        |  168 ++------------------------------------------------
 hw/pci.h        |    9 ++-
 hw/pci_bridge.c |  184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci_bridge.h |   41 ++++++++++++
 7 files changed, 241 insertions(+), 165 deletions(-)
 create mode 100644 hw/pci_bridge.c
 create mode 100644 hw/pci_bridge.h

diff --git a/Makefile.objs b/Makefile.objs
index 2bfb6d1..5c37e5c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -139,7 +139,7 @@ user-obj-y += cutils.o cache-utils.o
 hw-obj-y =
 hw-obj-y += vl.o loader.o
 hw-obj-y += virtio.o virtio-console.o
-hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o
+hw-obj-y += fw_cfg.o pci.o pci_host.o pcie_host.o pci_bridge.o
 hw-obj-y += watchdog.o
 hw-obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
 hw-obj-$(CONFIG_ECC) += ecc.o
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 0ecac55..88ee4a9 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -29,6 +29,7 @@
 #include "sysbus.h"
 #include "pci.h"
 #include "pci_host.h"
+#include "pci_bridge.h"
 #include "rwhandler.h"
 #include "apb_pci.h"
 #include "sysemu.h"
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index ee49d5a..f7a9cdc 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -27,6 +27,7 @@
 #include "sysbus.h"
 #include "pci.h"
 #include "pci_host.h"
+#include "pci_bridge.h"
 
 /* debug DEC */
 //#define DEBUG_DEC
diff --git a/hw/pci.c b/hw/pci.c
index fdf02d0..925d36e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -23,6 +23,7 @@
  */
 #include "hw.h"
 #include "pci.h"
+#include "pci_bridge.h"
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
@@ -286,10 +287,10 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     return bus;
 }
 
-static PCIBus *pci_register_secondary_bus(PCIBus *parent,
-                                          PCIDevice *dev,
-                                          pci_map_irq_fn map_irq,
-                                          const char *name)
+PCIBus *pci_register_secondary_bus(PCIBus *parent,
+                                   PCIDevice *dev,
+                                   pci_map_irq_fn map_irq,
+                                   const char *name)
 {
     PCIBus *bus;
     bus = pci_bus_new(&dev->qdev, name, 0);
@@ -302,7 +303,7 @@ static PCIBus *pci_register_secondary_bus(PCIBus *parent,
     return bus;
 }
 
-static void pci_unregister_secondary_bus(PCIBus *bus)
+void pci_unregister_secondary_bus(PCIBus *bus)
 {
     assert(QLIST_EMPTY(&bus->child));
     QLIST_REMOVE(bus, sibling);
@@ -815,75 +816,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     }
 }
 
-static uint32_t pci_config_get_io_base(PCIDevice *d,
-                                       uint32_t base, uint32_t base_upper16)
-{
-    uint32_t val;
-
-    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
-    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
-        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
-    }
-    return val;
-}
-
-static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
-{
-    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
-        << 16;
-}
-
-static pcibus_t pci_config_get_pref_base(PCIDevice *d,
-                                         uint32_t base, uint32_t upper)
-{
-    pcibus_t tmp;
-    pcibus_t val;
-
-    tmp = (pcibus_t)pci_get_word(d->config + base);
-    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
-    if (tmp & PCI_PREF_RANGE_TYPE_64) {
-        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
-    }
-    return val;
-}
-
-static pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
-{
-    pcibus_t base;
-    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-        base = pci_config_get_io_base(bridge,
-                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
-    } else {
-        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
-            base = pci_config_get_pref_base(
-                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
-        } else {
-            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
-        }
-    }
-
-    return base;
-}
-
-static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
-{
-    pcibus_t limit;
-    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
-        limit = pci_config_get_io_base(bridge,
-                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
-        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
-    } else {
-        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
-            limit = pci_config_get_pref_base(
-                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
-        } else {
-            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
-        }
-        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
-    }
-    return limit;
-}
-
 static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
                               uint8_t type)
 {
@@ -1529,20 +1461,12 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
     return res;
 }
 
-typedef struct {
-    PCIDevice dev;
-    PCIBus *bus;
-    uint32_t vid;
-    uint32_t did;
-} PCIBridge;
-
-
 static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
 {
     pci_update_mappings(d);
 }
 
-static void pci_bridge_update_mappings(PCIBus *b)
+void pci_bridge_update_mappings(PCIBus *b)
 {
     PCIBus *child;
 
@@ -1553,21 +1477,6 @@ static void pci_bridge_update_mappings(PCIBus *b)
     }
 }
 
-static void pci_bridge_write_config(PCIDevice *d,
-                             uint32_t address, uint32_t val, int len)
-{
-    pci_default_write_config(d, address, val, len);
-
-    if (/* io base/limit */
-        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
-
-        /* memory base/limit, prefetchable base/limit and
-           io base/limit upper 16 */
-        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
-        pci_bridge_update_mappings(d->bus);
-    }
-}
-
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 {
     PCIBus *sec;
@@ -1611,48 +1520,6 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
     return bus->devices[PCI_DEVFN(slot, function)];
 }
 
-static int pci_bridge_initfn(PCIDevice *dev)
-{
-    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
-
-    pci_config_set_vendor_id(s->dev.config, s->vid);
-    pci_config_set_device_id(s->dev.config, s->did);
-
-    pci_set_word(dev->config + PCI_STATUS,
-                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
-    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
-    dev->config[PCI_HEADER_TYPE] =
-        (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
-        PCI_HEADER_TYPE_BRIDGE;
-    pci_set_word(dev->config + PCI_SEC_STATUS,
-                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
-    return 0;
-}
-
-static int pci_bridge_exitfn(PCIDevice *pci_dev)
-{
-    PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
-    pci_unregister_secondary_bus(s->bus);
-    return 0;
-}
-
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
-                        uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name)
-{
-    PCIDevice *dev;
-    PCIBridge *s;
-
-    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
-    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
-    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
-    qdev_init_nofail(&dev->qdev);
-
-    s = DO_UPCAST(PCIBridge, dev, dev);
-    s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
-    return s->bus;
-}
-
 PCIDevice *pci_bridge_get_device(PCIBus *bus)
 {
     return bus->parent_dev;
@@ -1931,24 +1798,3 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
                        r->addr, r->addr + r->size - 1);
     }
 }
-
-static PCIDeviceInfo bridge_info = {
-    .qdev.name    = "pci-bridge",
-    .qdev.size    = sizeof(PCIBridge),
-    .init         = pci_bridge_initfn,
-    .exit         = pci_bridge_exitfn,
-    .config_write = pci_bridge_write_config,
-    .is_bridge    = 1,
-    .qdev.props   = (Property[]) {
-        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
-        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
-        DEFINE_PROP_END_OF_LIST(),
-    }
-};
-
-static void pci_register_devices(void)
-{
-    pci_qdev_register(&bridge_info);
-}
-
-device_init(pci_register_devices)
diff --git a/hw/pci.h b/hw/pci.h
index 4164074..3b5bca2 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -213,6 +213,11 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque, int devfn_min, int nirq);
+PCIBus *pci_register_secondary_bus(PCIBus *parent,
+                                   PCIDevice *dev,
+                                   pci_map_irq_fn map_irq,
+                                   const char *name);
+void pci_unregister_secondary_bus(PCIBus *bus);
 
 void pci_bus_set_mem_base(PCIBus *bus, target_phys_addr_t base);
 
@@ -234,10 +239,8 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
 
 void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
-PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
-                        uint16_t vid, uint16_t did,
-                        pci_map_irq_fn map_irq, const char *name);
 PCIDevice *pci_bridge_get_device(PCIBus *bus);
+void pci_bridge_update_mappings(PCIBus *b);
 
 static inline void
 pci_set_byte(uint8_t *config, uint8_t val)
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
new file mode 100644
index 0000000..cf0287d
--- /dev/null
+++ b/hw/pci_bridge.c
@@ -0,0 +1,184 @@
+/*
+ * QEMU PCI bus manager
+ *
+ * Copyright (c) 2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+/*
+ * split out from pci.c
+ * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ */
+
+#include "pci_bridge.h"
+
+typedef struct {
+    PCIDevice dev;
+    PCIBus *bus;
+    uint32_t vid;
+    uint32_t did;
+} PCIBridge;
+
+static uint32_t pci_config_get_io_base(PCIDevice *d,
+                                       uint32_t base, uint32_t base_upper16)
+{
+    uint32_t val;
+
+    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
+    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
+        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
+    }
+    return val;
+}
+
+static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
+{
+    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
+        << 16;
+}
+
+static pcibus_t pci_config_get_pref_base(PCIDevice *d,
+                                         uint32_t base, uint32_t upper)
+{
+    pcibus_t tmp;
+    pcibus_t val;
+
+    tmp = (pcibus_t)pci_get_word(d->config + base);
+    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
+    if (tmp & PCI_PREF_RANGE_TYPE_64) {
+        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
+    }
+    return val;
+}
+
+pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
+{
+    pcibus_t base;
+    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
+        base = pci_config_get_io_base(bridge,
+                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
+    } else {
+        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
+            base = pci_config_get_pref_base(
+                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
+        } else {
+            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
+        }
+    }
+
+    return base;
+}
+
+pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
+{
+    pcibus_t limit;
+    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
+        limit = pci_config_get_io_base(bridge,
+                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
+        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
+    } else {
+        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
+            limit = pci_config_get_pref_base(
+                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
+        } else {
+            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
+        }
+        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
+    }
+    return limit;
+}
+
+static void pci_bridge_write_config(PCIDevice *d,
+                                    uint32_t address, uint32_t val, int len)
+{
+    pci_default_write_config(d, address, val, len);
+
+    if (/* io base/limit */
+        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
+
+        /* memory base/limit, prefetchable base/limit and
+           io base/limit upper 16 */
+        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
+        pci_bridge_update_mappings(d->bus);
+    }
+}
+
+static int pci_bridge_initfn(PCIDevice *dev)
+{
+    PCIBridge *s = DO_UPCAST(PCIBridge, dev, dev);
+
+    pci_config_set_vendor_id(s->dev.config, s->vid);
+    pci_config_set_device_id(s->dev.config, s->did);
+
+    pci_set_word(dev->config + PCI_STATUS,
+                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
+    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
+    dev->config[PCI_HEADER_TYPE] =
+        (dev->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_MULTI_FUNCTION) |
+        PCI_HEADER_TYPE_BRIDGE;
+    pci_set_word(dev->config + PCI_SEC_STATUS,
+                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
+    return 0;
+}
+
+static int pci_bridge_exitfn(PCIDevice *pci_dev)
+{
+    PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
+    pci_unregister_secondary_bus(s->bus);
+    return 0;
+}
+
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
+                        uint16_t vid, uint16_t did,
+                        pci_map_irq_fn map_irq, const char *name)
+{
+    PCIDevice *dev;
+    PCIBridge *s;
+
+    dev = pci_create_multifunction(bus, devfn, multifunction, "pci-bridge");
+    qdev_prop_set_uint32(&dev->qdev, "vendorid", vid);
+    qdev_prop_set_uint32(&dev->qdev, "deviceid", did);
+    qdev_init_nofail(&dev->qdev);
+
+    s = DO_UPCAST(PCIBridge, dev, dev);
+    s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name);
+    return s->bus;
+}
+
+static PCIDeviceInfo bridge_info = {
+    .qdev.name    = "pci-bridge",
+    .qdev.size    = sizeof(PCIBridge),
+    .init         = pci_bridge_initfn,
+    .exit         = pci_bridge_exitfn,
+    .config_write = pci_bridge_write_config,
+    .is_bridge    = 1,
+    .qdev.props   = (Property[]) {
+        DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0),
+        DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void pci_register_devices(void)
+{
+    pci_qdev_register(&bridge_info);
+}
+
+device_init(pci_register_devices)
diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
new file mode 100644
index 0000000..ab06e03
--- /dev/null
+++ b/hw/pci_bridge.h
@@ -0,0 +1,41 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ */
+
+#ifndef QEMU_PCI_BRIDGE_H
+#define QEMU_PCI_BRIDGE_H
+
+#include "pci.h"
+
+pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
+pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
+
+PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
+                        uint16_t vid, uint16_t did,
+                        pci_map_irq_fn map_irq, const char *name);
+
+#endif  /* QEMU_PCI_BRIDGE_H */
+/*
+ * Local variables:
+ *  c-indent-level: 4
+ *  c-basic-offset: 4
+ *  tab-width: 8
+ *  indent-tab-mode: nil
+ * End:
+ */
-- 
1.7.1.1