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


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

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

For secondary bus:
- pci_bridge_init()
  qdev style API.
  New code should use this.

- 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