* ensuring a machine's buses have unique names @ 2021-08-12 11:05 Peter Maydell 2021-08-26 13:00 ` Peter Maydell 2021-08-26 14:08 ` Markus Armbruster 0 siblings, 2 replies; 13+ messages in thread From: Peter Maydell @ 2021-08-12 11:05 UTC (permalink / raw) To: QEMU Developers; +Cc: Markus Armbruster, Philippe Mathieu-Daudé What's the right way to ensure that when a machine has multiple buses of the same type (eg multiple i2c controllers, multiple sd card controllers) they all get assigned unique names so that the user can use '-device ...,bus=some-name' to put a device on a specific bus? For instance in hw/arm/xlnx-zynqmp.c, the SoC object creates a set of alias properties on the SoC for the sd-bus buses that its 4 SD card controllers create. The alias properties are named "sd-bus%d" so they are unique. This works, but it's kind of error-prone because you need each machine model to remember to create these aliases when necessary. mps3-an524 is an example of a machine that fails to do this for its i2c buses, and therefore the user can't usefully tell QEMU which bus to plug a command-line created i2c bus into. Ideally we should make buses get unique names by default and also assert() at startup that there aren't any duplicated names, I think. Side note: is there a way to mark a bus as "do not consider this when plugging devices where the user did not specify the bus themselves" ? Some of the i2c buses on that machine are purely internal to the board (eg there's one that has the touchscreen controller hanging off it and nothing else), and some are "this i2c bus is connected to the expansion port", so ideally if no bus is specified we would want to prefer the expansion-port i2c bus, not the ones that are internal-only. thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-08-12 11:05 ensuring a machine's buses have unique names Peter Maydell @ 2021-08-26 13:00 ` Peter Maydell 2021-08-26 14:08 ` Markus Armbruster 1 sibling, 0 replies; 13+ messages in thread From: Peter Maydell @ 2021-08-26 13:00 UTC (permalink / raw) To: QEMU Developers; +Cc: Markus Armbruster, Philippe Mathieu-Daudé On Thu, 12 Aug 2021 at 12:05, Peter Maydell <peter.maydell@linaro.org> wrote: > > What's the right way to ensure that when a machine has multiple > buses of the same type (eg multiple i2c controllers, multiple > sd card controllers) they all get assigned unique names so that > the user can use '-device ...,bus=some-name' to put a device on a > specific bus? > > For instance in hw/arm/xlnx-zynqmp.c, the SoC object creates > a set of alias properties on the SoC for the sd-bus buses that > its 4 SD card controllers create. The alias properties are named > "sd-bus%d" so they are unique. This works, but it's kind of error-prone > because you need each machine model to remember to create these > aliases when necessary. > > mps3-an524 is an example of a machine that fails to do this > for its i2c buses, and therefore the user can't usefully > tell QEMU which bus to plug a command-line created i2c bus into. > > Ideally we should make buses get unique names by default > and also assert() at startup that there aren't any duplicated > names, I think. > > Side note: is there a way to mark a bus as "do not consider > this when plugging devices where the user did not specify > the bus themselves" ? Some of the i2c buses on that machine > are purely internal to the board (eg there's one that has > the touchscreen controller hanging off it and nothing else), > and some are "this i2c bus is connected to the expansion port", > so ideally if no bus is specified we would want to prefer > the expansion-port i2c bus, not the ones that are internal-only. Ping, in the hopes anybody has an answer to these... -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-08-12 11:05 ensuring a machine's buses have unique names Peter Maydell 2021-08-26 13:00 ` Peter Maydell @ 2021-08-26 14:08 ` Markus Armbruster 2021-09-14 15:25 ` Peter Maydell 1 sibling, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2021-08-26 14:08 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, QEMU Developers, Markus Armbruster Peter Maydell <peter.maydell@linaro.org> writes: > What's the right way to ensure that when a machine has multiple > buses of the same type (eg multiple i2c controllers, multiple > sd card controllers) they all get assigned unique names so that > the user can use '-device ...,bus=some-name' to put a device on a > specific bus? > > For instance in hw/arm/xlnx-zynqmp.c, the SoC object creates > a set of alias properties on the SoC for the sd-bus buses that > its 4 SD card controllers create. The alias properties are named > "sd-bus%d" so they are unique. This works, but it's kind of error-prone > because you need each machine model to remember to create these > aliases when necessary. > > mps3-an524 is an example of a machine that fails to do this > for its i2c buses, and therefore the user can't usefully > tell QEMU which bus to plug a command-line created i2c bus into. Another one used to be isapc. It's not anymore. I believe it's due to commit 61de36761b565a4138d8ad7ec75489ab28fe84b6 Author: Alexander Graf <agraf@csgraf.de> Date: Thu Feb 6 16:08:15 2014 +0100 qdev: Keep global allocation counter per bus When we have 2 separate qdev devices that both create a qbus of the same type without specifying a bus name or device name, we end up with two buses of the same name, such as ide.0 on the Mac machines: dev: macio-ide, id "" bus: ide.0 type IDE dev: macio-ide, id "" bus: ide.0 type IDE If we now spawn a device that connects to a ide.0 the last created bus gets the device, with the first created bus inaccessible to the command line. After some discussion on IRC we concluded that the best quick fix way forward for this is to make automated bus-class type based allocation count a global counter. That's what this patch implements. With this we instead get dev: macio-ide, id "" bus: ide.1 type IDE dev: macio-ide, id "" bus: ide.0 type IDE on the example mentioned above. This also means that if you did -device ...,bus=ide.0 you got a device on the first bus (the last created one) before this patch and get that device on the second one (the first created one) now. Breaks migration unless you change bus=ide.0 to bus=ide.1 on the destination. This is intended and makes the bus enumeration work as expected. As per review request follows a list of otherwise affected boards and the reasoning for the conclusion that they are ok: target machine bus id times ------ ------- ------ ----- aarch64 n800 i2c-bus.0 2 aarch64 n810 i2c-bus.0 2 arm n800 i2c-bus.0 2 arm n810 i2c-bus.0 2 -> Devices are only created explicitly on one of the two buses, using s->mpu->i2c[0], so no change to the guest. aarch64 vexpress-a15 virtio-mmio-bus.0 4 aarch64 vexpress-a9 virtio-mmio-bus.0 4 aarch64 virt virtio-mmio-bus.0 32 arm vexpress-a15 virtio-mmio-bus.0 4 arm vexpress-a9 virtio-mmio-bus.0 4 arm virt virtio-mmio-bus.0 32 -> Makes -device bus= work for all virtio-mmio buses. Breaks migration. Workaround for migration from old to new: specify virtio-mmio-bus.4 or .32 respectively rather than .0 on the destination. aarch64 xilinx-zynq-a9 usb-bus.0 2 arm xilinx-zynq-a9 usb-bus.0 2 mips64el fulong2e usb-bus.0 2 -> Normal USB operation not affected. Migration driver needs command line to use the other bus. i386 isapc ide.0 2 x86_64 isapc ide.0 2 mips mips ide.0 2 mips64 mips ide.0 2 mips64el mips ide.0 2 mipsel mips ide.0 2 ppc g3beige ide.0 2 ppc mac99 ide.0 2 ppc prep ide.0 2 ppc64 g3beige ide.0 2 ppc64 mac99 ide.0 2 ppc64 prep ide.0 2 -> Makes -device bus= work for all IDE buses. Breaks migration. Workaround for migration from old to new: specify ide.1 rather than ide.0 on the destination. Signed-off-by: Alexander Graf <agraf@suse.de> Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Andreas Faerber <afaerber@suse.de> Signed-off-by: Alexander Graf <agraf@suse.de> Note that the automatic bus numbers depend on the order in which board code creates devices. Too implicit and fragile for my taste. But it's been working well enough. > Ideally we should make buses get unique names by default > and also assert() at startup that there aren't any duplicated > names, I think. > > Side note: is there a way to mark a bus as "do not consider > this when plugging devices where the user did not specify > the bus themselves" ? Some of the i2c buses on that machine > are purely internal to the board (eg there's one that has > the touchscreen controller hanging off it and nothing else), > and some are "this i2c bus is connected to the expansion port", > so ideally if no bus is specified we would want to prefer > the expansion-port i2c bus, not the ones that are internal-only. I think the only existing way is to make qbus_is_full() return true. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-08-26 14:08 ` Markus Armbruster @ 2021-09-14 15:25 ` Peter Maydell 2021-09-15 4:28 ` Markus Armbruster 2021-09-22 4:46 ` Markus Armbruster 0 siblings, 2 replies; 13+ messages in thread From: Peter Maydell @ 2021-09-14 15:25 UTC (permalink / raw) To: Markus Armbruster; +Cc: QEMU Developers, Philippe Mathieu-Daudé On Thu, 26 Aug 2021 at 15:08, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > What's the right way to ensure that when a machine has multiple > > buses of the same type (eg multiple i2c controllers, multiple > > sd card controllers) they all get assigned unique names so that > > the user can use '-device ...,bus=some-name' to put a device on a > > specific bus? > Another one used to be isapc. It's not anymore. I believe it's due to > > commit 61de36761b565a4138d8ad7ec75489ab28fe84b6 > Author: Alexander Graf <agraf@csgraf.de> > Date: Thu Feb 6 16:08:15 2014 +0100 > > qdev: Keep global allocation counter per bus > Note that the automatic bus numbers depend on the order in which board > code creates devices. Too implicit and fragile for my taste. But it's > been working well enough. I had a bit of a look into this. I think the problem here is that we created a family of easy-to-misuse APIs and then misused them... The qbus_create() and qbus_create_inplace() functions both take a 'const char *name' argument. If they're passed in NULL then they create an automatically-uniquified name (as per the commit above). If they're passed in a non-NULL string then they use it as-is, whether it's unique in the system or not. We then typically wrap qbus_create() in a bus-specific creation function (examples are scsi_bus_new(), ide_bus_new(), i2c_init_bus()). Mostly those creation functions also take a 'name' argument and pass it through. ide_bus_new() is an interesting exception which does not take a name argument. The easy-to-misuse part is that now we have a set of functions that look like you should pass them in a name (and where there's plenty of code in the codebase that passes in a name) but where that's the wrong thing unless you're a board model and are picking a guaranteed unique name, or you're an odd special case like virtio-scsi. (virtio-scsi is the one caller of scsi_bus_new() that passes in something other than NULL.) In particular for code which is implementing a device that is a whatever-controller, creating a whatever-bus and specifying a name is almost always going to be wrong, because as soon as some machine creates two of these whatever-controllers it has non-unique bus names. It looks like IDE buses are OK because ide_bus_new() takes no name argument, and SCSI buses are OK because the callers correctly pass in NULL, but almost all the "minor" buses (SD, I2C, ipack, aux...) have a lot of incorrect naming of buses in controller models. I'm not sure how best to sort this tangle out. We could: * make controller devices pass in NULL as bus name; this means that some bus names will change, which is an annoying breakage but for these minor bus types we can probably get away with it. This brings these buses into line with how we've been handling uniqueness for ide and scsi. * drop the 'name' argument for buses like ide that don't actually have any callsites that need to pass a name * split into foo_bus_new() and foo_bus_new_named() so that the "easy default" doesn't pass a name, and there's at least a place to put a doc comment explaining that the name passed into the _named() version should be unique ?? * something else ? thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-14 15:25 ` Peter Maydell @ 2021-09-15 4:28 ` Markus Armbruster 2021-09-21 13:20 ` Peter Maydell 2021-09-22 4:46 ` Markus Armbruster 1 sibling, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2021-09-15 4:28 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Markus Armbruster, QEMU Developers Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 26 Aug 2021 at 15:08, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> > What's the right way to ensure that when a machine has multiple >> > buses of the same type (eg multiple i2c controllers, multiple >> > sd card controllers) they all get assigned unique names so that >> > the user can use '-device ...,bus=some-name' to put a device on a >> > specific bus? > >> Another one used to be isapc. It's not anymore. I believe it's due to >> >> commit 61de36761b565a4138d8ad7ec75489ab28fe84b6 >> Author: Alexander Graf <agraf@csgraf.de> >> Date: Thu Feb 6 16:08:15 2014 +0100 >> >> qdev: Keep global allocation counter per bus > >> Note that the automatic bus numbers depend on the order in which board >> code creates devices. Too implicit and fragile for my taste. But it's >> been working well enough. > > I had a bit of a look into this. I think the problem here is that > we created a family of easy-to-misuse APIs and then misused them... Well, "mission accomplished!" > The qbus_create() and qbus_create_inplace() functions both take > a 'const char *name' argument. If they're passed in NULL then > they create an automatically-uniquified name (as per the commit > above). Fine print: if the device providing the bus has an ID (the thing set with id=ID), then the name is ID.N, where N counts from 0 in this device, else it's TYPE.N, where N counts from 0 globally per bus type, and TYPE is the bus's type name converted to lower case. Either scheme produces unique names, but together they need not: $ qemu-system-x86_64 --nodefaults -S -display none -monitor stdio -device intel-hda -device intel-hda,id=hda QEMU 6.1.50 monitor - type 'help' for more information (qemu) info qtree bus: main-system-bus type System [...] dev: i440FX-pcihost, id "" [...] bus: pci.0 type PCI dev: intel-hda, id "hda" [...] bus: hda.0 type HDA dev: intel-hda, id "" [...] bus: hda.0 type HDA [...] Both buses are named "hda.0". Awesome: we made avoiding device IDs that produce bus ID clashes the user's job. To know what to avoid, you need to know your machine type, and the buses provided by the devices you add without ID (which you shouldn't). "Fun" when your machine type evolves. Poorly designed from the start, and then commit 61de36761b5 blew its chance to fix it. > If they're passed in a non-NULL string then they use > it as-is, whether it's unique in the system or not. We then > typically wrap qbus_create() in a bus-specific creation function > (examples are scsi_bus_new(), ide_bus_new(), i2c_init_bus()). > Mostly those creation functions also take a 'name' argument and > pass it through. ide_bus_new() is an interesting exception which > does not take a name argument. > > The easy-to-misuse part is that now we have a set of functions > that look like you should pass them in a name (and where there's > plenty of code in the codebase that passes in a name) but where > that's the wrong thing unless you're a board model and are > picking a guaranteed unique name, or you're an odd special case > like virtio-scsi. (virtio-scsi is the one caller of scsi_bus_new() > that passes in something other than NULL.) In particular for > code which is implementing a device that is a whatever-controller, > creating a whatever-bus and specifying a name is almost always > going to be wrong, because as soon as some machine creates two > of these whatever-controllers it has non-unique bus names. Yes. > It looks like IDE buses are OK because ide_bus_new() takes no > name argument, and SCSI buses are OK because the callers > correctly pass in NULL, but almost all the "minor" buses > (SD, I2C, ipack, aux...) have a lot of incorrect naming of > buses in controller models. > > I'm not sure how best to sort this tangle out. We could: > * make controller devices pass in NULL as bus name; this > means that some bus names will change, which is an annoying > breakage but for these minor bus types we can probably > get away with it. This brings these buses into line with > how we've been handling uniqueness for ide and scsi. To gauge the breakage, we need a list of the affected bus names. > * drop the 'name' argument for buses like ide that don't > actually have any callsites that need to pass a name > * split into foo_bus_new() and foo_bus_new_named() so that > the "easy default" doesn't pass a name, and there's at least > a place to put a doc comment explaining that the name passed > into the _named() version should be unique ?? Yes, please. A bus name setter would be even more discouraging, but is no good, because it can't undo the side effect on the bus type's counter. Omitting foo_bus_new_named() when there is no user feels okay to me. > * something else ? > > thanks > -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-15 4:28 ` Markus Armbruster @ 2021-09-21 13:20 ` Peter Maydell 2021-09-21 15:48 ` BALATON Zoltan 2021-09-22 7:02 ` Markus Armbruster 0 siblings, 2 replies; 13+ messages in thread From: Peter Maydell @ 2021-09-21 13:20 UTC (permalink / raw) To: Markus Armbruster; +Cc: QEMU Developers, Philippe Mathieu-Daudé On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > I'm not sure how best to sort this tangle out. We could: > > * make controller devices pass in NULL as bus name; this > > means that some bus names will change, which is an annoying > > breakage but for these minor bus types we can probably > > get away with it. This brings these buses into line with > > how we've been handling uniqueness for ide and scsi. > > To gauge the breakage, we need a list of the affected bus names. Looking through, there are a few single-use or special purpose buses I'm going to ignore for now (eg vmbus, or the s390 ones). The four big bus types where controllers often specify a bus name and override the 'autogenerate unique name' handling are pci, ssi, sd, and i2c. (pci mostly gets away with it I expect by machines only having one pci bus.) Of those, I've gone through i2c. These are all the places where we create a specifically-named i2c bus (via i2c_init_bus()), together with the affected boards: hw/arm/pxa2xx.c - the PXA SoC code creates both the intended-for-use i2c buses (which get auto-names) and also several i2c buses intended for internal board-code use only which are all given the same name "dummy". Boards: connex, verdex, tosa, mainstone, akita, spitz, borzoi, terrier, z2 hw/arm/stellaris.c - The i2c controller names its bus "i2c". There is only one i2c controller on these boards, so no name conflicts. Boards: lm3s811evb, lm3s6965evb hw/display/ati.c - The ATI VGA device has an on-board i2c controller which it connects the DDC that holds the EDID information. The bus is always named "ati-vga.ddc", so if you have multiple of this PCI device in the system the buses have the same names. hw/display/sm501.c - Same as ATI, but the bus name is "sm501.i2c" hw/i2c/aspeed_i2c.c - This I2C controller has either 14 or 16 (!) different i2c buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,... The board code mostly seems to use these to wire up various on-board i2c devices. Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc hw/i2c/bitbang_i2c.c - the "GPIO to I2C bridge" device always names its bus "i2c". Used only on musicpal, which only creates one of these buses. Boards: musicpal hw/i2c/exynos4210_i2c.c - This i2c controller always names its bus "i2c". There are 9 of these controllers on the board, so they all have clashing names. Boards: nuri, smdkc210 hw/i2c/i2c_mux_pca954x.c - This is an i2c multiplexer. All the child buses are named "i2c-bus". The multiplexer is used by the aspeed and npcm7xx boards. (There's a programmable way to get at individual downstream i2c buses despite the name clash; none of the boards using this multiplexer actually connect any devices downstream of it yet.) Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc, npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc hw/i2c/mpc_i2c.c - This controller always names its bus "i2c". There is only one of these controllers in the machine. Boards: ppce500, mpc8544ds hw/i2c/npcm7xx_smbus.c - This controller always names its bus "i2c-bus". There are multiple controllers on the boards. The name also clashes with the one used by the pca954x muxes on these boards (see above). Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc hw/i2c/pm_smbus.c - This is the PC SMBUS implementation (it is not a QOM device...) The bus is always called "i2c". Boards: haven't worked through; at least all the x86 PC-like boards, I guess hw/i2c/ppc4xx_i2c.c - This controller always names its bus "i2c". The taihu and ref405ep have only one controller, but sam460ex has two which will have non-unique names. Boards: taihu, ref405ep, sam460ex hw/i2c/versatile_i2c.c - This controller always names its bus "i2c". The MPS boards all have multiples of this controller with clashing names; the others have only one controller. Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511, mps2-an505, mps2-an521, mps3-an524, mps3-an547, realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, versatileab, versatilepb, vexpress-a9, vexpress-a15 In a lot of these cases I suspect the i2c controllers are provided either to allow connection of various internal-to-the-board devices, or simply so that guest OS bootup code that initializes the i2c controller doesn't fall over. However since there's nothing stopping users from creating i2c devices themselves on the commandline, some people might be doing that. In some of these cases (eg the i2c bus on the ATI VGA driver) I suspect the desired behaviour is "unique bus name based on a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like we can't do that, though. (Also they probably don't want to permit users to command-line plug i2c devices into it...) thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-21 13:20 ` Peter Maydell @ 2021-09-21 15:48 ` BALATON Zoltan 2021-09-22 4:37 ` Markus Armbruster 2021-09-22 7:02 ` Markus Armbruster 1 sibling, 1 reply; 13+ messages in thread From: BALATON Zoltan @ 2021-09-21 15:48 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Markus Armbruster, QEMU Developers On Tue, 21 Sep 2021, Peter Maydell wrote: > On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >>> I'm not sure how best to sort this tangle out. We could: >>> * make controller devices pass in NULL as bus name; this >>> means that some bus names will change, which is an annoying >>> breakage but for these minor bus types we can probably >>> get away with it. This brings these buses into line with >>> how we've been handling uniqueness for ide and scsi. >> >> To gauge the breakage, we need a list of the affected bus names. > > Looking through, there are a few single-use or special > purpose buses I'm going to ignore for now (eg vmbus, or > the s390 ones). The four big bus types where controllers > often specify a bus name and override the 'autogenerate > unique name' handling are pci, ssi, sd, and i2c. (pci mostly > gets away with it I expect by machines only having one pci > bus.) Of those, I've gone through i2c. These are all the > places where we create a specifically-named i2c bus (via > i2c_init_bus()), together with the affected boards: > > hw/arm/pxa2xx.c > - the PXA SoC code creates both the intended-for-use > i2c buses (which get auto-names) and also several i2c > buses intended for internal board-code use only which > are all given the same name "dummy". > Boards: connex, verdex, tosa, mainstone, akita, spitz, > borzoi, terrier, z2 > hw/arm/stellaris.c > - The i2c controller names its bus "i2c". There is only one i2c > controller on these boards, so no name conflicts. > Boards: lm3s811evb, lm3s6965evb > hw/display/ati.c > - The ATI VGA device has an on-board i2c controller which it > connects the DDC that holds the EDID information. The bus is > always named "ati-vga.ddc", so if you have multiple of this > PCI device in the system the buses have the same names. > hw/display/sm501.c > - Same as ATI, but the bus name is "sm501.i2c" > hw/i2c/aspeed_i2c.c > - This I2C controller has either 14 or 16 (!) different i2c > buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,... > The board code mostly seems to use these to wire up various > on-board i2c devices. > Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, > swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, > tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc > hw/i2c/bitbang_i2c.c > - the "GPIO to I2C bridge" device always names its bus "i2c". > Used only on musicpal, which only creates one of these buses. > Boards: musicpal > hw/i2c/exynos4210_i2c.c > - This i2c controller always names its bus "i2c". There are 9 > of these controllers on the board, so they all have clashing > names. > Boards: nuri, smdkc210 > hw/i2c/i2c_mux_pca954x.c > - This is an i2c multiplexer. All the child buses are named > "i2c-bus". The multiplexer is used by the aspeed and npcm7xx > boards. (There's a programmable way to get at individual > downstream i2c buses despite the name clash; none of the boards > using this multiplexer actually connect any devices downstream of > it yet.) > Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, > swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, > tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc, > npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc > hw/i2c/mpc_i2c.c > - This controller always names its bus "i2c". There is only one > of these controllers in the machine. > Boards: ppce500, mpc8544ds > hw/i2c/npcm7xx_smbus.c > - This controller always names its bus "i2c-bus". There are multiple > controllers on the boards. The name also clashes with the one used > by the pca954x muxes on these boards (see above). > Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc > hw/i2c/pm_smbus.c > - This is the PC SMBUS implementation (it is not a QOM device...) > The bus is always called "i2c". > Boards: haven't worked through; at least all the x86 PC-like > boards, I guess > hw/i2c/ppc4xx_i2c.c > - This controller always names its bus "i2c". The taihu and > ref405ep have only one controller, but sam460ex has two which > will have non-unique names. > Boards: taihu, ref405ep, sam460ex > hw/i2c/versatile_i2c.c > - This controller always names its bus "i2c". The MPS boards all > have multiples of this controller with clashing names; the others > have only one controller. > Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511, > mps2-an505, mps2-an521, mps3-an524, mps3-an547, > realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, > versatileab, versatilepb, vexpress-a9, vexpress-a15 > > In a lot of these cases I suspect the i2c controllers are > provided either to allow connection of various internal-to-the-board > devices, or simply so that guest OS bootup code that initializes > the i2c controller doesn't fall over. However since there's > nothing stopping users from creating i2c devices themselves > on the commandline, some people might be doing that. > > In some of these cases (eg the i2c bus on the ATI VGA driver) > I suspect the desired behaviour is "unique bus name based on > a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like > we can't do that, though. (Also they probably don't want to > permit users to command-line plug i2c devices into it...) To me it looks like device code can't really set a globally unique name on creating the bus without getting some help from upper levels. So maybe naming busses should be done by qdev (or whatever is handling this) instead of passing the name as an argument to qbus_create or only use that name as a unique component within the device and append it to a unique name for the device. Thus we would get names like sys.pci-0.ati-vga-0.ddc or so but not sure we want that as it's hard to use on command line. Alternatively we can accept non unique names but use another unique property such as device id to identify devices which could be generated as an integer incremented after every device add or some hash which would result in shorter unique ids. Such id is already used by -drive and -net options where used supplies a unique id and then can use that to reference the created object by that id in other options. This could be extended to devices and buses if it had a unique id, then it's not a problem to have non-unique names. Regards, BALATON Zoltan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-21 15:48 ` BALATON Zoltan @ 2021-09-22 4:37 ` Markus Armbruster 2021-09-22 9:58 ` BALATON Zoltan 0 siblings, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2021-09-22 4:37 UTC (permalink / raw) To: BALATON Zoltan Cc: Peter Maydell, Philippe Mathieu-Daudé, QEMU Developers BALATON Zoltan <balaton@eik.bme.hu> writes: > On Tue, 21 Sep 2021, Peter Maydell wrote: >> On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote: >>> >>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> I'm not sure how best to sort this tangle out. We could: >>>> * make controller devices pass in NULL as bus name; this >>>> means that some bus names will change, which is an annoying >>>> breakage but for these minor bus types we can probably >>>> get away with it. This brings these buses into line with >>>> how we've been handling uniqueness for ide and scsi. >>> >>> To gauge the breakage, we need a list of the affected bus names. >> >> Looking through, there are a few single-use or special >> purpose buses I'm going to ignore for now (eg vmbus, or >> the s390 ones). The four big bus types where controllers >> often specify a bus name and override the 'autogenerate >> unique name' handling are pci, ssi, sd, and i2c. (pci mostly >> gets away with it I expect by machines only having one pci >> bus.) Of those, I've gone through i2c. These are all the >> places where we create a specifically-named i2c bus (via >> i2c_init_bus()), together with the affected boards: >> >> hw/arm/pxa2xx.c >> - the PXA SoC code creates both the intended-for-use >> i2c buses (which get auto-names) and also several i2c >> buses intended for internal board-code use only which >> are all given the same name "dummy". >> Boards: connex, verdex, tosa, mainstone, akita, spitz, >> borzoi, terrier, z2 >> hw/arm/stellaris.c >> - The i2c controller names its bus "i2c". There is only one i2c >> controller on these boards, so no name conflicts. >> Boards: lm3s811evb, lm3s6965evb >> hw/display/ati.c >> - The ATI VGA device has an on-board i2c controller which it >> connects the DDC that holds the EDID information. The bus is >> always named "ati-vga.ddc", so if you have multiple of this >> PCI device in the system the buses have the same names. >> hw/display/sm501.c >> - Same as ATI, but the bus name is "sm501.i2c" >> hw/i2c/aspeed_i2c.c >> - This I2C controller has either 14 or 16 (!) different i2c >> buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,... >> The board code mostly seems to use these to wire up various >> on-board i2c devices. >> Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, >> swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, >> tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc >> hw/i2c/bitbang_i2c.c >> - the "GPIO to I2C bridge" device always names its bus "i2c". >> Used only on musicpal, which only creates one of these buses. >> Boards: musicpal >> hw/i2c/exynos4210_i2c.c >> - This i2c controller always names its bus "i2c". There are 9 >> of these controllers on the board, so they all have clashing >> names. >> Boards: nuri, smdkc210 >> hw/i2c/i2c_mux_pca954x.c >> - This is an i2c multiplexer. All the child buses are named >> "i2c-bus". The multiplexer is used by the aspeed and npcm7xx >> boards. (There's a programmable way to get at individual >> downstream i2c buses despite the name clash; none of the boards >> using this multiplexer actually connect any devices downstream of >> it yet.) >> Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, >> swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, >> tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc, >> npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc >> hw/i2c/mpc_i2c.c >> - This controller always names its bus "i2c". There is only one >> of these controllers in the machine. >> Boards: ppce500, mpc8544ds >> hw/i2c/npcm7xx_smbus.c >> - This controller always names its bus "i2c-bus". There are multiple >> controllers on the boards. The name also clashes with the one used >> by the pca954x muxes on these boards (see above). >> Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc >> hw/i2c/pm_smbus.c >> - This is the PC SMBUS implementation (it is not a QOM device...) >> The bus is always called "i2c". >> Boards: haven't worked through; at least all the x86 PC-like >> boards, I guess >> hw/i2c/ppc4xx_i2c.c >> - This controller always names its bus "i2c". The taihu and >> ref405ep have only one controller, but sam460ex has two which >> will have non-unique names. >> Boards: taihu, ref405ep, sam460ex >> hw/i2c/versatile_i2c.c >> - This controller always names its bus "i2c". The MPS boards all >> have multiples of this controller with clashing names; the others >> have only one controller. >> Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511, >> mps2-an505, mps2-an521, mps3-an524, mps3-an547, >> realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, >> versatileab, versatilepb, vexpress-a9, vexpress-a15 >> >> In a lot of these cases I suspect the i2c controllers are >> provided either to allow connection of various internal-to-the-board >> devices, or simply so that guest OS bootup code that initializes >> the i2c controller doesn't fall over. However since there's >> nothing stopping users from creating i2c devices themselves >> on the commandline, some people might be doing that. >> >> In some of these cases (eg the i2c bus on the ATI VGA driver) >> I suspect the desired behaviour is "unique bus name based on >> a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like >> we can't do that, though. (Also they probably don't want to >> permit users to command-line plug i2c devices into it...) > > To me it looks like device code can't really set a globally unique > name on creating the bus without getting some help from upper > levels. So maybe naming busses should be done by qdev (or whatever is > handling this) instead of passing the name as an argument to > qbus_create or only use that name as a unique component within the > device and append it to a unique name for the device. Have you read the whole thread? qdev does come up with a name when passed a null pointer. The problem is that we often don't. Another problem is that qdev can come up with non-unique names unless the user is careful with device IDs (the values of -device / device_add parameter id). > Thus we would > get names like sys.pci-0.ati-vga-0.ddc or so but not sure we want that > as it's hard to use on command line. Alternatively we can accept non > unique names but use another unique property such as device id to > identify devices which could be generated as an integer incremented > after every device add or some hash which would result in shorter > unique ids. Such id is already used by -drive and -net options where > used supplies a unique id and then can use that to reference the > created object by that id in other options. This could be extended to > devices and buses if it had a unique id, then it's not a problem to > have non-unique names. > > Regards, > BALATON Zoltan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-22 4:37 ` Markus Armbruster @ 2021-09-22 9:58 ` BALATON Zoltan 2021-09-22 13:07 ` Markus Armbruster 0 siblings, 1 reply; 13+ messages in thread From: BALATON Zoltan @ 2021-09-22 9:58 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Philippe Mathieu-Daudé, QEMU Developers On Wed, 22 Sep 2021, Markus Armbruster wrote: > BALATON Zoltan <balaton@eik.bme.hu> writes: > >> On Tue, 21 Sep 2021, Peter Maydell wrote: >>> On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote: >>>> >>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>>> I'm not sure how best to sort this tangle out. We could: >>>>> * make controller devices pass in NULL as bus name; this >>>>> means that some bus names will change, which is an annoying >>>>> breakage but for these minor bus types we can probably >>>>> get away with it. This brings these buses into line with >>>>> how we've been handling uniqueness for ide and scsi. >>>> >>>> To gauge the breakage, we need a list of the affected bus names. >>> >>> Looking through, there are a few single-use or special >>> purpose buses I'm going to ignore for now (eg vmbus, or >>> the s390 ones). The four big bus types where controllers >>> often specify a bus name and override the 'autogenerate >>> unique name' handling are pci, ssi, sd, and i2c. (pci mostly >>> gets away with it I expect by machines only having one pci >>> bus.) Of those, I've gone through i2c. These are all the >>> places where we create a specifically-named i2c bus (via >>> i2c_init_bus()), together with the affected boards: >>> >>> hw/arm/pxa2xx.c >>> - the PXA SoC code creates both the intended-for-use >>> i2c buses (which get auto-names) and also several i2c >>> buses intended for internal board-code use only which >>> are all given the same name "dummy". >>> Boards: connex, verdex, tosa, mainstone, akita, spitz, >>> borzoi, terrier, z2 >>> hw/arm/stellaris.c >>> - The i2c controller names its bus "i2c". There is only one i2c >>> controller on these boards, so no name conflicts. >>> Boards: lm3s811evb, lm3s6965evb >>> hw/display/ati.c >>> - The ATI VGA device has an on-board i2c controller which it >>> connects the DDC that holds the EDID information. The bus is >>> always named "ati-vga.ddc", so if you have multiple of this >>> PCI device in the system the buses have the same names. >>> hw/display/sm501.c >>> - Same as ATI, but the bus name is "sm501.i2c" >>> hw/i2c/aspeed_i2c.c >>> - This I2C controller has either 14 or 16 (!) different i2c >>> buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,... >>> The board code mostly seems to use these to wire up various >>> on-board i2c devices. >>> Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, >>> swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, >>> tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc >>> hw/i2c/bitbang_i2c.c >>> - the "GPIO to I2C bridge" device always names its bus "i2c". >>> Used only on musicpal, which only creates one of these buses. >>> Boards: musicpal >>> hw/i2c/exynos4210_i2c.c >>> - This i2c controller always names its bus "i2c". There are 9 >>> of these controllers on the board, so they all have clashing >>> names. >>> Boards: nuri, smdkc210 >>> hw/i2c/i2c_mux_pca954x.c >>> - This is an i2c multiplexer. All the child buses are named >>> "i2c-bus". The multiplexer is used by the aspeed and npcm7xx >>> boards. (There's a programmable way to get at individual >>> downstream i2c buses despite the name clash; none of the boards >>> using this multiplexer actually connect any devices downstream of >>> it yet.) >>> Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, >>> swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, >>> tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc, >>> npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc >>> hw/i2c/mpc_i2c.c >>> - This controller always names its bus "i2c". There is only one >>> of these controllers in the machine. >>> Boards: ppce500, mpc8544ds >>> hw/i2c/npcm7xx_smbus.c >>> - This controller always names its bus "i2c-bus". There are multiple >>> controllers on the boards. The name also clashes with the one used >>> by the pca954x muxes on these boards (see above). >>> Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc >>> hw/i2c/pm_smbus.c >>> - This is the PC SMBUS implementation (it is not a QOM device...) >>> The bus is always called "i2c". >>> Boards: haven't worked through; at least all the x86 PC-like >>> boards, I guess >>> hw/i2c/ppc4xx_i2c.c >>> - This controller always names its bus "i2c". The taihu and >>> ref405ep have only one controller, but sam460ex has two which >>> will have non-unique names. >>> Boards: taihu, ref405ep, sam460ex >>> hw/i2c/versatile_i2c.c >>> - This controller always names its bus "i2c". The MPS boards all >>> have multiples of this controller with clashing names; the others >>> have only one controller. >>> Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511, >>> mps2-an505, mps2-an521, mps3-an524, mps3-an547, >>> realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, >>> versatileab, versatilepb, vexpress-a9, vexpress-a15 >>> >>> In a lot of these cases I suspect the i2c controllers are >>> provided either to allow connection of various internal-to-the-board >>> devices, or simply so that guest OS bootup code that initializes >>> the i2c controller doesn't fall over. However since there's >>> nothing stopping users from creating i2c devices themselves >>> on the commandline, some people might be doing that. >>> >>> In some of these cases (eg the i2c bus on the ATI VGA driver) >>> I suspect the desired behaviour is "unique bus name based on >>> a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like >>> we can't do that, though. (Also they probably don't want to >>> permit users to command-line plug i2c devices into it...) >> >> To me it looks like device code can't really set a globally unique >> name on creating the bus without getting some help from upper >> levels. So maybe naming busses should be done by qdev (or whatever is >> handling this) instead of passing the name as an argument to >> qbus_create or only use that name as a unique component within the >> device and append it to a unique name for the device. > > Have you read the whole thread? qdev does come up with a name when No I haven't. This just got my attention because I'm responsible for adding ati-vga.ddc and sm501.i2c and some ppc440 stuff so I was wondering what could I do better bur otherwise I did not check the whole thread so just ignore what I said if it's not useful in this context. Regards, BALATON Zoltan > passed a null pointer. The problem is that we often don't. Another > problem is that qdev can come up with non-unique names unless the user > is careful with device IDs (the values of -device / device_add parameter > id). > >> Thus we would >> get names like sys.pci-0.ati-vga-0.ddc or so but not sure we want that >> as it's hard to use on command line. Alternatively we can accept non >> unique names but use another unique property such as device id to >> identify devices which could be generated as an integer incremented >> after every device add or some hash which would result in shorter >> unique ids. Such id is already used by -drive and -net options where >> used supplies a unique id and then can use that to reference the >> created object by that id in other options. This could be extended to >> devices and buses if it had a unique id, then it's not a problem to >> have non-unique names. >> >> Regards, >> BALATON Zoltan > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-22 9:58 ` BALATON Zoltan @ 2021-09-22 13:07 ` Markus Armbruster 0 siblings, 0 replies; 13+ messages in thread From: Markus Armbruster @ 2021-09-22 13:07 UTC (permalink / raw) To: BALATON Zoltan Cc: Peter Maydell, Philippe Mathieu-Daudé, QEMU Developers BALATON Zoltan <balaton@eik.bme.hu> writes: > On Wed, 22 Sep 2021, Markus Armbruster wrote: >> BALATON Zoltan <balaton@eik.bme.hu> writes: >>> To me it looks like device code can't really set a globally unique >>> name on creating the bus without getting some help from upper >>> levels. So maybe naming busses should be done by qdev (or whatever is >>> handling this) instead of passing the name as an argument to >>> qbus_create or only use that name as a unique component within the >>> device and append it to a unique name for the device. >> >> Have you read the whole thread? qdev does come up with a name when > > No I haven't. This just got my attention because I'm responsible for > adding ati-vga.ddc and sm501.i2c and some ppc440 stuff so I was > wondering what could I do better bur otherwise I did not check the > whole thread so just ignore what I said if it's not useful in this > context. I wouldn't call it "not useful". What you could do better in future code: pass a null bus name. This is not obvious. As Peter noted "we created a family of easy-to-misuse APIs". [...] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-21 13:20 ` Peter Maydell 2021-09-21 15:48 ` BALATON Zoltan @ 2021-09-22 7:02 ` Markus Armbruster 2021-09-22 8:14 ` Cédric Le Goater 1 sibling, 1 reply; 13+ messages in thread From: Markus Armbruster @ 2021-09-22 7:02 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers, Philippe Mathieu-Daudé Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> > I'm not sure how best to sort this tangle out. We could: >> > * make controller devices pass in NULL as bus name; this >> > means that some bus names will change, which is an annoying >> > breakage but for these minor bus types we can probably >> > get away with it. This brings these buses into line with >> > how we've been handling uniqueness for ide and scsi. >> >> To gauge the breakage, we need a list of the affected bus names. > > Looking through, there are a few single-use or special > purpose buses I'm going to ignore for now (eg vmbus, or > the s390 ones). The four big bus types where controllers > often specify a bus name and override the 'autogenerate > unique name' handling are pci, ssi, sd, and i2c. (pci mostly > gets away with it I expect by machines only having one pci > bus.) Of those, I've gone through i2c. These are all the > places where we create a specifically-named i2c bus (via > i2c_init_bus()), together with the affected boards: > > hw/arm/pxa2xx.c > - the PXA SoC code creates both the intended-for-use > i2c buses (which get auto-names) and also several i2c > buses intended for internal board-code use only which > are all given the same name "dummy". > Boards: connex, verdex, tosa, mainstone, akita, spitz, > borzoi, terrier, z2 > hw/arm/stellaris.c > - The i2c controller names its bus "i2c". There is only one i2c > controller on these boards, so no name conflicts. > Boards: lm3s811evb, lm3s6965evb > hw/display/ati.c > - The ATI VGA device has an on-board i2c controller which it > connects the DDC that holds the EDID information. The bus is > always named "ati-vga.ddc", so if you have multiple of this > PCI device in the system the buses have the same names. > hw/display/sm501.c > - Same as ATI, but the bus name is "sm501.i2c" > hw/i2c/aspeed_i2c.c > - This I2C controller has either 14 or 16 (!) different i2c > buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,... > The board code mostly seems to use these to wire up various > on-board i2c devices. > Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, > swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, > tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc > hw/i2c/bitbang_i2c.c > - the "GPIO to I2C bridge" device always names its bus "i2c". > Used only on musicpal, which only creates one of these buses. > Boards: musicpal > hw/i2c/exynos4210_i2c.c > - This i2c controller always names its bus "i2c". There are 9 > of these controllers on the board, so they all have clashing > names. > Boards: nuri, smdkc210 > hw/i2c/i2c_mux_pca954x.c > - This is an i2c multiplexer. All the child buses are named > "i2c-bus". The multiplexer is used by the aspeed and npcm7xx > boards. (There's a programmable way to get at individual > downstream i2c buses despite the name clash; none of the boards > using this multiplexer actually connect any devices downstream of > it yet.) > Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, > swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, > tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc, > npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc > hw/i2c/mpc_i2c.c > - This controller always names its bus "i2c". There is only one > of these controllers in the machine. > Boards: ppce500, mpc8544ds > hw/i2c/npcm7xx_smbus.c > - This controller always names its bus "i2c-bus". There are multiple > controllers on the boards. The name also clashes with the one used > by the pca954x muxes on these boards (see above). > Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc > hw/i2c/pm_smbus.c > - This is the PC SMBUS implementation (it is not a QOM device...) > The bus is always called "i2c". > Boards: haven't worked through; at least all the x86 PC-like > boards, I guess > hw/i2c/ppc4xx_i2c.c > - This controller always names its bus "i2c". The taihu and > ref405ep have only one controller, but sam460ex has two which > will have non-unique names. > Boards: taihu, ref405ep, sam460ex > hw/i2c/versatile_i2c.c > - This controller always names its bus "i2c". The MPS boards all > have multiples of this controller with clashing names; the others > have only one controller. > Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511, > mps2-an505, mps2-an521, mps3-an524, mps3-an547, > realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, > versatileab, versatilepb, vexpress-a9, vexpress-a15 > > In a lot of these cases I suspect the i2c controllers are > provided either to allow connection of various internal-to-the-board > devices, or simply so that guest OS bootup code that initializes > the i2c controller doesn't fall over. However since there's > nothing stopping users from creating i2c devices themselves > on the commandline, some people might be doing that. What are the devices they could add? Anything they could *reasonably* want to add? Breaking "unreasonable" uses could be defendable. The only spot where we set ->bus_type = TYPE_I2C_BUS is i2c_slave_class_init(). Therefore, only the concrete subtypes of TYPE_I2C_SLAVE can go on an i2c bus, which makes sense. These are: TYPE_ADM1272 "adm1272" TYPE_AER915 "aer915" TYPE_AT24C_EE "at24c-eeprom" TYPE_DS1338 "ds1338" "emc1413" "emc1414" TYPE_I2CDDC "i2c-ddc" TYPE_LM8323 "lm8323" TYPE_M41T80 "m41t80" TYPE_MAX34451 "max34451" TYPE_MAX7310 "max7310" TYPE_PCA9546 "pca9546" TYPE_PCA9548 "pca9548" TYPE_PCA9552 "pca9552" TYPE_PXA2XX_I2C_SLAVE "pxa2xx-i2c-slave" TYPE_SII9022 "sii9022" TYPE_SMBUS_DEVICE "smbus-device" TYPE_SMBUS_EEPROM "smbus-eeprom" TYPE_SMBUS_IPMI "smbus-ipmi" TYPE_SSD0303 "ssd0303" TYPE_TMP105 "tmp105" "tmp421" "tmp422" "tmp423" TYPE_TOSA_DAC "tosa_dac" TYPE_TWL92230 "twl92230" TYPE_WM8750 "wm8750" Grep hits in: hw/arm/aspeed.c hw/arm/musicpal.c hw/arm/npcm7xx_boards.c hw/arm/nseries.c hw/arm/pxa2xx.c hw/arm/realview.c hw/arm/spitz.c hw/arm/stellaris.c hw/arm/tosa.c hw/arm/versatilepb.c hw/arm/vexpress.c hw/arm/z2.c hw/audio/marvell_88w8618.c hw/audio/wm8750.c hw/display/ati.c hw/display/i2c-ddc.c hw/display/sii9022.c hw/display/sm501.c hw/display/ssd0303.c hw/display/xlnx_dp.c hw/gpio/max7310.c hw/i2c/i2c_mux_pca954x.c hw/i2c/pmbus_device.c hw/i2c/smbus_eeprom.c hw/i2c/smbus_slave.c hw/input/lm832x.c hw/ipmi/smbus_ipmi.c hw/misc/pca9552.c hw/nvram/eeprom_at24c.c hw/ppc/e500.c hw/ppc/sam460ex.c hw/rtc/ds1338.c hw/rtc/m41t80.c hw/rtc/twl92230.c hw/sensor/adm1272.c hw/sensor/emc141x.c hw/sensor/max34451.c hw/sensor/tmp105.c hw/sensor/tmp421.c include/hw/audio/wm8750.h include/hw/display/i2c-ddc.h include/hw/i2c/i2c_mux_pca954x.h include/hw/i2c/smbus_slave.h include/hw/input/lm832x.h include/hw/misc/pca9552.h include/hw/sensor/tmp105.h tests/qtest/adm1272-test.c tests/qtest/ds1338-test.c tests/qtest/emc141x-test.c tests/qtest/max34451-test.c tests/qtest/pca9552-test.c tests/qtest/tmp105-test.c > > In some of these cases (eg the i2c bus on the ATI VGA driver) > I suspect the desired behaviour is "unique bus name based on > a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like > we can't do that, though. We could add yet another case to qbus_init(): if name is non-null, and bus->parent has a flag set, we use "%s.%d", name, bus->parent->num_child_bus. "Could" doesn not imply "should". > (Also they probably don't want to > permit users to command-line plug i2c devices into it...) Then they should massage the bus so it doesn't accept additional devices. Didn't you post a patch related to this recently? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-22 7:02 ` Markus Armbruster @ 2021-09-22 8:14 ` Cédric Le Goater 0 siblings, 0 replies; 13+ messages in thread From: Cédric Le Goater @ 2021-09-22 8:14 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: QEMU Developers, Philippe Mathieu-Daudé On 9/22/21 09:02, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote: >>> >>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> I'm not sure how best to sort this tangle out. We could: >>>> * make controller devices pass in NULL as bus name; this >>>> means that some bus names will change, which is an annoying >>>> breakage but for these minor bus types we can probably >>>> get away with it. This brings these buses into line with >>>> how we've been handling uniqueness for ide and scsi. >>> >>> To gauge the breakage, we need a list of the affected bus names. >> >> Looking through, there are a few single-use or special >> purpose buses I'm going to ignore for now (eg vmbus, or >> the s390 ones). The four big bus types where controllers >> often specify a bus name and override the 'autogenerate >> unique name' handling are pci, ssi, sd, and i2c. (pci mostly >> gets away with it I expect by machines only having one pci >> bus.) Of those, I've gone through i2c. These are all the >> places where we create a specifically-named i2c bus (via >> i2c_init_bus()), together with the affected boards: >> >> hw/arm/pxa2xx.c >> - the PXA SoC code creates both the intended-for-use >> i2c buses (which get auto-names) and also several i2c >> buses intended for internal board-code use only which >> are all given the same name "dummy". >> Boards: connex, verdex, tosa, mainstone, akita, spitz, >> borzoi, terrier, z2 >> hw/arm/stellaris.c >> - The i2c controller names its bus "i2c". There is only one i2c >> controller on these boards, so no name conflicts. >> Boards: lm3s811evb, lm3s6965evb >> hw/display/ati.c >> - The ATI VGA device has an on-board i2c controller which it >> connects the DDC that holds the EDID information. The bus is >> always named "ati-vga.ddc", so if you have multiple of this >> PCI device in the system the buses have the same names. >> hw/display/sm501.c >> - Same as ATI, but the bus name is "sm501.i2c" >> hw/i2c/aspeed_i2c.c >> - This I2C controller has either 14 or 16 (!) different i2c >> buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,... >> The board code mostly seems to use these to wire up various >> on-board i2c devices. >> Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, >> swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, >> tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc >> hw/i2c/bitbang_i2c.c >> - the "GPIO to I2C bridge" device always names its bus "i2c". >> Used only on musicpal, which only creates one of these buses. >> Boards: musicpal >> hw/i2c/exynos4210_i2c.c >> - This i2c controller always names its bus "i2c". There are 9 >> of these controllers on the board, so they all have clashing >> names. >> Boards: nuri, smdkc210 >> hw/i2c/i2c_mux_pca954x.c >> - This is an i2c multiplexer. All the child buses are named >> "i2c-bus". The multiplexer is used by the aspeed and npcm7xx >> boards. (There's a programmable way to get at individual >> downstream i2c buses despite the name clash; none of the boards >> using this multiplexer actually connect any devices downstream of >> it yet.) >> Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc, >> swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb, >> tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc, >> npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc >> hw/i2c/mpc_i2c.c >> - This controller always names its bus "i2c". There is only one >> of these controllers in the machine. >> Boards: ppce500, mpc8544ds >> hw/i2c/npcm7xx_smbus.c >> - This controller always names its bus "i2c-bus". There are multiple >> controllers on the boards. The name also clashes with the one used >> by the pca954x muxes on these boards (see above). >> Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc >> hw/i2c/pm_smbus.c >> - This is the PC SMBUS implementation (it is not a QOM device...) >> The bus is always called "i2c". >> Boards: haven't worked through; at least all the x86 PC-like >> boards, I guess >> hw/i2c/ppc4xx_i2c.c >> - This controller always names its bus "i2c". The taihu and >> ref405ep have only one controller, but sam460ex has two which >> will have non-unique names. >> Boards: taihu, ref405ep, sam460ex >> hw/i2c/versatile_i2c.c >> - This controller always names its bus "i2c". The MPS boards all >> have multiples of this controller with clashing names; the others >> have only one controller. >> Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511, >> mps2-an505, mps2-an521, mps3-an524, mps3-an547, >> realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9, >> versatileab, versatilepb, vexpress-a9, vexpress-a15 >> >> In a lot of these cases I suspect the i2c controllers are >> provided either to allow connection of various internal-to-the-board >> devices, or simply so that guest OS bootup code that initializes >> the i2c controller doesn't fall over. However since there's >> nothing stopping users from creating i2c devices themselves >> on the commandline, some people might be doing that. > > What are the devices they could add? Anything they could *reasonably* > want to add? Breaking "unreasonable" uses could be defendable. A reasonable dev scenario is to add extra I2C devices on the command line to test a new DT and kernel. C. > > The only spot where we set ->bus_type = TYPE_I2C_BUS is > i2c_slave_class_init(). Therefore, only the concrete subtypes of > TYPE_I2C_SLAVE can go on an i2c bus, which makes sense. These are: > > TYPE_ADM1272 "adm1272" > TYPE_AER915 "aer915" > TYPE_AT24C_EE "at24c-eeprom" > TYPE_DS1338 "ds1338" > "emc1413" > "emc1414" > TYPE_I2CDDC "i2c-ddc" > TYPE_LM8323 "lm8323" > TYPE_M41T80 "m41t80" > TYPE_MAX34451 "max34451" > TYPE_MAX7310 "max7310" > TYPE_PCA9546 "pca9546" > TYPE_PCA9548 "pca9548" > TYPE_PCA9552 "pca9552" > TYPE_PXA2XX_I2C_SLAVE "pxa2xx-i2c-slave" > TYPE_SII9022 "sii9022" > TYPE_SMBUS_DEVICE "smbus-device" > TYPE_SMBUS_EEPROM "smbus-eeprom" > TYPE_SMBUS_IPMI "smbus-ipmi" > TYPE_SSD0303 "ssd0303" > TYPE_TMP105 "tmp105" > "tmp421" > "tmp422" > "tmp423" > TYPE_TOSA_DAC "tosa_dac" > TYPE_TWL92230 "twl92230" > TYPE_WM8750 "wm8750" > > Grep hits in: > > hw/arm/aspeed.c > hw/arm/musicpal.c > hw/arm/npcm7xx_boards.c > hw/arm/nseries.c > hw/arm/pxa2xx.c > hw/arm/realview.c > hw/arm/spitz.c > hw/arm/stellaris.c > hw/arm/tosa.c > hw/arm/versatilepb.c > hw/arm/vexpress.c > hw/arm/z2.c > hw/audio/marvell_88w8618.c > hw/audio/wm8750.c > hw/display/ati.c > hw/display/i2c-ddc.c > hw/display/sii9022.c > hw/display/sm501.c > hw/display/ssd0303.c > hw/display/xlnx_dp.c > hw/gpio/max7310.c > hw/i2c/i2c_mux_pca954x.c > hw/i2c/pmbus_device.c > hw/i2c/smbus_eeprom.c > hw/i2c/smbus_slave.c > hw/input/lm832x.c > hw/ipmi/smbus_ipmi.c > hw/misc/pca9552.c > hw/nvram/eeprom_at24c.c > hw/ppc/e500.c > hw/ppc/sam460ex.c > hw/rtc/ds1338.c > hw/rtc/m41t80.c > hw/rtc/twl92230.c > hw/sensor/adm1272.c > hw/sensor/emc141x.c > hw/sensor/max34451.c > hw/sensor/tmp105.c > hw/sensor/tmp421.c > include/hw/audio/wm8750.h > include/hw/display/i2c-ddc.h > include/hw/i2c/i2c_mux_pca954x.h > include/hw/i2c/smbus_slave.h > include/hw/input/lm832x.h > include/hw/misc/pca9552.h > include/hw/sensor/tmp105.h > tests/qtest/adm1272-test.c > tests/qtest/ds1338-test.c > tests/qtest/emc141x-test.c > tests/qtest/max34451-test.c > tests/qtest/pca9552-test.c > tests/qtest/tmp105-test.c > >> >> In some of these cases (eg the i2c bus on the ATI VGA driver) >> I suspect the desired behaviour is "unique bus name based on >> a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like >> we can't do that, though. > > We could add yet another case to qbus_init(): if name is non-null, and > bus->parent has a flag set, we use > > "%s.%d", name, bus->parent->num_child_bus. > > "Could" doesn not imply "should". > >> (Also they probably don't want to >> permit users to command-line plug i2c devices into it...) > > Then they should massage the bus so it doesn't accept additional > devices. Didn't you post a patch related to this recently? > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ensuring a machine's buses have unique names 2021-09-14 15:25 ` Peter Maydell 2021-09-15 4:28 ` Markus Armbruster @ 2021-09-22 4:46 ` Markus Armbruster 1 sibling, 0 replies; 13+ messages in thread From: Markus Armbruster @ 2021-09-22 4:46 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers, Philippe Mathieu-Daudé Peter Maydell <peter.maydell@linaro.org> writes: [...] > I'm not sure how best to sort this tangle out. We could: > * make controller devices pass in NULL as bus name; this > means that some bus names will change, which is an annoying > breakage but for these minor bus types we can probably > get away with it. This brings these buses into line with > how we've been handling uniqueness for ide and scsi. > * drop the 'name' argument for buses like ide that don't > actually have any callsites that need to pass a name > * split into foo_bus_new() and foo_bus_new_named() so that > the "easy default" doesn't pass a name, and there's at least > a place to put a doc comment explaining that the name passed > into the _named() version should be unique ?? > * something else ? A possible work-around for non-unique bus IDs is QOM paths. Precedence, kind of: commit 6287d827d494b5850049584c3f7fb1a589dbb1de Author: Daniel P. Berrangé <berrange@redhat.com> Date: Fri Sep 11 13:33:56 2015 +0100 monitor: allow device_del to accept QOM paths Currently device_del requires that the client provide the device short ID. device_add allows devices to be created without giving an ID, at which point there is no way to delete them with device_del. The QOM object path, however, provides an alternative way to identify the devices. Allowing device_del to accept an object path ensures all devices are deletable regardless of whether they have an ID. (qemu) device_add usb-mouse (qemu) qom-list /machine/peripheral-anon device[0] (child<usb-mouse>) type (string) (qemu) device_del /machine/peripheral-anon/device[0] Devices are required to be marked as hotpluggable otherwise an error is raised (qemu) device_del /machine/unattached/device[4] Device 'PIIX3' does not support hotplugging Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Message-Id: <1441974836-17476-1-git-send-email-berrange@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> [Commit message touched up, accidental white-space change dropped] Signed-off-by: Markus Armbruster <armbru@redhat.com> Their length makes QOM paths inconvenient for humans, but machines won't care. However, we already burned /-separated paths for paths within the qdev tree (the thing info qtree shows). Friends don't let friends use them (I should be able to dig up a critique if you're curious). Without that, it could be made to work like -device virtio-scsi,id=vscsi -device scsi-cd,bus=/machine/peripheral/vscsi/virtio-backend/vscsi.0 We should consult with libvirt developers before we go down this route. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-22 13:12 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-12 11:05 ensuring a machine's buses have unique names Peter Maydell 2021-08-26 13:00 ` Peter Maydell 2021-08-26 14:08 ` Markus Armbruster 2021-09-14 15:25 ` Peter Maydell 2021-09-15 4:28 ` Markus Armbruster 2021-09-21 13:20 ` Peter Maydell 2021-09-21 15:48 ` BALATON Zoltan 2021-09-22 4:37 ` Markus Armbruster 2021-09-22 9:58 ` BALATON Zoltan 2021-09-22 13:07 ` Markus Armbruster 2021-09-22 7:02 ` Markus Armbruster 2021-09-22 8:14 ` Cédric Le Goater 2021-09-22 4:46 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.