diff mbox series

[02/13] s390_flic: add migration-enabled property

Message ID 20240509170044.190795-3-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series fix --without-default-devices build and (mostly) tests | expand

Commit Message

Paolo Bonzini May 9, 2024, 5 p.m. UTC
Instead of mucking with css_migration_enabled(), add a property specific to
the FLIC device, similar to what is done for TYPE_S390_STATTRIB.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/s390x/s390_flic.h | 1 +
 hw/intc/s390_flic.c          | 6 +++++-
 hw/s390x/s390-virtio-ccw.c   | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

Comments

Thomas Huth May 10, 2024, 5:06 a.m. UTC | #1
On 09/05/2024 19.00, Paolo Bonzini wrote:
> Instead of mucking with css_migration_enabled(), add a property specific to
> the FLIC device, similar to what is done for TYPE_S390_STATTRIB.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/hw/s390x/s390_flic.h | 1 +
>   hw/intc/s390_flic.c          | 6 +++++-
>   hw/s390x/s390-virtio-ccw.c   | 1 +
>   3 files changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

(BTW: It's really good that Daniel's patch series is going to mark the old 
machine types as deprecated, too ... migration stuff has been so hacky in 
the 2.x days...)

> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
> index 3907a13d076..bcb081def58 100644
> --- a/include/hw/s390x/s390_flic.h
> +++ b/include/hw/s390x/s390_flic.h
> @@ -47,6 +47,7 @@ struct S390FLICState {
>       /* to limit AdapterRoutes.num_routes for compat */
>       uint32_t adapter_routes_max_batch;
>       bool ais_supported;
> +    bool migration_enabled;
>   };
>   
>   
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index f4a848460b8..7f930800877 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -405,6 +405,8 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data)
>   static Property s390_flic_common_properties[] = {
>       DEFINE_PROP_UINT32("adapter_routes_max_batch", S390FLICState,
>                          adapter_routes_max_batch, ADAPTER_ROUTES_MAX_GSI),
> +    DEFINE_PROP_BOOL("migration-enabled", S390FLICState,
> +                     migration_enabled, true),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -457,7 +459,9 @@ type_init(qemu_s390_flic_register_types)
>   
>   static bool adapter_info_so_needed(void *opaque)
>   {
> -    return css_migration_enabled();
> +    S390FLICState *fs = S390_FLIC_COMMON(opaque);
> +
> +    return fs->migration_enabled;
>   }
>   
>   const VMStateDescription vmstate_adapter_info_so = {
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index feabc173eb3..1383e47eeb5 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -1174,6 +1174,7 @@ static void ccw_machine_2_9_class_options(MachineClass *mc)
>       S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>       static GlobalProperty compat[] = {
>           { TYPE_S390_STATTRIB, "migration-enabled", "off", },
> +        { TYPE_S390_FLIC_COMMON, "migration-enabled", "off", },
>       };
>   
>       ccw_machine_2_10_class_options(mc);
Marc Hartmayer May 16, 2024, 2:42 p.m. UTC | #2
On Thu, May 09, 2024 at 07:00 PM +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Instead of mucking with css_migration_enabled(), add a property specific to
> the FLIC device, similar to what is done for TYPE_S390_STATTRIB.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/hw/s390x/s390_flic.h | 1 +
>  hw/intc/s390_flic.c          | 6 +++++-
>  hw/s390x/s390-virtio-ccw.c   | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
> index 3907a13d076..bcb081def58 100644
> --- a/include/hw/s390x/s390_flic.h
> +++ b/include/hw/s390x/s390_flic.h
> @@ -47,6 +47,7 @@ struct S390FLICState {
>      /* to limit AdapterRoutes.num_routes for compat */
>      uint32_t adapter_routes_max_batch;
>      bool ais_supported;
> +    bool migration_enabled;
>  };
>  
>  
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index f4a848460b8..7f930800877 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -405,6 +405,8 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data)
>  static Property s390_flic_common_properties[] = {
>      DEFINE_PROP_UINT32("adapter_routes_max_batch", S390FLICState,
>                         adapter_routes_max_batch, ADAPTER_ROUTES_MAX_GSI),
> +    DEFINE_PROP_BOOL("migration-enabled", S390FLICState,
> +                     migration_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -457,7 +459,9 @@ type_init(qemu_s390_flic_register_types)
>  
>  static bool adapter_info_so_needed(void *opaque)
>  {
> -    return css_migration_enabled();
> +    S390FLICState *fs = S390_FLIC_COMMON(opaque);
> +
> +    return fs->migration_enabled;
>  }
>  
>  const VMStateDescription vmstate_adapter_info_so = {
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index feabc173eb3..1383e47eeb5 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -1174,6 +1174,7 @@ static void ccw_machine_2_9_class_options(MachineClass *mc)
>      S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>      static GlobalProperty compat[] = {
>          { TYPE_S390_STATTRIB, "migration-enabled", "off", },
> +        { TYPE_S390_FLIC_COMMON, "migration-enabled", "off", },
>      };
>  
>      ccw_machine_2_10_class_options(mc);
> -- 
> 2.45.0
>
>

This patch causes QEMU to crash when trying to save the domain state
(e.g. using libvirt)

$ cat dom.xml
<domain type='kvm'>
  <name>bug</name>
  <memory unit='KiB'>1048576</memory>
  <vcpu placement='static'>2</vcpu>
  <os>
    <type arch='s390x' machine='s390-ccw-virtio-9.1'>hvm</type>
    <kernel>/var/lib/libvirt/images/hades/vmlinux-s390x</kernel>
    <initrd>/var/lib/libvirt/images/hades/ramdisk-s390x</initrd>
  </os>
  <devices>
    <emulator>/usr/bin/qemu-system-s390x</emulator>
    <console type='pty'>
      <target type='sclp' port='0'/>
    </console>
  </devices>
</domain>
$ virsh create dom.xml
Domain 'bug' created from dom.xml
$ virsh save bug data
error: Failed to save domain 'bug' to data
error: operation failed: domain is not running

$ coredumpctl gdb
(gdb) bt
#0  object_dynamic_cast_assert (obj=0x2aa364aedd0, typename=typename@entry=0x2aa3491bd56 "s390-flic", file=file@entry=0x2aa34920c7a "/root/git/qemu/include/hw/s390x/s390_flic.h", line=line@entry=42, func=func@entry=0x2aa34a4b964 <__func__.14> "S390_FLIC_COMMON")
    at ../qom/object.c:923
#1  0x000002aa3459b518 in S390_FLIC_COMMON (obj=<optimized out>) at /root/git/qemu/include/hw/s390x/s390_flic.h:42
#2  adapter_info_so_needed (opaque=<optimized out>) at ../hw/intc/s390_flic.c:462
#3  0x000002aa348b24dc in vmstate_section_needed (vmsd=0x2aa34c80fe0 <vmstate_adapter_info_so>, opaque=<optimized out>) at ../migration/vmstate.c:330
#4  vmstate_subsection_save (f=0x2aa36602bd0, vmsd=0x2aa34c80f78 <vmstate_adapter_info>, opaque=0x2aa364aedd0, vmdesc=0x0) at ../migration/vmstate.c:528
#5  vmstate_save_state_v (f=f@entry=0x2aa36602bd0, vmsd=0x2aa34c80f78 <vmstate_adapter_info>, opaque=opaque@entry=0x2aa364aedd0, vmdesc=vmdesc@entry=0x0, version_id=version_id@entry=1, errp=0x0) at ../migration/vmstate.c:443
#6  0x000002aa348b2886 in vmstate_save_state (f=0x2aa36602bd0, vmsd=<optimized out>, opaque=0x2aa364aedd0, vmdesc_id=0x0) at ../migration/vmstate.c:341
#7  vmstate_save_state_v (f=f@entry=0x2aa36602bd0, vmsd=0x2aa34c80e50 <vmstate_adapter_routes>, opaque=opaque@entry=0x2aa364aedd0, vmdesc=vmdesc@entry=0x0, version_id=version_id@entry=1, errp=0x0) at ../migration/vmstate.c:401
#8  0x000002aa348b2886 in vmstate_save_state (f=0x2aa36602bd0, vmsd=<optimized out>, opaque=0x2aa364aedd0, vmdesc_id=0x0) at ../migration/vmstate.c:341
#9  vmstate_save_state_v (f=0x2aa36602bd0, vmsd=0x2aa34c6cdf0 <vmstate_virtio_ccw_dev>, opaque=<optimized out>, vmdesc=0x0, version_id=version_id@entry=1, errp=0x0) at ../migration/vmstate.c:401
#10 0x000002aa348b2d7e in vmstate_save_state (f=<optimized out>, vmsd=<optimized out>, opaque=<optimized out>, vmdesc_id=<optimized out>) at ../migration/vmstate.c:341
#11 0x000002aa345c9726 in virtio_save (vdev=0x2aa364afe20, f=0x2aa36602bd0) at ../hw/virtio/virtio.c:2808
#12 0x000002aa348b23de in vmstate_save_state_v (f=f@entry=0x2aa36602bd0, vmsd=0x2aa34c80cd0 <vmstate_virtio_console>, opaque=<optimized out>, vmdesc=vmdesc@entry=0x2aa36602280, version_id=version_id@entry=3, errp=0x3ff73efb438) at ../migration/vmstate.c:408
#13 0x000002aa348b2dbe in vmstate_save_state_with_err (f=f@entry=0x2aa36602bd0, vmsd=<optimized out>, opaque=<optimized out>, vmdesc_id=vmdesc_id@entry=0x2aa36602280, errp=errp@entry=0x3ff73efb438) at ../migration/vmstate.c:347
#14 0x000002aa344993ce in vmstate_save (f=f@entry=0x2aa36602bd0, se=se@entry=0x2aa365cac80, vmdesc=vmdesc@entry=0x2aa36602280, errp=<optimized out>, errp@entry=0x3ff73efb438) at ../migration/savevm.c:1037
#15 0x000002aa3449cb80 in qemu_savevm_state_complete_precopy_non_iterable (f=f@entry=0x2aa36602bd0, in_postcopy=<optimized out>, in_postcopy@entry=false, inactivate_disks=false, inactivate_disks@entry=true) at ../migration/savevm.c:1554
#16 0x000002aa3449d15a in qemu_savevm_state_complete_precopy (f=0x2aa36602bd0, iterable_only=iterable_only@entry=false, inactivate_disks=false) at ../migration/savevm.c:1630
#17 0x000002aa3448ca00 in migration_completion_precopy (s=0x2aa3625d1f0, current_active_state=0x3ff73efb67c) at ../migration/migration.c:2710
#18 migration_completion (s=0x2aa3625d1f0) at ../migration/migration.c:2774
#19 migration_iteration_run (s=0x2aa3625d1f0) at ../migration/migration.c:3198
#20 migration_thread (opaque=opaque@entry=0x2aa3625d1f0) at ../migration/migration.c:3464
#21 0x000002aa3483bc12 in qemu_thread_start (args=<optimized out>) at ../util/qemu-thread-posix.c:541
#22 0x000003ff91bac3fa in start_thread () at /lib64/libc.so.6
#23 0x000003ff91c2bb18 in thread_start () at /lib64/libc.so.6
Thomas Huth May 17, 2024, 5:47 a.m. UTC | #3
On 16/05/2024 16.42, Marc Hartmayer wrote:
> On Thu, May 09, 2024 at 07:00 PM +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Instead of mucking with css_migration_enabled(), add a property specific to
>> the FLIC device, similar to what is done for TYPE_S390_STATTRIB.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   include/hw/s390x/s390_flic.h | 1 +
>>   hw/intc/s390_flic.c          | 6 +++++-
>>   hw/s390x/s390-virtio-ccw.c   | 1 +
>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
>> index 3907a13d076..bcb081def58 100644
>> --- a/include/hw/s390x/s390_flic.h
>> +++ b/include/hw/s390x/s390_flic.h
>> @@ -47,6 +47,7 @@ struct S390FLICState {
>>       /* to limit AdapterRoutes.num_routes for compat */
>>       uint32_t adapter_routes_max_batch;
>>       bool ais_supported;
>> +    bool migration_enabled;
>>   };
>>   
>>   
>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>> index f4a848460b8..7f930800877 100644
>> --- a/hw/intc/s390_flic.c
>> +++ b/hw/intc/s390_flic.c
>> @@ -405,6 +405,8 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data)
>>   static Property s390_flic_common_properties[] = {
>>       DEFINE_PROP_UINT32("adapter_routes_max_batch", S390FLICState,
>>                          adapter_routes_max_batch, ADAPTER_ROUTES_MAX_GSI),
>> +    DEFINE_PROP_BOOL("migration-enabled", S390FLICState,
>> +                     migration_enabled, true),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> @@ -457,7 +459,9 @@ type_init(qemu_s390_flic_register_types)
>>   
>>   static bool adapter_info_so_needed(void *opaque)
>>   {
>> -    return css_migration_enabled();
>> +    S390FLICState *fs = S390_FLIC_COMMON(opaque);
>> +
>> +    return fs->migration_enabled;
>>   }
...
> This patch causes QEMU to crash when trying to save the domain state
> (e.g. using libvirt)

Oh, drat, that vmstate belongs to a ccw device, not to a flic device, so the 
"opaque" pointer in adapter_info_so_needed points to the wrong structure.

I guess the easiest fix is:

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -459,7 +459,7 @@ type_init(qemu_s390_flic_register_types)

  static bool adapter_info_so_needed(void *opaque)
  {
-    S390FLICState *fs = S390_FLIC_COMMON(opaque);
+    S390FLICState *fs = s390_get_flic();

      return fs->migration_enabled;
  }

I'll send it as a proper patch...

  Thomas
diff mbox series

Patch

diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
index 3907a13d076..bcb081def58 100644
--- a/include/hw/s390x/s390_flic.h
+++ b/include/hw/s390x/s390_flic.h
@@ -47,6 +47,7 @@  struct S390FLICState {
     /* to limit AdapterRoutes.num_routes for compat */
     uint32_t adapter_routes_max_batch;
     bool ais_supported;
+    bool migration_enabled;
 };
 
 
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index f4a848460b8..7f930800877 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -405,6 +405,8 @@  static void qemu_s390_flic_class_init(ObjectClass *oc, void *data)
 static Property s390_flic_common_properties[] = {
     DEFINE_PROP_UINT32("adapter_routes_max_batch", S390FLICState,
                        adapter_routes_max_batch, ADAPTER_ROUTES_MAX_GSI),
+    DEFINE_PROP_BOOL("migration-enabled", S390FLICState,
+                     migration_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -457,7 +459,9 @@  type_init(qemu_s390_flic_register_types)
 
 static bool adapter_info_so_needed(void *opaque)
 {
-    return css_migration_enabled();
+    S390FLICState *fs = S390_FLIC_COMMON(opaque);
+
+    return fs->migration_enabled;
 }
 
 const VMStateDescription vmstate_adapter_info_so = {
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index feabc173eb3..1383e47eeb5 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -1174,6 +1174,7 @@  static void ccw_machine_2_9_class_options(MachineClass *mc)
     S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
     static GlobalProperty compat[] = {
         { TYPE_S390_STATTRIB, "migration-enabled", "off", },
+        { TYPE_S390_FLIC_COMMON, "migration-enabled", "off", },
     };
 
     ccw_machine_2_10_class_options(mc);