Cédric Le Goater <[email protected]> writes: > On 1/22/26 14:02, Markus Armbruster wrote: >> Cédric Le Goater <[email protected]> writes: >> >>> On 1/20/26 11:36, Markus Armbruster wrote: >>>> Cc: QOM/qdev maintainers >>>> Cédric Le Goater <[email protected]> writes: >>>> >>>>> Hello, >>>>> >>>>> On 1/19/26 15:25, Markus Armbruster wrote: >>>>>> Cédric Le Goater <[email protected]> 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 <[email protected]> >>>>>>>>> >>>>>>>>> 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/[email protected]/ >>>> >>>> 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/[email protected]/ >>>>>>> >>>>>>> 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!
