[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 <address@hidden>
> 
> 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