[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 <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.

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