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

[Thomas Huth]

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