Re: ensuring a machine's buses have unique names

[Markus Armbruster]

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