Re: [PATCH v3 3/5] hw/i2c: parent slaves created with i2c_slave_create_simple

[Markus Armbruster <armbru@redhat.com>]

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

> On 7/1/26 08:29, Philippe Mathieu-Daudé wrote:
>> On 28/6/26 02:45, Emmanuel Blot wrote:
>>> Slaves created with i2c_slave_create_simple() were left unparented and
>>> showed up under /machine/unattached with no stable QOM path. Add each
>>> slave as a QOM child of its bus, named after its I2C address, so it has
>>> a deterministic and addressable QOM path.
>>
>> +Markus
>>
>> Personally I never liked the idea of having buses being parents
>> of the devices attached to them, 
>
> What was the problem ?
>
>> but maybe it is simpler this way.
>
> It is much simpler for functional tests when looking for board
> devices which don't have ids.

A QOM object must be the child of exactly one parent.  This defines the
QOM composition tree.  The link from parent to child is a property of
the parent, and therefore has a name that is unique within its parent.
An object's canonical QOM path is these names on the path from root to
object in the QOM composition tree separated by '/'.

For devices:

* If a device is plugged in with -device / device_add, and it has an ID,
  we make it a child of /machine/peripheral/ with name ID.  If it
  doesn't have an ID, we make it a child of /machine/peripheral-anon/
  with name device[N], where N counts up from zero.  The canonical QOM
  path /machine/peripheral/ID is stable.  The canonical QOM path
  /machine/peripheral-anon/device[N] isn't: it depends on the number of
  devices already there.

* If a device is part of another device, it should be its child.  The
  child's canonical QOM path is the parent's plus '/CHILD-NAME'.  Stable
  as long as the parent's path and the child name are.

* "Should" because we have a lot of code that fails to pick the parent.
  When such a device gets realized, we make it a child of
  /machine/unattached/ orphanage with name device[N], where N counts up
  from zero.  The canonical QOM path /machine/unattached/device[N]
  depends on the number of children already in the orphanage, which
  makes it unstable.

  Letting code get away with not picking a parent was a mistake.  I
  guess it "saved" us some thinking about what's part of what when
  converting existing devices to QOM.  In other words, it enabled sloppy
  hardware modeling.  We've been "saving" thinking ever since.

  I want /machine/unattached/ to be empty.  If an onboard device isn't
  part of another device, put it into /machine/ with a sensible name.

  Whether a device plugged into a bus is part of the device providing
  the bus is a hardware modeling question.

Now to the patch in question.  I understand it makes
i2c_slave_create_simple() pick a parent.  Many machines and devices use
that.  I examined its effect for machine anacapa-bmc with v5 of this
series using "info qom-tree".

The patch moves a bunch of devices from /machine/unattached to

    /machine/soc/i2c/bus[0]/aspeed.i2c.bus.0/0x70 (pca9546)
    /machine/soc/i2c/bus[0]/aspeed.i2c.bus.0/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[1]/aspeed.i2c.bus.1/0x70 (pca9546)
    /machine/soc/i2c/bus[1]/aspeed.i2c.bus.1/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[2]/aspeed.i2c.bus.2/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[3]/aspeed.i2c.bus.3/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[4]/aspeed.i2c.bus.4/0x70 (pca9548)
    /machine/soc/i2c/bus[4]/aspeed.i2c.bus.4/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[5]/aspeed.i2c.bus.5/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[6]/aspeed.i2c.bus.6/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[7]/aspeed.i2c.bus.7/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72 (pca9546)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x22 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.0/0x24 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.1/0x22 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0x72/i2c.1/0x24 (pca9552)
    /machine/soc/i2c/bus[8]/aspeed.i2c.bus.8/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[9]/aspeed.i2c.bus.9/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0x71 (pca9548)
    /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0x71/i2c.5/0x22 (pca9552)
    /machine/soc/i2c/bus[10]/aspeed.i2c.bus.10/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0x71 (pca9548)
    /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0x71/i2c.5/0x22 (pca9552)
    /machine/soc/i2c/bus[11]/aspeed.i2c.bus.11/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[12]/aspeed.i2c.bus.12/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[13]/aspeed.i2c.bus.13/0x70 (pca9548)
    /machine/soc/i2c/bus[13]/aspeed.i2c.bus.13/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[14]/aspeed.i2c.bus.14/0xff (aspeed.i2c.slave)
    /machine/soc/i2c/bus[15]/aspeed.i2c.bus.15/0xff (aspeed.i2c.slave)

Is this a reasonable way to model the hardware?  I can't tell; I don't
know the hardware.  But it sure feels less unreasonable than the
/unattached/ crap!

[...]