Re: [PATCH v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices

[Thomas Huth <thuth@redhat.com>]

On 20/10/2023 11.54, Juan Quintela wrote:
> Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
>> Am 20.10.23 um 11:07 schrieb Juan Quintela:
>>> Just with make check I can see that we can have more than one of this
>>> devices, so use ANY.
>>> ok 5 /s390x/device/introspect/abstract-interfaces
>>> ...
>>> Broken pipe
>>> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195:
>>> kill_qemu() tried to terminate QEMU process but encountered exit
>>> status 1 (expected 0)
>>> Aborted (core dumped)
>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>    hw/s390x/s390-skeys.c    | 3 ++-
>>>    hw/s390x/s390-stattrib.c | 3 ++-
>>>    2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> Actually both devices should be theŕe only once - I think.
> 
> Reverting the patch (but with the check that we don't add duplicated
> entries):
> 
> # Testing device 's390-skeys-qemu'
> Broken pipe
> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:194: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
> Aborted (core dumped)
> $
> 
> This is device-intraspect-test.
> 
> Somehow this function decides that you can hotplug this two s390
> devices, if that is not the case, they need to be marked somehow not
> hot-plugabble.

Sorry, no, it's not hot-plugging what is happening here, it's device 
introspection. That means it should always be possible to create a second 
instance of a device for introspection - it must just not be realized if 
there can be only one instance.

Looking at the code here:

tatic inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
                                             Error **errp)
{
     S390SKeysState *ss = S390_SKEYS(obj);

     /* Prevent double registration of savevm handler */
     if (ss->migration_enabled == value) {
         return;
     }

     ss->migration_enabled = value;

     if (ss->migration_enabled) {
         register_savevm_live(TYPE_S390_SKEYS, 0, 1,
                              &savevm_s390_storage_keys, ss);
     } else {
         unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
     }
}

static void s390_skeys_instance_init(Object *obj)
{
     object_property_add_bool(obj, "migration-enabled",
                              s390_skeys_get_migration_enabled,
                              s390_skeys_set_migration_enabled);
     object_property_set_bool(obj, "migration-enabled", true, NULL);
}

I think the problem is the object_property_set_bool() in the 
_instance_init() function. The setting of the property should maybe rather 
happen during the realization instead?

  Thomas