* Yury Kotov (yury-ko...@yandex-team.ru) wrote: > Hi! > > 11.12.2019, 14:17, "Dr. David Alan Gilbert" <dgilb...@redhat.com>: > > * Yury Kotov (yury-ko...@yandex-team.ru) wrote: > >> Hi, > >> > >> I found that it's possible to remove a RAMBlock during migration. > >> E.g. device hot-unplugging initiated by a guest (how to reproduce is > >> below). > >> And I want to clarify whether RAMBlock removing (or even adding) during > >> migration is valid operation or it's a bug. > >> > >> Currently, it may cause some race conditions with migration thread and > >> migration may fail because of them. For instance, vmstate_unregister_ram > >> function which is called during PCIe device removing does these: > >> - Memset idstr -> target may receive unknown/zeroed idstr -> migration > >> fail > >> - Set RAMBlock flags as non-migratable -> migration fail > >> > >> RAMBlock removing itself seems safe for migration thread because of RCU. > >> But it seems to me there are other possible race conditions (didn't test > >> it): > >> - qemu_put_buffer_async -> saves pointer to RAMBlock's memory > >> -> block will be freed out of RCU (between ram save iterations) > >> -> qemu_fflush -> access to freed memory. > >> > >> So, I have the following questions: > >> 1. Is RAMBlock removing/adding OK during migration? > > > > I don't think that any hot(un)plug is safe during migration. > > While it's true we hold RCUs as we walk lists, we can't hold the RCU > > around the entire migration. > > I agree. Currently, it's unsafe to do any hot(un)plug. > But I thought (and wanted to clarify) it would be nice to make it safe. > Hold the RCU around the entire migration is not the only way actually. > For example, we can defer RAMBlock deletion: refcount RAMBlocks before > migration and unref them after migration.
Yes, that might work. > > > > There's lots of other problems; for example we call the .save_setup > > methods on devices at the start of migration, but then call the iterate > > on those devices later - if the device is added/removed between stages > > we'll end up either having done a setup and not calling the actual save, > > or the other way around. > > Hm... Yeah, that's a problem, thanks for mentioning it! > > > > > Juan added checks to qdev_device_add/qdev_unplug in b06424d ~2.5 years > > ago. > > I see that hot(un)plug during migration has many issues. > But generally it has three groups (if I didn't miss something): > 1) RAMBlock add/del > 2) Device add/del > 3) VMState add/del > > IIUC, RAMBlocks are not always connected to some devices. > So, in theory, it might become possible to hot(un)plug a block > without hot adding/removing a device. It's why I wanted to clarify > is there a sense to fix separately the problems related to RAMBlocks. Possibly true. > But, if you think there is no sense to fix all related problems > to let hot(un)plugging during migration be allowed, I think we can add > an assert(!migrate_is_idle()) in qemu_ram_free. is_idle is probably the wrong thing, because of the new WAIT_UNPLUG state that happens just after setup and is designed to allow vfio devices to be unplugged before we actually start the guts of the migration; however the idea makes sense. > >> 2. If yes then what should we do with vmstate_unregister_ram? > >> - Just remove vmstate_unregister_ram (my RFC patch) > >> - Refcount RAMBlock's migratable/non-migratable state > >> - Something else? > >> 3. If it mustn't be possible, so may be > >> assert(migration_is_idle()) in qemu_ram_free? > >> > >> P.S. > >> I'm working on a fix of below problem and trying to choose better way: > >> allow device removing and fix all problem like this or fix a particular > >> device. > >> > >> -------- > >> How to reproduce device removing during migration: > >> > >> 1. Source QEMU command line (target is similar) > >> $ x86_64-softmmu/qemu-system-x86_64 \ > >> -nodefaults -no-user-config -m 1024 -M q35 \ > >> -qmp unix:./src.sock,server,nowait \ > >> -drive file=./image,format=raw,if=virtio \ > >> -device ioh3420,id=pcie.1 \ > >> -device virtio-net,bus=pcie.1 > >> 2. Start migration with slow speed (to simplify reproducing) > >> 3. Power off a device on the hotplug pcie.1 bus: > >> $ echo 0 > /sys/bus/pci/slots/0/power > >> 4. Increase migration speed and wait until fail > >> > >> Most likely you will get something like this: > >> qemu-system-x86_64: get_pci_config_device: Bad config data: > >> i=0xaa read: 0 device: 40 cmask: ff wmask: 0 w1cmask:19 > >> qemu-system-x86_64: Failed to load PCIDevice:config > >> qemu-system-x86_64: Failed to load > >> ioh-3240-express-root-port:parent_obj.parent_obj.parent_obj > >> qemu-system-x86_64: error while loading state for instance 0x0 of device > >> '0000:00:03.0/ioh-3240-express-root-port' > >> qemu-system-x86_64: load of migration failed: Invalid argument > >> > >> This error is just an illustration of the removing device possibility, > >> but not actually an illustration of the race conditions for removing > >> RAMBlock. > > > > What path does this actually take - does it end up going via qdev_unplug > > or some other way? > > 1) Guest: writes to slot's pci config > 2) QEMU: pcie_cap_slot_write_config -> pcie_unplug_device > > So, it's only guest driven action and qdev_unplug doesn't help here. Hmm we need to find a way to stop that; lets see if Michael Tsirkin has any ideas (cc'd) - I'm thinking if we could defer the unplug until the end of the migration we'd be OK; but it feels racy as to whether the destination is started with the device that the guest is unplugging. Dave > > > > Dave > > > >> Regards, > >> Yury > >> > >> Yury Kotov (1): > >> migration: Remove vmstate_unregister_ram > >> > >> hw/block/pflash_cfi01.c | 1 - > >> hw/block/pflash_cfi02.c | 1 - > >> hw/mem/pc-dimm.c | 5 ----- > >> hw/misc/ivshmem.c | 2 -- > >> hw/pci/pci.c | 1 - > >> include/migration/vmstate.h | 1 - > >> migration/savevm.c | 6 ------ > >> 7 files changed, 17 deletions(-) > >> > >> -- > >> 2.24.0 > > -- > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > > Regards, > Yury > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
