[Qemu-devel] Re: [PATCH v2 5/5] pci_bridge: introduce pci bridge library.

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

On Mon, Jul 12, 2010 at 03:10:00PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 12, 2010 at 07:36:44PM +0900, Isaku Yamahata wrote:
> > diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
> > index ddb2c82..4697c7a 100644
> > --- a/hw/pci_bridge.h
> > +++ b/hw/pci_bridge.h
> > @@ -29,13 +29,27 @@
> >  #include "pci.h"
> >  
> >  PCIDevice *pci_bridge_get_device(PCIBus *bus);
> > +PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
> >  
> > -pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type);
> > -pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type);
> > +pcibus_t pci_bridge_get_base(const PCIDevice *bridge, uint8_t type);
> > +pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type);
> >  
> > -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, bool multifunction,
> > -                        uint16_t vid, uint16_t did,
> > -                        pci_map_irq_fn map_irq, const char *name);
> > +void pci_bridge_write_config(PCIDevice *d,
> > +                             uint32_t address, uint32_t val, int len);
> > +void pci_bridge_reset_reg(PCIDevice *dev);
> > +void pci_bridge_reset(DeviceState *qdev);
> > +
> > +int pci_bridge_initfn(PCIDevice *pci_dev);
> > +int pci_bridge_exitfn(PCIDevice *pci_dev);
> > +
> > +void pci_bridge_qdev_register(PCIDeviceInfo *info);
> > +
> > +PCIBridge *pci_bridge_create(PCIBus *bus, int devfn, bool multifunction,
> > +                             pci_map_irq_fn map_irq,
> > +                             const char *name, const char *bus_name);
> > +PCIBridge *pci_bridge_create_simple(PCIBus *bus, int devfn, bool multifunction,
> > +                                    pci_map_irq_fn map_irq,
> > +                                    const char *name, const char *bus_name);
> 
> The APIs leave much to be desired.  _simple and regular are same?  What does _register do?
> 
> We really should just use qdev: Can't we use pci_qdev_register_many and
> pci_create to create the bridge?  Long term, all pci_create variants
> should go and get replaced with qdev_create.

If struct PCIBridge is exported, those three can be eliminated.
I think it is okay to export struct PCIBridge, but it would not
be a good idea to export PCIBus which is embedded in PCIBridge::sec_bus.

So how should we go?
- export both PCIBus and PCIBridge.

- make PCIBridge::sec_bus pointer, and export PCIBridge.
  And kill register, create, create_simple.
  v1 patch. Although you rejected it, I suppose you didn't see
  this issue.

- introduce wrapper functions to convert types.
  This patch.

- better alternatives?

-- 
yamahata