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

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

On Thu, Jul 08, 2010 at 07:49:35PM +0300, Michael S. Tsirkin wrote:

> > > >   - 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.
> > > 
> > > We have a single caller, right? I think you mean pci_register_bus?
> > > So IIUC, you propose that we add pci_register_host_bus,
> > > and make pci_register_bus a compatibility wrapper?
> > > Sure, let's just add a comment this is deprecated.
> > > 
> > > I am not sure why do we need an API to deal with secondary bus:
> > > it is always a part of the bridge, so all users can and should call
> > > pci_bridge_init?
> > 
> > Okay, then how about the following?
> > 
> > For root bus:
> > - pci_host_bus_new()/pci_host_bus_new_inplace()
> >   qbus style api. pci segment must be specified.
> >   New code should use this.
> 
> I'd prefer a simple _init which works like _inplace.
> Allocating memory is simple enough for users.

I see. Then
- export PCIBus. This is necessary to tell its size.
- pci_host_bus_init()/pci_host_bus_cleanup()
  which don't allocate memory.

> > - pci_bus_new()
> >   qbus style API.
> >   convenience wrapper for compatibility of
> >   pci_host_bus_new(pci segment = 0)
> >   In fact, the only current user piix_pci.c. It's easy to remove it.
> > - pci_register_bus()
> >   old style API. deprecated.
> >   It has been kept for compatibility so far.
> >   This will be gradually replaced with pci_host_bus_new()
> 
> Also, let's make these helpers inline: will make it possible
> to check code by comparing binary after changes.

Agreed.

> 
> > For secondary bus:
> > - pci_bridge_init()
> >   qdev style API.
> >   New code should use this.
> 
> Well - isn't the way we do this a bit backwards?
> I thought the idea was that each device has its own
> PCIDeviceInfo qdev structure, instead of the common pci-bridge.
> 
> And then pci_bridge_init (or _setup to avoid reusing existing names)
> would be a common function that devices can reuse in their init
> functions..

Agreed and in fact I'm having such a patch.
Just I think it's next phase of clean up.
The patch series creates a library for pci bridge as you suggested before
and converts apb_pci.c and dec_pci.c into qdev.

> 
> > - pci_{register, unregister}_secondary_bus():
> >   old stype API. deprecated. 
> >   Keep them only for internal use in pci.c
> >   or they can be easily removed or renamed for qdev style.
> > 
> > For pci device:
> > - pci_create()
> >   qdev style API.
> >   The transitional function until completion of qdev conversion.
> >   If the creation of a device tree from config file is implemented,
> >   this function will be unnecessary.
> > 
> > - pci_create_simple()
> >   qdev style API.
> >   convenience function = pci_create() + qdev_init_nofail()
> > 
> > -- 
> > yamahata
> 

-- 
yamahata