All of lore.kernel.org
 help / color / mirror / Atom feed
* 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.