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 <yamah...@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.
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
