| Message ID | 20240509170044.190795-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix --without-default-devices build and (mostly) tests | expand |
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);
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
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 --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);
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(-)