| Message ID | 87k3631i29.fsf@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 20, 2014 at 02:06:38PM +0200, Markus Armbruster wrote: > Knut Omang <knut.omang@oracle.com> writes: > > > On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote: > >> Il 20/08/2014 08:53, Knut Omang ha scritto: > >> > A unique bus name is necessary to be able to refer to each instance > >> > from the command line and monitors. > >> > >> Is it needed? Can't you just add id= to the -device option? > > > > Yes, as far as I understand the problem is that the id= would work on > > the ioh3420 device itself, while what is needed here is to name the > > secondary bus of the ioh3420, which I haven't found a way to name from > > the command line. > > Bus names in qdev are a mess. Here are the rules: > > 1. If code provides a name, that's the name. > > 2. Else, if the device has an ID, the name is ID.N, where N counts the > device's buses from zero. > > 3. Else, the name is BUS-TYPE-NAME.N, where N counts the buses of this > type from zero. > > This results in a usable bus name unless device IDs collide with bus > type names, or the code provides names that collide. > > The user needs to take care to use IDs that don't collide with bus > names. Adding new bus names may screw some users. > > The user needs to take further care to use IDs whenever the code > provides a bus name that collides. Adding code that provides bus names > may screw some users. > > Broken by design. > > The problem here is "code provides names that collide": device > q35-pcihost provides the name "pcie.0". Bound to collide with the first > PCIE bus named under rule 3. For instance, if you next add an ioh3420 > without ID, its bus is also named "pcie.0". > > Rule 1 should be taken out and shot. Unfortunately, that'll break ABI > left & right. Instead, we can try to reduce its use. The appended does > exactly that for q35-pcihost. With it applied, the bus provided by > q35-pcihost still gets the same name "pcie.0", but under rule 3 instead > of rule 1. Rule 3 then names further PCIE buses "pcie.1", "pcie.2", ... > instead of "pcie.0", "pcie.1", ... Better, but it's still an ABI break. > > > Maybe an even better solution would be to have default names for > > everything, if not specified, from a user friendliness perspective? > > Buses *have* a default name! You're confusing this with device IDs, > which exist only when the user sets one. > > Changes in this area are difficult, because the names are all ABI. > Names that cannot be used are fair game, of course. > > > I suppose this is a more general issue of sensible default values > > though, but the fact that it is easy to create devices which cannot be > > referred has caused me some confusion from time to time. > > Picking default qdev IDs risks collisions with the user's IDs. We > shouldn't do that. We do it anyway in a few places, for historical > reasons. > > QOM paths might be a sane way to let users refer to devices without IDs. > > > While writing the above, I stumbled another rule 1 screwup: pci_bridge.c > attempts to "improve" the boring standard bus names chosen via rule 2 or > 3. > > pci_bridge_initfn() provides a bus name of its own (commit 8a3d80f > pci_bridge: user-friendly default bus name): > a. If pci_bridge_map_irq() set a bus name, that's the name. > > b. Else, if the device has an ID, that's the name. Thus, ID.N is > "improved" to just ID, at the cost of a special case: now users have to > avoid not just IDs of the form BUS-TYPE-NAME.N, but also plain > BUS-TYPE-NAME. > > Callers of pci_bridge_map_irq() generally provide a name. Some names > contain spaces, thus can't collide (but would be bloody inconvenient on > the command line or in the monitor). Others don't, but thankfully the > ones I checked are in dead code. Craptastic. > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 37f228e..469aafd 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -47,7 +47,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp) > sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem); > sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4); > > - pci->bus = pci_bus_new(DEVICE(s), "pcie.0", > + pci->bus = pci_bus_new(DEVICE(s), NULL, > s->mch.pci_address_space, s->mch.address_space_io, > 0, TYPE_PCIE_BUS); > qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus)); This is for the root bus, I think it won't help Knut who's trying to add devices behind root ports.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Aug 20, 2014 at 02:06:38PM +0200, Markus Armbruster wrote: >> Knut Omang <knut.omang@oracle.com> writes: >> >> > On Wed, 2014-08-20 at 10:52 +0200, Paolo Bonzini wrote: >> >> Il 20/08/2014 08:53, Knut Omang ha scritto: >> >> > A unique bus name is necessary to be able to refer to each instance >> >> > from the command line and monitors. >> >> >> >> Is it needed? Can't you just add id= to the -device option? >> > >> > Yes, as far as I understand the problem is that the id= would work on >> > the ioh3420 device itself, while what is needed here is to name the >> > secondary bus of the ioh3420, which I haven't found a way to name from >> > the command line. >> >> Bus names in qdev are a mess. Here are the rules: >> >> 1. If code provides a name, that's the name. >> >> 2. Else, if the device has an ID, the name is ID.N, where N counts the >> device's buses from zero. >> >> 3. Else, the name is BUS-TYPE-NAME.N, where N counts the buses of this >> type from zero. >> >> This results in a usable bus name unless device IDs collide with bus >> type names, or the code provides names that collide. >> >> The user needs to take care to use IDs that don't collide with bus >> names. Adding new bus names may screw some users. >> >> The user needs to take further care to use IDs whenever the code >> provides a bus name that collides. Adding code that provides bus names >> may screw some users. >> >> Broken by design. >> >> The problem here is "code provides names that collide": device >> q35-pcihost provides the name "pcie.0". Bound to collide with the first >> PCIE bus named under rule 3. For instance, if you next add an ioh3420 >> without ID, its bus is also named "pcie.0". >> >> Rule 1 should be taken out and shot. Unfortunately, that'll break ABI >> left & right. Instead, we can try to reduce its use. The appended does >> exactly that for q35-pcihost. With it applied, the bus provided by >> q35-pcihost still gets the same name "pcie.0", but under rule 3 instead >> of rule 1. Rule 3 then names further PCIE buses "pcie.1", "pcie.2", ... >> instead of "pcie.0", "pcie.1", ... Better, but it's still an ABI break. >> >> > Maybe an even better solution would be to have default names for >> > everything, if not specified, from a user friendliness perspective? >> >> Buses *have* a default name! You're confusing this with device IDs, >> which exist only when the user sets one. >> >> Changes in this area are difficult, because the names are all ABI. >> Names that cannot be used are fair game, of course. >> >> > I suppose this is a more general issue of sensible default values >> > though, but the fact that it is easy to create devices which cannot be >> > referred has caused me some confusion from time to time. >> >> Picking default qdev IDs risks collisions with the user's IDs. We >> shouldn't do that. We do it anyway in a few places, for historical >> reasons. >> >> QOM paths might be a sane way to let users refer to devices without IDs. >> >> >> While writing the above, I stumbled another rule 1 screwup: pci_bridge.c >> attempts to "improve" the boring standard bus names chosen via rule 2 or >> 3. >> >> pci_bridge_initfn() provides a bus name of its own (commit 8a3d80f >> pci_bridge: user-friendly default bus name): >> a. If pci_bridge_map_irq() set a bus name, that's the name. >> >> b. Else, if the device has an ID, that's the name. Thus, ID.N is >> "improved" to just ID, at the cost of a special case: now users have to >> avoid not just IDs of the form BUS-TYPE-NAME.N, but also plain >> BUS-TYPE-NAME. >> >> Callers of pci_bridge_map_irq() generally provide a name. Some names >> contain spaces, thus can't collide (but would be bloody inconvenient on >> the command line or in the monitor). Others don't, but thankfully the >> ones I checked are in dead code. Craptastic. >> >> >> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c >> index 37f228e..469aafd 100644 >> --- a/hw/pci-host/q35.c >> +++ b/hw/pci-host/q35.c >> @@ -47,7 +47,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp) >> sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem); >> sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4); >> >> - pci->bus = pci_bus_new(DEVICE(s), "pcie.0", >> + pci->bus = pci_bus_new(DEVICE(s), NULL, >> s->mch.pci_address_space, s->mch.address_space_io, >> 0, TYPE_PCIE_BUS); >> qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus)); > > This is for the root bus, I think it won't help Knut who's trying to > add devices behind root ports. Read again, more slowly :) Yes, I null the name of the root bus. That makes the qdev machinery derive the very same "pcie.0" name via rule 3 instead of rule 1, with the side effect that future (non-root) PCIE buses get different names. In particular, the next one named via rule 3 will be called "pcie.1" instead of "pcie.0", making it actually accessible.
Il 20/08/2014 15:03, Markus Armbruster ha scritto: >> > >> > This is for the root bus, I think it won't help Knut who's trying to >> > add devices behind root ports. > Read again, more slowly :) > > Yes, I null the name of the root bus. That makes the qdev machinery > derive the very same "pcie.0" name via rule 3 instead of rule 1, with > the side effect that future (non-root) PCIE buses get different names. > In particular, the next one named via rule 3 will be called "pcie.1" > instead of "pcie.0", making it actually accessible. I agree that this is a big improvement. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 20/08/2014 15:03, Markus Armbruster ha scritto: >>> > >>> > This is for the root bus, I think it won't help Knut who's trying to >>> > add devices behind root ports. >> Read again, more slowly :) >> >> Yes, I null the name of the root bus. That makes the qdev machinery >> derive the very same "pcie.0" name via rule 3 instead of rule 1, with >> the side effect that future (non-root) PCIE buses get different names. >> In particular, the next one named via rule 3 will be called "pcie.1" >> instead of "pcie.0", making it actually accessible. > > I agree that this is a big improvement. It's also an ABI break. I'm not saying we can't do it, just that we better consider it carefully.
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 37f228e..469aafd 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -47,7 +47,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp) sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, &pci->data_mem); sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4); - pci->bus = pci_bus_new(DEVICE(s), "pcie.0", + pci->bus = pci_bus_new(DEVICE(s), NULL, s->mch.pci_address_space, s->mch.address_space_io, 0, TYPE_PCIE_BUS); qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus));