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

["Michael S. Tsirkin" <mst@redhat.com>]

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