| Message ID | 8ab6e42be20518d6491e0cd0d5985f7df6912c01.1278037560.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
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? > 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 <yamahata@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. Let's just put PCIBus in a header? It could be a new header named pci_internals.h or something like this. > --- > 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. > } > > 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
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 <yamahata@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 >
On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote: > 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 <yamahata@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. I am not sure myself yet. Generally I'm not very happy with how interrupts are handled. Specifically: - lots of indirect calls through qemu_irq not type-safe, hard to debug and can not be good for performance need to find a way to chase these pointers at setup time - lots of loops over irq pins and over buses need to precompute and store at setup time, and use bits for booleans - information is duplicated, e.g. piix duplicates irq states need to use from a single place with the last issue, be careful not to break migration: we need to compute and store old data on migration In case of piix_pci interrupts are controlled through PIIX3 device, so we create the host bus, the device on it, and finally make another call to make interrupts on the bus get device as the opaque pointer. All this looks very convoluted. > > > 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
On Wed, Jul 07, 2010 at 02:47:10PM +0300, Michael S. Tsirkin wrote: > > > 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. > > I am not sure myself yet. Generally I'm not very happy with how > interrupts are handled. > > Specifically: > - lots of indirect calls through qemu_irq > not type-safe, hard to debug and can not be good for performance > need to find a way to chase these pointers at setup time > - lots of loops over irq pins and over buses > need to precompute and store at setup time, and use bits for booleans > - information is duplicated, e.g. piix duplicates irq states > need to use from a single place > with the last issue, be careful not to break migration: > we need to compute and store old data on migration > > In case of piix_pci interrupts are controlled through PIIX3 device, so > we create the host bus, the device on it, and finally make another call > to make interrupts on the bus get device as the opaque pointer. > All this looks very convoluted. I see, it's concern about over all piix_pci. Can you please comment on pci_bus_new() issue below? I'm afraid that you missed it. > > > > --- > > > > 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.
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 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 > - 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?
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()
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
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 >
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; } 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)
allocate PCIBus dynamically for PCIBridge and bug fix of pci_unregister_secondary_bus(). 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 <yamahata@valinux.co.jp> --- hw/pci.c | 25 ++++++++++++++----------- 1 files changed, 14 insertions(+), 11 deletions(-)