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

[Isaku Yamahata <yamahata@valinux.co.jp>]

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