diff mbox series

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

Message ID 20231020090731.28701-7-quintela@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: Check for duplicates on vmstate_register() | expand

Commit Message

Juan Quintela Oct. 20, 2023, 9:07 a.m. UTC 
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(-)

Comments

Christian Borntraeger Oct. 20, 2023, 9:26 a.m. UTC | #1 
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.

> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 5024faf411..ef089e1967 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -22,6 +22,7 @@
>   #include "sysemu/kvm.h"
>   #include "migration/qemu-file-types.h"
>   #include "migration/register.h"
> +#include "migration/vmstate.h"
>   
>   #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
>   #define S390_SKEYS_SAVE_FLAG_EOS 0x01
> @@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
>       ss->migration_enabled = value;
>   
>       if (ss->migration_enabled) {
> -        register_savevm_live(TYPE_S390_SKEYS, 0, 1,
> +        register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
>                                &savevm_s390_storage_keys, ss);
>       } else {
>           unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index 220e845d12..055d382c3c 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -13,6 +13,7 @@
>   #include "qemu/units.h"
>   #include "migration/qemu-file.h"
>   #include "migration/register.h"
> +#include "migration/vmstate.h"
>   #include "hw/s390x/storage-attributes.h"
>   #include "qemu/error-report.h"
>   #include "exec/ram_addr.h"
> @@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
>   {
>       S390StAttribState *sas = S390_STATTRIB(obj);
>   
> -    register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
> +    register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
>                            &savevm_s390_stattrib_handlers, sas);
>   
>       object_property_add_bool(obj, "migration-enabled",
Juan Quintela Oct. 20, 2023, 9:54 a.m. UTC | #2 
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.

static void test_one_device(QTestState *qts, const char *type)
{
    QDict *resp;
    char *help, *escaped;
    GRegex *comma;

    g_test_message("Testing device '%s'", type);

    resp = qtest_qmp(qts, "{'execute': 'device-list-properties',"
                          " 'arguments': {'typename': %s}}",
               type);
    qobject_unref(resp);

    comma = g_regex_new(",", 0, 0, NULL);
    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
    g_regex_unref(comma);

    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
    g_free(help);
    g_free(escaped);
}

Thanks, Juan.



>
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 5024faf411..ef089e1967 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -22,6 +22,7 @@
>>   #include "sysemu/kvm.h"
>>   #include "migration/qemu-file-types.h"
>>   #include "migration/register.h"
>> +#include "migration/vmstate.h"
>>     #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k
>> storage keys */
>>   #define S390_SKEYS_SAVE_FLAG_EOS 0x01
>> @@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
>>       ss->migration_enabled = value;
>>         if (ss->migration_enabled) {
>> -        register_savevm_live(TYPE_S390_SKEYS, 0, 1,
>> +        register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
>>                                &savevm_s390_storage_keys, ss);
>>       } else {
>>           unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>> index 220e845d12..055d382c3c 100644
>> --- a/hw/s390x/s390-stattrib.c
>> +++ b/hw/s390x/s390-stattrib.c
>> @@ -13,6 +13,7 @@
>>   #include "qemu/units.h"
>>   #include "migration/qemu-file.h"
>>   #include "migration/register.h"
>> +#include "migration/vmstate.h"
>>   #include "hw/s390x/storage-attributes.h"
>>   #include "qemu/error-report.h"
>>   #include "exec/ram_addr.h"
>> @@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
>>   {
>>       S390StAttribState *sas = S390_STATTRIB(obj);
>>   -    register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
>> +    register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
>>                            &savevm_s390_stattrib_handlers, sas);
>>         object_property_add_bool(obj, "migration-enabled",
Thomas Huth Oct. 20, 2023, 10:04 a.m. UTC | #3 
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
Thomas Huth Oct. 20, 2023, 3:08 p.m. UTC | #4 
On 20/10/2023 11.07, Juan Quintela wrote:
> 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(-)

Please use this patch series instead:

  https://lore.kernel.org/qemu-devel/20231020150554.664422-1-thuth@redhat.com/

  Thanks,
   Thomas
diff mbox series

Patch

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..ef089e1967 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -22,6 +22,7 @@ 
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
+#include "migration/vmstate.h"
 
 #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
 #define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -457,7 +458,7 @@  static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
     ss->migration_enabled = value;
 
     if (ss->migration_enabled) {
-        register_savevm_live(TYPE_S390_SKEYS, 0, 1,
+        register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
                              &savevm_s390_storage_keys, ss);
     } else {
         unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 220e845d12..055d382c3c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -13,6 +13,7 @@ 
 #include "qemu/units.h"
 #include "migration/qemu-file.h"
 #include "migration/register.h"
+#include "migration/vmstate.h"
 #include "hw/s390x/storage-attributes.h"
 #include "qemu/error-report.h"
 #include "exec/ram_addr.h"
@@ -380,7 +381,7 @@  static void s390_stattrib_instance_init(Object *obj)
 {
     S390StAttribState *sas = S390_STATTRIB(obj);
 
-    register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
+    register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
                          &savevm_s390_stattrib_handlers, sas);
 
     object_property_add_bool(obj, "migration-enabled",