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