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

[Michael S. Tsirkin]

On Fri, Jul 09, 2010 at 12:43:18AM +0900, Isaku Yamahata wrote:
> On Thu, Jul 08, 2010 at 05:04:32PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> > > But you claim it's only for root bus, not for secondary bus.
> > 
> > It is currently, isn't it?
> > 
> > > Now I realized why you've rejected such patches so far.
> > > Then, you also mean the current pci_register_secondary_bus() is broken.
> > 
> > Sorry about being dense, what is broken?
> 
> I've regarded pci_bus_new() (or _inplace) as new qdev style API.

The names are pretty bad btw, aren't they?  Would
pci_bus_init/pci_bus_cleanup be better?  And whoever wants to allocate
memory, can do it with malloc, right?

> And pci_register_bus() (or pci_register_secondary_bus()) as old 
> (so deprecated) API.
> So pci_reguster_bus() would be replaced with pci_bus_new() gradually
> like the changeset of 7cd9eee0f6fd6953114068dd98d91fca1237880b
> I've thought that pci_bus_new() is for both root and secondary bus.
> However, according to your comment, the situation seems different.

Forget my comment, it's different according to the code, isn't it?

> > > 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.
> > 
> > IMHO the confusion comes from the fact we have too
> > many functions that do almost, but not quite, the same
> > thing, and the function names do not say anything.
> > 
> > We have a ton of 5 line functions with names like
> > _allocate_inplace, _new, _register, _simple
> 
> Fully Agreed. Some clean up is necessary.
> 
> 
> > >   - 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.

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

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

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