On 4/27/21 4:02 PM, Dr. David Alan Gilbert wrote: > * Laurent Vivier ([email protected]) wrote: >> In the failover case configuration, virtio_net_device_realize() uses an >> add_migration_state_change_notifier() to add a state notifier, but this >> notifier is not removed by the unrealize function when the virtio-net >> card is unplugged. >> >> If the card is unplugged and a migration is started, the notifier is >> called and as it is not valid anymore QEMU crashes. >> >> This patch fixes the problem by adding the >> remove_migration_state_change_notifier() in virtio_net_device_unrealize(). >> >> The problem can be reproduced with: >> >> $ qemu-system-x86_64 -enable-kvm -m 1g -M q35 \ >> -device pcie-root-port,slot=4,id=root1 \ >> -device pcie-root-port,slot=5,id=root2 \ >> -device >> virtio-net-pci,id=net1,mac=52:54:00:6f:55:cc,failover=on,bus=root1 \ >> -monitor stdio disk.qcow2 >> (qemu) device_del net1 >> (qemu) migrate "exec:gzip -c > STATEFILE.gz" >> >> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault. >> 0x0000000000000000 in ?? () >> (gdb) bt >> #0 0x0000000000000000 in () >> #1 0x0000555555d726d7 in notifier_list_notify (...) >> at .../util/notify.c:39 >> #2 0x0000555555842c1a in migrate_fd_connect (...) >> at .../migration/migration.c:3975 >> #3 0x0000555555950f7d in migration_channel_connect (...) >> error@entry=0x0) at .../migration/channel.c:107 >> #4 0x0000555555910922 in exec_start_outgoing_migration (...) >> at .../migration/exec.c:42 >> >> Reported-by: Igor Mammedov <[email protected]> >> Signed-off-by: Laurent Vivier <[email protected]> > > Yep, I think that's OK.
IIUC HMP device_del ends calling device_finalize(), and per Igor's explanation of qdev transition states: https://www.mail-archive.com/[email protected]/msg661853.html I'd expect this to be done in instance_init/instance_finalize, but since 'failover' is a virtio_net_properties, the callbacks has to be registered in the realize() function, thus unregistered in unrealize(). Mentioning it is in realize/unrealize due to 'failover' being a property: Reviewed-by: Philippe Mathieu-Daudé <[email protected]> > Reviewed-by: Dr. David Alan Gilbert <[email protected]> > >> --- >> hw/net/virtio-net.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 66b9ff451185..914051feb75b 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -3373,6 +3373,7 @@ static void virtio_net_device_unrealize(DeviceState >> *dev) >> >> if (n->failover) { >> device_listener_unregister(&n->primary_listener); >> + remove_migration_state_change_notifier(&n->migration_state); >> } >> >> max_queues = n->multiqueue ? n->max_queues : 1; >> -- >> 2.30.2 >>
