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
