Re: [PATCH v1 0/1] hw/i2c/aspeed: Introduce 'bus-label' to customize bus naming

[Markus Armbruster <armbru@redhat.com>]

Cédric Le Goater <clg@kaod.org> writes:

> On 1/22/26 14:02, Markus Armbruster wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>> 
>>> On 1/20/26 11:36, Markus Armbruster wrote:
>>>> Cc: QOM/qdev maintainers
>>>> Cédric Le Goater <clg@kaod.org> writes:
>>>>
>>>>> Hello,
>>>>>
>>>>> On 1/19/26 15:25, Markus Armbruster wrote:
>>>>>> Cédric Le Goater <clg@kaod.org> writes:
>>>>>>
>>>>>>> On 1/15/26 20:47, Philippe Mathieu-Daudé wrote:
>>>>>>>> Hi,
>>>>>>>> Cc'ing Markus.
>>>>>>>> On 12/1/26 09:30, Kane Chen via qemu development wrote:
>>>>>>>>> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
>>>>>>>>>
>>>>>>>>> Currently, the Aspeed I2C controller uses a static naming convention
>>>>>>>>> for its buses (e.g., "aspeed.i2c.bus.0"). This approach leads to
>>>>>>>>> object name conflicts in machine models that incorporate multiple I2C
>>>>>>>>> controllers, such as an Aspeed SoC paired with an external IO expander
>>>>>>>>> or a co-processor like the AST1700.
>>>>>>>>>
>>>>>>>> Is this a side-effect of Problem 4: 'The /machine/unattached/ orphanage'
>>>>>>>> described here?
>>>>>>>> https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
>>>>
>>>> No, but ...
>>>>
>>>>>>>> This problem isn't specific to I2C nor Aspeed.
>>>>
>>>> ... yes, indeed.  Details below.
>>>>
>>>>>>> See the discussion here :
>>>>>>>
>>>>>>>      https://lore.kernel.org/qemu-devel/006fa26f-6b84-4e82-b6e1-7d1353579441@kaod.org/
>>>>>>>
>>>>>>> The Aspeed SoC has 3*16 I2C buses attached on 3 different I2C
>>>>>>> controllers plus the I2C/I3C buses. We need to find a way to
>>>>>>> distinguish these groups at the QEMU machine level to be able
>>>>>>> to add devices on the right bus when using the command line.
>>>>>>>
>>>>>>> Suggestions welcome !
>>>>>>
>>>>>> Please show me how to start a QEMU with the 48 I2C mentioned above,
>>>>>> complete with output of "info qtree".
>>>>>
>>>>> Clone
>>>>>
>>>>>     https://github.com/legoater/qemu/commits/aspeed-11.0
>>>>>
>>>>> Download
>>>>>
>>>>>     https://github.com/AspeedTech-BMC/openbmc/releases/download/v09.08/ast2700-default-obmc.tar.gz
>>>>>
>>>>> And run :
>>>>>
>>>>>     qemu-system-aarch64 -M ast2700a1-evb -m 8G -smp 4 -net nic,macaddr=,netdev=net0 -netdev user,id=net0,hostfwd=::2207-:22 -drive file=path/to/ast2700-default/image-bmc,format=raw,if=mtd -nographic -serial mon:stdio -snapshot
>>>>>
>>>>> Today, to attach an I2C device on one of the Aspeed SoC I2C buses :
>>>>>
>>>>>     -device tmp105,bus=aspeed.i2c.bus.1,address=0x4d,id=tmp-test
>>>>>
>>>>> The Aspeed SoC I2C bus names follow the "aspeed.i2c.bus.X" format.
>>>>> This is the model typename. The 2 new IO expander models attached
>>>>> to the Aspeed SoC have an extra 16 I2C buses each. These buses use
>>>>> an "ioexpX.Y" name, as proposed in the aspeed-next branch.
>>>>>
>>>>> Attaching a device to one of the IO expanders I2C buses would be :
>>>>>
>>>>>     -device tmp105,bus=ioexp0.1,address=0x4d,id=tmp-test
>>>>>
>>>>> See the qtree below.
>>>>
>>>> Thank you!
>>>
>>> Thank you for taking the time of analyzing the code and history.
>>>
>>>> Machine ast2700a1-evb has QOM objects
>>>>
>>>>      /machine/soc/i2c/ (aspeed.i2c-ast2700)
>>>>        /bus[N] (aspeed.i2c.bus)    for N in 0..15
>>>>          /aspeed.i2c.bus.N (i2c-bus)
>>>>
>>>> in both master and aspeed-11.0.
>>>
>>> Yes, that doesn't change because I would prefer not to break the current
>>> user interface. The "i2c.X" bus name would have been a better choice.
>>> I didn't anticipated that 10y ago when I proposed this model.
>> 
>> We make mistakes, we learn :)
>> 
>>>> Object aspeed.i2c-ast2700 is a sysbus device that contains 16
>>>> aspeed.i2c.bus objects as children bus[N] for N in 0..15.  Each object
>>>> has a property "bus-id" with value N.
>>>>
>>>> Object aspeed.i2c.bus is a sysbus device that contains an i2c-bus object
>>>> as child aspeed.i2c.bus.N.
>>>>
>>>> Aside: why parent TYPE_SYS_BUS_DEVICE, and not TYPE_DEVICE?  It doesn't
>>>> actually plug into a sysbus...
>>>
>>> The AspeedI2CBus model has a couple of MRs and an IRQ. I guess that's why.
>> 
>> Sysbus was a questionable idea from the start.
>> 
>> Qdev was built around the design assumption "each device plugs into a
>> bus provided by a device".  This isn't not how real hardware works, but
>> simplifications can be useful.  However, this one broke down right away:
>> most onboard devices and many other devices don't plug into a
>> recognizable bus.  To make the assumption work, Paul Brook invented the
>> catch all "system bus".  Various infrastructure was then tied to the
>> system bus over time, because it needed to be tied to something, system
>> bus is something, therefore it needed to be tied to the system bus.
>> 
>> The design assumption is long gone.  We still create system bus devices
>> just to be able to use infrastructure.  Blerch.
>> 
>> End of digression.
>> 
>>>> Object i2c-bus is a qbus.
>>>>
>>>> In master, object aspeed.i2c.bus computes the child name by appending .N
>>>> to its own type name TYPE_ASPEED_I2C_BUS.
>>>>
>>>> In aspeed-11.0, it takes it from its property "bus-name".  The property
>>>> is set by its QOM parent aspeed.i2c-ast2700, and computed by appending
>>>> .N to the value of its property "bus-label".  It defaults the property
>>>> to TYPE_ASPEED_I2C_BUS.
>>>>
>>>> Since nothing sets this property in this case, we get the same child
>>>> name.
>>>>
>>>> aspeed-11.0 additionally has QOM objects
>>>>
>>>>      /machine/soc/ioexp[M] (aspeed.ast1700)    for M in 0..1
>>>>          /ioexp-i2c[0] (aspeed.i2c-ast2700)
>>>>            /bus[N] (aspeed.i2c.bus)    for N in 0..15
>>>>              /ioexpM.N (i2c-bus)
>>>>
>>>> Object aspeed.ast1700 is a sysbus device that contains an
>>>> aspeed.i2c-ast2700 as child "ioexp-i2c[0]".  It has a property
>>>> "board-idx" with value M.
>>>>
>>>> Aside: we only ever create one aspeed.i2c-ast2700 child.  Why [0]?
>>>
>>> Right. We can drop [*] in aspeed_ast1700_instance_init() - PATCH 15.
>>>
>>>> Aside^2: I tried to strangle the "[*]" feature in the crib, but failed.
>>>> Has been a minor thorn in my side ever since.
>>>>
>>>> aspeed.ast1700 set its child's (aspeed.i2c-ast2700) property "bus-label"
>>>> to "ioexpM".  This makes the child set the grandchild's (aspeed.i2c.bus)
>>>> property "bus-name" to "ioexpM.N", which makes the grandchild name the
>>>> great-grandchild (i2c-bus) "ioexpM.N".
>>>>
>>>> This naming business is complicated, and I had a hard time ferreting it
>>>> out.  As far as I can tell, it's all in service of -device bus=...
>>>
>>> yes.
>>>
>>> That's why I regret not letting QEMU taking care of the naming with :
>>>
>>>     s->bus = i2c_init_bus(dev, NULL);
>>>
>>> This would break the user interface though. This is still an option.
>> 
>> Since the devices providing these i2c buses have no qdev ID, these buses
>> would then be named i2c.N, where N counts up from 0.  I think.  See "Bus
>> names are defined as follows" below.
>> 
>> Good enough?
>
> Good enough to avoid the bus naming conflict, not good enough
> to easily identify a bus in the machine topology and it's also
> breaking the user interface ... too many cons to be a good
> choice.

How is it breaking the user interface?

Mind, I didn't mean to propose changing existing bus names, i.e. the bus
names "aspeed.i2c.bus.N" of the i2c bus objects at
/machine/soc/i2c/bus[N]/aspeed.i2c_init_bus.N.  Only the bus names of
the new i2c bus objects at
/machine/soc/ioexp[M]/ioexp-i2c[0]/bus[N]/NEW-BUS-OBJECT.

>>>> Let's examine how that works.
>>>>
>>>> We want to be able to plug i2c devices into any of these 48 i2c buses
>>>> with -device / device_add.  To do that, we need to select a bus with the
>>>> "bus" argument.
>>>>
>>>> In a world saner than the one we live in, the value of "bus" would be a
>>>> QOM path or qdev ID, where qdev ID is shorthand for the QOM path
>>>> /machine/peripheral/ID.
>> 
>> Nonsense.
>> 
>> "QOM path or qdev ID" is how the @id argument of device_del and
>> device_sync_config work.  A qdev ID resolves to a qdev.  Here, we need
>> to resolve to a qbus.
>> 
>> Instead, "QOM path or qbus ID".  Except the concept "qbus ID" does not
>> exist.  All we have is qbus names, which aren't unique (we screwed that
>> up).
>> 
>>>> For instance, the first i2c bus could then be selected with absolute QOM
>>>> path "/machine/soc/i2c/bus[0]" or partial QOM paths "soc/i2c/bus[0]" or
>>>> "i2c/bus[0]".  Partial QOM paths are a convenience feature that is
>>>> virtually unknown (and possibly ill-advised): you can omit leading path
>>>> components as long as there's no ambiguity.
>>>>
>>>> However, in the world we live in, the value of bus is not a QOM path,
>>>> it's a path in the qtree.  Why?  qdev and -device / device_add predate
>>>> QOM.
>>>>
>>>> If the path starts with "/", it's anchored at the main system bus.
>>>>
>>>> Else, it's anchored at a bus whose name is the first path component.  If
>>>> there's more than one such bus, we pick the first one we find.  This is
>>>> a misfeature.
>>>>
>>>> Remaining path components, if any, pick a path in the qtree from that
>>>> anchor towards leaves.  Note that the child of a qbus is always a qdev,
>>>> and the child of a qdev always a qbus.
>>>>
>>>> This must ultimately resolve to a qbus of the appropriate type.
>>>>
>>>> Picking a qdev child of a qbus works like this:
>>>>
>>>> * If a child with a (user-specified) qdev ID equal to the path component
>>>>   exists, pick it.  Since qdev IDs are unique, there can only be one.
>>>>
>>>> * Else, if children whose QOM type name equals the path component
>>>>   exists, pick the first one.
>>>>
>>>> * Else, if children whose qdev alias equals the path component exists,
>>>>   pick the first one.
>>>>
>>>> Picking the first one is again a misfeature.
>>>>
>>>> Picking a qbus child is simpler: we pick the first child whose bus name
>>>> equals the path component.
>>>>
>>>> Bus names are defined as follows:
>>>>
>>>> * Whatever creates the bus may set its name.
>>>>
>>>> * Else, if the qbus's parent qdev has an ID, the bus name is ID.N, where
>>>>   N counts up from 0 within that qdev.
>>>>
>>>> * Else, the bus name is TYPE.N, where TYPE is the parent qdev's QOM type
>>>>   name, and N counts up from 0 within that bus class.
>> 
>> The qbus's QOM type name, of course.
>> 
>>>> The only case where this is actually works is picking the N-th bus child
>>>> provided by a qdev with an ID: use bus=ID.N (a partial tree path of just
>>>> one component).  Anything else is unfit for purpose, except in special
>>>> cases, e.g. when the machine can have just one device of a certain type.
>>>>
>>>> This mess is harmless for user-created devices: just specify the ID.
>>>> It's awful for onboard devices, which cannot have an ID.
>>>>
>>>> This is a qdev design flaw.  It's not specific to I2C or Aspeed, as
>>>> Philippe suspected.
>>>>
>>>> To illustrate it further, let's have a look at the qtree of machine
>>>> ast2700a1-evb.  Output of "info qtree" in master:
>>>>
>>>>      bus: main-system-bus
>>>>        type System
>>>>        [...]
>>>>        dev: aspeed.i2c.bus, id ""    for N in 0..15
>>>>          [...]
>>>>          bus: aspeed.i2c.bus.N
>>>>            type i2c-bus
>>>>            [...]
>>>>        In aspeed-11.0, we additionally have
>>>>
>>>>        dev: aspeed.i2c.bus, id ""    for M in 0..1, N in 0..15
>>>>          [...]
>>>>          bus: ioexpM.N
>>>>            type i2c-bus
>>>>            [...]
>>>>
>>>> The i2c-buses all have unique names: aspeed.i2c-bus.N and ioexpM.N.  We
>>>> can point to any of them with a partial qtree path of just one
>>>> component: bus=NAME where NAME is one of these unique names works, and
>>>> there is no ambiguity.
>>>>
>>>> The buses have unique names only because device code takes pains to make
>>>> them configurable by parent devices, and the parent devices cooperate to
>>>> configure them so the resulting bus names are unique.
>>>>
>>>> This is a lot of complexity to work around this qdev design flaw for
>>>> just one special instance.
>>>>
>>>> Can we instead remedy the design flaw once and for all?
>>>>
>>>> Here't the obvious stupid idea: give -device / device_add the means to
>>>> pick a bus by QOM path.
>>>>
>>>> Thoughts?
>>>
>>> Could we support both methods ? I mean looking up the bus by the
>>> qtree name and by the QOM path ?
>> 
>> We can't just change "bus".  Management applications and human users
>> rely on it.
>> 
>> Except we might be able to break aspects that aren't actually used in
>> anger.  Is anyone using '/' in qtree paths in anger?  I sure hope nobody
>> does, because that way is madness.  But it's hard to be reasonably sure.
>
> Could we add a prefix to the bus name argument to signify a change of
> namespace for the lookup ? like 'qom:' or 'machine:' ?

Hmm.

The value of "bus" is a path in the qtree.  It either starts with '/'
(main system bus) or a bus name.  Recall that a bus name can be

* set by whatever creates the bus

* ID.N, where ID is the parent qdev's ID.

* TYPE.N, where TYPE is the bus's QOM type name.

TYPE.N start with "qom:" or "machine:", because QOM type names cannot
create ':' since commit b447378e121 (qom/object: Limit type names to
alphanumerical and some few special characters).

ID.N shouldn't start with "qom:" or "machine:", because

* Users cannot pick qdev IDs containing ':' since commit b560a9ab9be
  (qemu-option: Reject anti-social IDs).  Except they can, because we
  regressed in 9.2 I'll post a patch.

* Code should not pick such qdev IDs (but it totally could).

Likewise, code setting bus names containing ':' would be unwise (but it
totally could).

I'm not sure we want to rely on this particular house of cards.

As so often, the combination of ill-advised convenience features and
equally ill-advised loose syntax bites us in the butt.

>
> Thanks !
>
> C.
>
>> If we can break '/' in the value of "bus", we could overload "bus": make
>> it a QOM path or a qbus ID, similar to how device_del's @id is a QOM
>> path or a qdev ID.  We'd have to define "qbus ID".
>> 
>> Instead, we could perhaps add an argument "parent" to specify the QOM
>> parent object, mutually exclusive with "bus".  For devices that plug
>> into a certain kind of bus, the parent object must be an instance of
>> that bus.
>> 
>>> Thanks,
>>>
>>> C.
>> 
>> You're welcome!