Re: [PATCH v2 7/7] hw/arm: Add ID for NPCM7XX SMBus

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> +Markus/Eduardo
>
> On 11/1/21 18:33, Peter Maydell wrote:
>> On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote:
>>>
>>> The ID can be used to indicate SMBus modules when adding
>>> dynamic devices to them.
>>>
>>> Signed-off-by: Hao Wu <wuhaotsh@google.com>
>>> ---
>>>  hw/arm/npcm7xx.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
>>> index 2ab0080e0b..72953d65ef 100644
>>> --- a/hw/arm/npcm7xx.c
>>> +++ b/hw/arm/npcm7xx.c
>>> @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj)
>>>      for (i = 0; i < ARRAY_SIZE(s->smbus); i++) {
>>>          object_initialize_child(obj, "smbus[*]", &s->smbus[i],
>>>                                  TYPE_NPCM7XX_SMBUS);
>>> +        DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i);
>>>      }
>> 
>> This one looks weird to me -- I'm pretty sure we shouldn't be messing
>> about with the DeviceState id string like that. It's supposed to be
>> internal to the QOM/qdev code.

It's meant for the user.  Devices created by code shouldn't set it.  Not
least because any ID they pick could clash with the user's.

> I agree with you, however:
>
> $ git grep -F -- '->id = g_strdup' hw
> hw/arm/virt.c:1512:    dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> hw/block/xen-block.c:761:    drive->id = g_strdup(id);
> hw/block/xen-block.c:853:    iothread->id = g_strdup(id);
> hw/core/machine-qmp-cmds.c:169:        m->id = g_strdup(object_get_canonical_path_component(obj));
> hw/mem/pc-dimm.c:244:        di->id = g_strdup(dev->id);
> hw/pci-bridge/pci_expander_bridge.c:248:        bds->id = g_strdup(dev_name);
> hw/ppc/e500.c:1009:        dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
> hw/s390x/s390-pci-bus.c:1003:            dev->id = g_strdup_printf("auto_%02x:%02x.%01x",
> hw/sh4/sh7750.c:819:    dev->id = g_strdup("sci");
> hw/sh4/sh7750.c:836:    dev->id = g_strdup("scif");
> hw/virtio/virtio-mem-pci.c:69:        vi->id = g_strdup(dev->id);
> hw/virtio/virtio-pmem-pci.c:74:        vi->id = g_strdup(dev->id);

This includes false positives, i.e. assignments to members of structs
other than DeviceState.  It also misses other ways to mess with
DeviceState member id.

Compiling with

    diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
    index 1bad07002d..b17ccd6065 100644
    --- a/include/hw/qdev-core.h
    +++ b/include/hw/qdev-core.h
    @@ -176,7 +176,7 @@ struct DeviceState {
         Object parent_obj;
         /*< public >*/

    -    char *id;
    +    char *const id;
         char *canonical_path;
         bool realized;
         bool pending_deleted_event;

ferrets out the actual assignments.  I got:

../hw/ppc/spapr_vio.c: In function ‘spapr_vio_busdev_realize’:
../hw/ppc/spapr_vio.c:505:22: error: assignment of read-only member ‘id’
  505 |         dev->qdev.id = id;
      |                      ^

This supplies a default ID to a vio-spapr-device.  Feels like a bad
idea.

../softmmu/qdev-monitor.c: In function ‘qdev_set_id’:
../softmmu/qdev-monitor.c:593:21: error: assignment of read-only member ‘id’
  593 |             dev->id = id;
      |                     ^

This assigns the user-supplied ID.

../hw/dma/xlnx-zdma.c: In function ‘zdma_realize’:
../hw/dma/xlnx-zdma.c:777:12: error: assignment of read-only location ‘*r’
  777 |         *r = (RegisterInfo) {
      |            ^

This *clobbers* the DeviceState embedded in *r, including its member id.
Suspicious.

../hw/pci-bridge/pci_expander_bridge.c: In function ‘pxb_dev_realize_common’:
../hw/pci-bridge/pci_expander_bridge.c:248:17: error: assignment of read-only member ‘id’
  248 |         bds->id = g_strdup(dev_name);
      |                 ^

This creates a pci-bridge device and gives it the same ID as the pxb
device being realized.  Doesn't this result in duplicate IDs?!?

../hw/arm/virt.c: In function ‘create_platform_bus’:
../hw/arm/virt.c:1512:13: error: assignment of read-only member ‘id’
 1512 |     dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
      |             ^

Helper function to create a platform-bus-device.  ID is set to
"platform-bus-device".  Feels like a bad idea.

../hw/ppc/e500.c: In function ‘ppce500_init’:
../hw/ppc/e500.c:1009:17: error: assignment of read-only member ‘id’
 1009 |         dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
      |                 ^

Likewise.

../hw/s390x/s390-pci-bus.c: In function ‘s390_pcihost_plug’:
../hw/s390x/s390-pci-bus.c:1003:21: error: assignment of read-only member ‘id’
 1003 |             dev->id = g_strdup_printf("auto_%02x:%02x.%01x",
      |                     ^

This supplies a default ID to a the device being plugged (I think).
Feels like a bad idea.

../hw/sh4/sh7750.c: In function ‘sh7750_init’:
../hw/sh4/sh7750.c:819:13: error: assignment of read-only member ‘id’
  819 |     dev->id = g_strdup("sci");
      |             ^
../hw/sh4/sh7750.c:836:13: error: assignment of read-only member ‘id’
  836 |     dev->id = g_strdup("scif");
      |             ^


These create sh-serial devices.  Their IDs are set to "sci" and "scif",
respectively.  Feels like a bad idea.