From 04131e00097c5b85f96af6a66b1c009446c90ec1 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 18 Oct 2023 11:12:39 +0200 Subject: [PATCH 01/17] tests/qtest/migration-test: Disable the analyze-migration.py test on s390x The analyze-migration.py script fails on s390x hosts: Traceback (most recent call last): File "scripts/analyze-migration.py", line 662, in dump.read(dump_memory = args.memory) File "scripts/analyze-migration.py", line 596, in read classdesc = self.section_classes[section_key] KeyError: ('s390-storage_attributes', 0) It obviously never has been adapted to s390x yet, so until this has been done, disable this test on s390x. Signed-off-by: Thomas Huth Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231018091239.164452-1-thuth@redhat.com> --- tests/qtest/migration-test.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index e1c110537b..241b409857 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -3034,7 +3034,9 @@ int main(int argc, char **argv) qtest_add_func("/migration/bad_dest", test_baddest); #ifndef _WIN32 - qtest_add_func("/migration/analyze-script", test_analyze_script); + if (!g_str_equal(arch, "s390x")) { + qtest_add_func("/migration/analyze-script", test_analyze_script); + } #endif qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain); qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle); From c8a7fc5179c649eed1d4286776a23e8a1a183cdc Mon Sep 17 00:00:00 2001 From: Steve Sistare Date: Wed, 18 Oct 2023 06:03:36 -0700 Subject: [PATCH 02/17] migration: simplify blockers Modify migrate_add_blocker and migrate_del_blocker to take an Error ** reason. This allows migration to own the Error object, so that if an error occurs in migrate_add_blocker, migration code can free the Error and clear the client handle, simplifying client code. It also simplifies the migrate_del_blocker call site. In addition, this is a pre-requisite for a proposed future patch that would add a mode argument to migration requests to support live update, and maintain a list of blockers for each mode. A blocker may apply to a single mode or to multiple modes, and passing Error** will allow one Error object to be registered for multiple modes. No functional change. Signed-off-by: Steve Sistare Tested-by: Michael Galaxy Reviewed-by: Michael Galaxy Reviewed-by: Peter Xu Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <1697634216-84215-1-git-send-email-steven.sistare@oracle.com> --- backends/tpm/tpm_emulator.c | 10 ++-------- block/parallels.c | 9 +++------ block/qcow.c | 6 ++---- block/vdi.c | 6 ++---- block/vhdx.c | 6 ++---- block/vmdk.c | 6 ++---- block/vpc.c | 6 ++---- block/vvfat.c | 6 ++---- dump/dump.c | 4 ++-- hw/9pfs/9p.c | 10 ++-------- hw/display/virtio-gpu-base.c | 8 ++------ hw/intc/arm_gic_kvm.c | 3 +-- hw/intc/arm_gicv3_its_kvm.c | 3 +-- hw/intc/arm_gicv3_kvm.c | 3 +-- hw/misc/ivshmem.c | 8 ++------ hw/ppc/pef.c | 2 +- hw/ppc/spapr.c | 9 +-------- hw/ppc/spapr_events.c | 6 +++++- hw/ppc/spapr_rtas.c | 2 +- hw/remote/proxy.c | 7 ++----- hw/s390x/s390-virtio-ccw.c | 9 +++------ hw/scsi/vhost-scsi.c | 8 +++----- hw/vfio/common.c | 10 ++-------- hw/vfio/migration.c | 16 ++-------------- hw/virtio/vhost.c | 8 ++------ include/migration/blocker.h | 24 +++++++++++++++++------- migration/migration.c | 22 ++++++++++++++-------- stubs/migr-blocker.c | 4 ++-- target/i386/kvm/kvm.c | 8 ++++---- target/i386/nvmm/nvmm-all.c | 3 +-- target/i386/sev.c | 2 +- target/i386/whpx/whpx-all.c | 3 +-- ui/vdagent.c | 5 ++--- 33 files changed, 92 insertions(+), 150 deletions(-) diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c index 402a2d6312..bf1a90f5d7 100644 --- a/backends/tpm/tpm_emulator.c +++ b/backends/tpm/tpm_emulator.c @@ -534,11 +534,8 @@ static int tpm_emulator_block_migration(TPMEmulator *tpm_emu) error_setg(&tpm_emu->migration_blocker, "Migration disabled: TPM emulator does not support " "migration"); - if (migrate_add_blocker(tpm_emu->migration_blocker, &err) < 0) { + if (migrate_add_blocker(&tpm_emu->migration_blocker, &err) < 0) { error_report_err(err); - error_free(tpm_emu->migration_blocker); - tpm_emu->migration_blocker = NULL; - return -1; } } @@ -1016,10 +1013,7 @@ static void tpm_emulator_inst_finalize(Object *obj) qapi_free_TPMEmulatorOptions(tpm_emu->options); - if (tpm_emu->migration_blocker) { - migrate_del_blocker(tpm_emu->migration_blocker); - error_free(tpm_emu->migration_blocker); - } + migrate_del_blocker(&tpm_emu->migration_blocker); tpm_sized_buffer_reset(&state_blobs->volatil); tpm_sized_buffer_reset(&state_blobs->permanent); diff --git a/block/parallels.c b/block/parallels.c index 6b46623241..1d695ce7fb 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1369,9 +1369,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, bdrv_get_device_or_node_name(bs)); bdrv_graph_rdunlock_main_loop(); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_setg(errp, "Migration blocker error"); goto fail; } qemu_co_mutex_init(&s->lock); @@ -1406,7 +1405,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); if (ret < 0) { error_setg_errno(errp, -ret, "Could not repair corrupted image"); - migrate_del_blocker(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); goto fail; } } @@ -1423,7 +1422,6 @@ fail: */ parallels_free_used_bitmap(bs); - error_free(s->migration_blocker); g_free(s->bat_dirty_bmap); qemu_vfree(s->header); return ret; @@ -1448,8 +1446,7 @@ static void parallels_close(BlockDriverState *bs) g_free(s->bat_dirty_bmap); qemu_vfree(s->header); - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); } static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs) diff --git a/block/qcow.c b/block/qcow.c index 38a16253b8..fdd4c83948 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -307,9 +307,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, bdrv_get_device_or_node_name(bs)); bdrv_graph_rdunlock_main_loop(); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_free(s->migration_blocker); goto fail; } @@ -802,8 +801,7 @@ static void qcow_close(BlockDriverState *bs) g_free(s->cluster_cache); g_free(s->cluster_data); - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); } static int coroutine_fn GRAPH_UNLOCKED diff --git a/block/vdi.c b/block/vdi.c index 3ed43b6f35..fd7e365383 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -498,9 +498,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, bdrv_get_device_or_node_name(bs)); bdrv_graph_rdunlock_main_loop(); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_free(s->migration_blocker); goto fail_free_bmap; } @@ -988,8 +987,7 @@ static void vdi_close(BlockDriverState *bs) qemu_vfree(s->bmap); - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); } static int vdi_has_zero_init(BlockDriverState *bs) diff --git a/block/vhdx.c b/block/vhdx.c index 73cb214fb4..e37f8c0926 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -985,8 +985,7 @@ static void vhdx_close(BlockDriverState *bs) s->bat = NULL; qemu_vfree(s->parent_entries); s->parent_entries = NULL; - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); qemu_vfree(s->log.hdr); s->log.hdr = NULL; vhdx_region_unregister_all(s); @@ -1097,9 +1096,8 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_free(s->migration_blocker); goto fail; } diff --git a/block/vmdk.c b/block/vmdk.c index 8a3b152798..1335d39e16 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1386,9 +1386,8 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_free(s->migration_blocker); goto fail; } @@ -2867,8 +2866,7 @@ static void vmdk_close(BlockDriverState *bs) vmdk_free_extents(bs); g_free(s->create_type); - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); } static int64_t coroutine_fn GRAPH_RDLOCK diff --git a/block/vpc.c b/block/vpc.c index 945847fe4a..c30cf8689a 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -452,9 +452,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, bdrv_get_device_or_node_name(bs)); bdrv_graph_rdunlock_main_loop(); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_free(s->migration_blocker); goto fail; } @@ -1190,8 +1189,7 @@ static void vpc_close(BlockDriverState *bs) g_free(s->pageentry_u8); #endif - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); } static QemuOptsList vpc_create_opts = { diff --git a/block/vvfat.c b/block/vvfat.c index b0415798c0..266e036dcd 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1268,9 +1268,8 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, "The vvfat (rw) format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - ret = migrate_add_blocker(s->migration_blocker, errp); + ret = migrate_add_blocker(&s->migration_blocker, errp); if (ret < 0) { - error_free(s->migration_blocker); goto fail; } } @@ -3239,8 +3238,7 @@ static void vvfat_close(BlockDriverState *bs) g_free(s->cluster_buffer); if (s->qcow) { - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); + migrate_del_blocker(&s->migration_blocker); } } diff --git a/dump/dump.c b/dump/dump.c index d3578ddc62..d355ada62e 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -111,7 +111,7 @@ static int dump_cleanup(DumpState *s) qemu_mutex_unlock_iothread(); } } - migrate_del_blocker(dump_migration_blocker); + migrate_del_blocker(&dump_migration_blocker); return 0; } @@ -2158,7 +2158,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, * Allows even for -only-migratable, but forbid migration during the * process of dump guest memory. */ - if (migrate_add_blocker_internal(dump_migration_blocker, errp)) { + if (migrate_add_blocker_internal(&dump_migration_blocker, errp)) { /* Remember to release the fd before passing it over to dump state */ close(fd); return; diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 323f042e65..af636cfb2d 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -406,11 +406,7 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, V9fsFidState *fidp) * delete the migration blocker. Ideally, this * should be hooked to transport close notification */ - if (pdu->s->migration_blocker) { - migrate_del_blocker(pdu->s->migration_blocker); - error_free(pdu->s->migration_blocker); - pdu->s->migration_blocker = NULL; - } + migrate_del_blocker(&pdu->s->migration_blocker); } return free_fid(pdu, fidp); } @@ -1505,10 +1501,8 @@ static void coroutine_fn v9fs_attach(void *opaque) error_setg(&s->migration_blocker, "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); - err = migrate_add_blocker(s->migration_blocker, NULL); + err = migrate_add_blocker(&s->migration_blocker, NULL); if (err < 0) { - error_free(s->migration_blocker); - s->migration_blocker = NULL; clunk_fid(s, fid); goto out; } diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c index 50c5373b65..37af256219 100644 --- a/hw/display/virtio-gpu-base.c +++ b/hw/display/virtio-gpu-base.c @@ -184,8 +184,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev, if (virtio_gpu_virgl_enabled(g->conf)) { error_setg(&g->migration_blocker, "virgl is not yet migratable"); - if (migrate_add_blocker(g->migration_blocker, errp) < 0) { - error_free(g->migration_blocker); + if (migrate_add_blocker(&g->migration_blocker, errp) < 0) { return false; } } @@ -253,10 +252,7 @@ virtio_gpu_base_device_unrealize(DeviceState *qdev) { VirtIOGPUBase *g = VIRTIO_GPU_BASE(qdev); - if (g->migration_blocker) { - migrate_del_blocker(g->migration_blocker); - error_free(g->migration_blocker); - } + migrate_del_blocker(&g->migration_blocker); } static void diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c index 1d588946bc..e0d9e512a3 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -516,8 +516,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) if (!kvm_arm_gic_can_save_restore(s)) { error_setg(&s->migration_blocker, "This operating system kernel does " "not support vGICv2 migration"); - if (migrate_add_blocker(s->migration_blocker, errp) < 0) { - error_free(s->migration_blocker); + if (migrate_add_blocker(&s->migration_blocker, errp) < 0) { return; } } diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c index 7eda9fb86e..61c1cc7bdb 100644 --- a/hw/intc/arm_gicv3_its_kvm.c +++ b/hw/intc/arm_gicv3_its_kvm.c @@ -114,8 +114,7 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp) GITS_CTLR)) { error_setg(&s->migration_blocker, "This operating system kernel " "does not support vITS migration"); - if (migrate_add_blocker(s->migration_blocker, errp) < 0) { - error_free(s->migration_blocker); + if (migrate_add_blocker(&s->migration_blocker, errp) < 0) { return; } } else { diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c index 72ad916d3d..77eb37e131 100644 --- a/hw/intc/arm_gicv3_kvm.c +++ b/hw/intc/arm_gicv3_kvm.c @@ -878,8 +878,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) GICD_CTLR)) { error_setg(&s->migration_blocker, "This operating system kernel does " "not support vGICv3 migration"); - if (migrate_add_blocker(s->migration_blocker, errp) < 0) { - error_free(s->migration_blocker); + if (migrate_add_blocker(&s->migration_blocker, errp) < 0) { return; } } diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index d66d912172..0447888029 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -903,8 +903,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) if (!ivshmem_is_master(s)) { error_setg(&s->migration_blocker, "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); - if (migrate_add_blocker(s->migration_blocker, errp) < 0) { - error_free(s->migration_blocker); + if (migrate_add_blocker(&s->migration_blocker, errp) < 0) { return; } } @@ -922,10 +921,7 @@ static void ivshmem_exit(PCIDevice *dev) IVShmemState *s = IVSHMEM_COMMON(dev); int i; - if (s->migration_blocker) { - migrate_del_blocker(s->migration_blocker); - error_free(s->migration_blocker); - } + migrate_del_blocker(&s->migration_blocker); if (memory_region_is_mapped(s->ivshmem_bar2)) { if (!s->hostmem) { diff --git a/hw/ppc/pef.c b/hw/ppc/pef.c index cc44d5e339..d28ed3ba73 100644 --- a/hw/ppc/pef.c +++ b/hw/ppc/pef.c @@ -63,7 +63,7 @@ static int kvmppc_svm_init(ConfidentialGuestSupport *cgs, Error **errp) /* add migration blocker */ error_setg(&pef_mig_blocker, "PEF: Migration is not implemented"); /* NB: This can fail if --only-migratable is used */ - migrate_add_blocker(pef_mig_blocker, &error_fatal); + migrate_add_blocker(&pef_mig_blocker, &error_fatal); cgs->ready = true; diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index cb840676d3..b25093be28 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1761,7 +1761,7 @@ static void spapr_machine_reset(MachineState *machine, ShutdownCause reason) /* Signal all vCPUs waiting on this condition */ qemu_cond_broadcast(&spapr->fwnmi_machine_check_interlock_cond); - migrate_del_blocker(spapr->fwnmi_migration_blocker); + migrate_del_blocker(&spapr->fwnmi_migration_blocker); } static void spapr_create_nvram(SpaprMachineState *spapr) @@ -2937,13 +2937,6 @@ static void spapr_machine_init(MachineState *machine) spapr_create_lmb_dr_connectors(spapr); } - if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI) == SPAPR_CAP_ON) { - /* Create the error string for live migration blocker */ - error_setg(&spapr->fwnmi_migration_blocker, - "A machine check is being handled during migration. The handler" - "may run and log hardware error on the destination"); - } - if (mc->nvdimm_supported) { spapr_create_nvdimm_dr_connectors(spapr); } diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 4508e40814..deb4641505 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -920,7 +920,11 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered) * fails when running with -only-migrate. A proper interface to * delay migration completion for a bit could avoid that. */ - ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, NULL); + error_setg(&spapr->fwnmi_migration_blocker, + "A machine check is being handled during migration. The handler" + "may run and log hardware error on the destination"); + + ret = migrate_add_blocker(&spapr->fwnmi_migration_blocker, NULL); if (ret == -EBUSY) { warn_report("Received a fwnmi while migration was in progress"); } diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 7df21581c2..26c384b261 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -496,7 +496,7 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu, spapr->fwnmi_machine_check_interlock = -1; qemu_cond_signal(&spapr->fwnmi_machine_check_interlock_cond); rtas_st(rets, 0, RTAS_OUT_SUCCESS); - migrate_del_blocker(spapr->fwnmi_migration_blocker); + migrate_del_blocker(&spapr->fwnmi_migration_blocker); } static struct rtas_call { diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c index 2052d721e5..fbc85a8d36 100644 --- a/hw/remote/proxy.c +++ b/hw/remote/proxy.c @@ -107,8 +107,7 @@ static void pci_proxy_dev_realize(PCIDevice *device, Error **errp) error_setg(&dev->migration_blocker, "%s does not support migration", TYPE_PCI_PROXY_DEV); - if (migrate_add_blocker(dev->migration_blocker, errp) < 0) { - error_free(dev->migration_blocker); + if (migrate_add_blocker(&dev->migration_blocker, errp) < 0) { object_unref(dev->ioc); return; } @@ -134,9 +133,7 @@ static void pci_proxy_dev_exit(PCIDevice *pdev) qio_channel_close(dev->ioc, NULL); } - migrate_del_blocker(dev->migration_blocker); - - error_free(dev->migration_blocker); + migrate_del_blocker(&dev->migration_blocker); proxy_memory_listener_deconfigure(&dev->proxy_listener); diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 2d75f2131f..07a5157c0f 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -332,8 +332,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms) s390_pv_vm_disable(); } ms->pv = false; - migrate_del_blocker(pv_mig_blocker); - error_free_or_abort(&pv_mig_blocker); + migrate_del_blocker(&pv_mig_blocker); ram_block_discard_disable(false); } @@ -356,11 +355,10 @@ static int s390_machine_protect(S390CcwMachineState *ms) error_setg(&pv_mig_blocker, "protected VMs are currently not migratable."); - rc = migrate_add_blocker(pv_mig_blocker, &local_err); + rc = migrate_add_blocker(&pv_mig_blocker, &local_err); if (rc) { ram_block_discard_disable(false); error_report_err(local_err); - error_free_or_abort(&pv_mig_blocker); return rc; } @@ -368,8 +366,7 @@ static int s390_machine_protect(S390CcwMachineState *ms) rc = s390_pv_vm_enable(); if (rc) { ram_block_discard_disable(false); - migrate_del_blocker(pv_mig_blocker); - error_free_or_abort(&pv_mig_blocker); + migrate_del_blocker(&pv_mig_blocker); return rc; } diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 443f67daa4..14e23cc5b4 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -208,7 +208,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) "When external environment supports it (Orchestrator migrates " "target SCSI device state or use shared storage over network), " "set 'migratable' property to true to enable migration."); - if (migrate_add_blocker(vsc->migration_blocker, errp) < 0) { + if (migrate_add_blocker(&vsc->migration_blocker, errp) < 0) { goto free_virtio; } } @@ -241,10 +241,9 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) free_vqs: g_free(vqs); if (!vsc->migratable) { - migrate_del_blocker(vsc->migration_blocker); + migrate_del_blocker(&vsc->migration_blocker); } free_virtio: - error_free(vsc->migration_blocker); virtio_scsi_common_unrealize(dev); close_fd: if (vhostfd >= 0) { @@ -260,8 +259,7 @@ static void vhost_scsi_unrealize(DeviceState *dev) struct vhost_virtqueue *vqs = vsc->dev.vqs; if (!vsc->migratable) { - migrate_del_blocker(vsc->migration_blocker); - error_free(vsc->migration_blocker); + migrate_del_blocker(&vsc->migration_blocker); } /* This will stop vhost backend. */ diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 5ff5acf1d8..d806057b40 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -129,11 +129,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp) error_setg(&multiple_devices_migration_blocker, "Multiple VFIO devices migration is supported only if all of " "them support P2P migration"); - ret = migrate_add_blocker(multiple_devices_migration_blocker, errp); - if (ret < 0) { - error_free(multiple_devices_migration_blocker); - multiple_devices_migration_blocker = NULL; - } + ret = migrate_add_blocker(&multiple_devices_migration_blocker, errp); return ret; } @@ -145,9 +141,7 @@ void vfio_unblock_multiple_devices_migration(void) return; } - migrate_del_blocker(multiple_devices_migration_blocker); - error_free(multiple_devices_migration_blocker); - multiple_devices_migration_blocker = NULL; + migrate_del_blocker(&multiple_devices_migration_blocker); } bool vfio_viommu_preset(VFIODevice *vbasedev) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index da43dcd2fe..0aaad65c06 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -891,8 +891,6 @@ static void vfio_migration_deinit(VFIODevice *vbasedev) static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) { - int ret; - if (vbasedev->enable_migration == ON_OFF_AUTO_ON) { error_propagate(errp, err); return -EINVAL; @@ -901,13 +899,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) vbasedev->migration_blocker = error_copy(err); error_free(err); - ret = migrate_add_blocker(vbasedev->migration_blocker, errp); - if (ret < 0) { - error_free(vbasedev->migration_blocker); - vbasedev->migration_blocker = NULL; - } - - return ret; + return migrate_add_blocker(&vbasedev->migration_blocker, errp); } /* ---------------------------------------------------------------------- */ @@ -994,9 +986,5 @@ void vfio_migration_exit(VFIODevice *vbasedev) vfio_migration_deinit(vbasedev); } - if (vbasedev->migration_blocker) { - migrate_del_blocker(vbasedev->migration_blocker); - error_free(vbasedev->migration_blocker); - vbasedev->migration_blocker = NULL; - } + migrate_del_blocker(&vbasedev->migration_blocker); } diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 9f37206ba0..d737671028 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1527,9 +1527,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, } if (hdev->migration_blocker != NULL) { - r = migrate_add_blocker(hdev->migration_blocker, errp); + r = migrate_add_blocker(&hdev->migration_blocker, errp); if (r < 0) { - error_free(hdev->migration_blocker); goto fail_busyloop; } } @@ -1597,10 +1596,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev) memory_listener_unregister(&hdev->memory_listener); QLIST_REMOVE(hdev, entry); } - if (hdev->migration_blocker) { - migrate_del_blocker(hdev->migration_blocker); - error_free(hdev->migration_blocker); - } + migrate_del_blocker(&hdev->migration_blocker); g_free(hdev->mem); g_free(hdev->mem_sections); if (hdev->vhost_ops) { diff --git a/include/migration/blocker.h b/include/migration/blocker.h index 9cebe2ba06..b048f301b4 100644 --- a/include/migration/blocker.h +++ b/include/migration/blocker.h @@ -17,19 +17,23 @@ /** * @migrate_add_blocker - prevent migration from proceeding * - * @reason - an error to be returned whenever migration is attempted + * @reasonp - address of an error to be returned whenever migration is attempted * * @errp - [out] The reason (if any) we cannot block migration right now. * * @returns - 0 on success, -EBUSY/-EACCES on failure, with errp set. + * + * *@reasonp is freed and set to NULL if failure is returned. + * On success, the caller must not free @reasonp, except by + * calling migrate_del_blocker. */ -int migrate_add_blocker(Error *reason, Error **errp); +int migrate_add_blocker(Error **reasonp, Error **errp); /** * @migrate_add_blocker_internal - prevent migration from proceeding without * only-migrate implications * - * @reason - an error to be returned whenever migration is attempted + * @reasonp - address of an error to be returned whenever migration is attempted * * @errp - [out] The reason (if any) we cannot block migration right now. * @@ -38,14 +42,20 @@ int migrate_add_blocker(Error *reason, Error **errp); * Some of the migration blockers can be temporary (e.g., for a few seconds), * so it shouldn't need to conflict with "-only-migratable". For those cases, * we can call this function rather than @migrate_add_blocker(). + * + * *@reasonp is freed and set to NULL if failure is returned. + * On success, the caller must not free @reasonp, except by + * calling migrate_del_blocker. */ -int migrate_add_blocker_internal(Error *reason, Error **errp); +int migrate_add_blocker_internal(Error **reasonp, Error **errp); /** - * @migrate_del_blocker - remove a blocking error from migration + * @migrate_del_blocker - remove a blocking error from migration and free it. * - * @reason - the error blocking migration + * @reasonp - address of the error blocking migration + * + * This function frees *@reasonp and sets it to NULL. */ -void migrate_del_blocker(Error *reason); +void migrate_del_blocker(Error **reasonp); #endif diff --git a/migration/migration.c b/migration/migration.c index 05c0b801ba..818b02b707 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1465,35 +1465,41 @@ int migrate_init(MigrationState *s, Error **errp) return 0; } -int migrate_add_blocker_internal(Error *reason, Error **errp) +int migrate_add_blocker_internal(Error **reasonp, Error **errp) { /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */ if (runstate_check(RUN_STATE_SAVE_VM) || !migration_is_idle()) { - error_propagate_prepend(errp, error_copy(reason), + error_propagate_prepend(errp, *reasonp, "disallowing migration blocker " "(migration/snapshot in progress) for: "); + *reasonp = NULL; return -EBUSY; } - migration_blockers = g_slist_prepend(migration_blockers, reason); + migration_blockers = g_slist_prepend(migration_blockers, *reasonp); return 0; } -int migrate_add_blocker(Error *reason, Error **errp) +int migrate_add_blocker(Error **reasonp, Error **errp) { if (only_migratable) { - error_propagate_prepend(errp, error_copy(reason), + error_propagate_prepend(errp, *reasonp, "disallowing migration blocker " "(--only-migratable) for: "); + *reasonp = NULL; return -EACCES; } - return migrate_add_blocker_internal(reason, errp); + return migrate_add_blocker_internal(reasonp, errp); } -void migrate_del_blocker(Error *reason) +void migrate_del_blocker(Error **reasonp) { - migration_blockers = g_slist_remove(migration_blockers, reason); + if (*reasonp) { + migration_blockers = g_slist_remove(migration_blockers, *reasonp); + error_free(*reasonp); + *reasonp = NULL; + } } void qmp_migrate_incoming(const char *uri, Error **errp) diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c index 5676a2f93c..17a5dbf87b 100644 --- a/stubs/migr-blocker.c +++ b/stubs/migr-blocker.c @@ -1,11 +1,11 @@ #include "qemu/osdep.h" #include "migration/blocker.h" -int migrate_add_blocker(Error *reason, Error **errp) +int migrate_add_blocker(Error **reasonp, Error **errp) { return 0; } -void migrate_del_blocker(Error *reason) +void migrate_del_blocker(Error **reasonp) { } diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index ab72bcdfad..e7c054cc16 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1603,7 +1603,7 @@ static int hyperv_init_vcpu(X86CPU *cpu) error_setg(&hv_passthrough_mig_blocker, "'hv-passthrough' CPU flag prevents migration, use explicit" " set of hv-* flags instead"); - ret = migrate_add_blocker(hv_passthrough_mig_blocker, &local_err); + ret = migrate_add_blocker(&hv_passthrough_mig_blocker, &local_err); if (ret < 0) { error_report_err(local_err); return ret; @@ -1617,7 +1617,7 @@ static int hyperv_init_vcpu(X86CPU *cpu) " use explicit 'hv-no-nonarch-coresharing=on' instead (but" " make sure SMT is disabled and/or that vCPUs are properly" " pinned)"); - ret = migrate_add_blocker(hv_no_nonarch_cs_mig_blocker, &local_err); + ret = migrate_add_blocker(&hv_no_nonarch_cs_mig_blocker, &local_err); if (ret < 0) { error_report_err(local_err); return ret; @@ -2213,7 +2213,7 @@ int kvm_arch_init_vcpu(CPUState *cs) error_setg(&invtsc_mig_blocker, "State blocked by non-migratable CPU device" " (invtsc flag)"); - r = migrate_add_blocker(invtsc_mig_blocker, &local_err); + r = migrate_add_blocker(&invtsc_mig_blocker, &local_err); if (r < 0) { error_report_err(local_err); return r; @@ -2271,7 +2271,7 @@ int kvm_arch_init_vcpu(CPUState *cs) return 0; fail: - migrate_del_blocker(invtsc_mig_blocker); + migrate_del_blocker(&invtsc_mig_blocker); return r; } diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c index fb769868f2..7d752bc5e0 100644 --- a/target/i386/nvmm/nvmm-all.c +++ b/target/i386/nvmm/nvmm-all.c @@ -929,9 +929,8 @@ nvmm_init_vcpu(CPUState *cpu) error_setg(&nvmm_migration_blocker, "NVMM: Migration not supported"); - if (migrate_add_blocker(nvmm_migration_blocker, &local_error) < 0) { + if (migrate_add_blocker(&nvmm_migration_blocker, &local_error) < 0) { error_report_err(local_error); - error_free(nvmm_migration_blocker); return -EINVAL; } } diff --git a/target/i386/sev.c b/target/i386/sev.c index fe2144c038..9a71246682 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -891,7 +891,7 @@ sev_launch_finish(SevGuestState *sev) /* add migration blocker */ error_setg(&sev_mig_blocker, "SEV: Migration is not implemented"); - migrate_add_blocker(sev_mig_blocker, &error_fatal); + migrate_add_blocker(&sev_mig_blocker, &error_fatal); } static void diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c index df3aba2642..d29ba916a0 100644 --- a/target/i386/whpx/whpx-all.c +++ b/target/i386/whpx/whpx-all.c @@ -2160,9 +2160,8 @@ int whpx_init_vcpu(CPUState *cpu) "State blocked due to non-migratable CPUID feature support," "dirty memory tracking support, and XSAVE/XRSTOR support"); - if (migrate_add_blocker(whpx_migration_blocker, &local_error) < 0) { + if (migrate_add_blocker(&whpx_migration_blocker, &local_error) < 0) { error_report_err(local_error); - error_free(whpx_migration_blocker); ret = -EINVAL; goto error; } diff --git a/ui/vdagent.c b/ui/vdagent.c index 00d36a8677..d8f2f95432 100644 --- a/ui/vdagent.c +++ b/ui/vdagent.c @@ -671,7 +671,7 @@ static void vdagent_chr_open(Chardev *chr, return; #endif - if (migrate_add_blocker(vd->migration_blocker, errp) != 0) { + if (migrate_add_blocker(&vd->migration_blocker, errp) != 0) { return; } @@ -924,13 +924,12 @@ static void vdagent_chr_fini(Object *obj) { VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj); - migrate_del_blocker(vd->migration_blocker); + migrate_del_blocker(&vd->migration_blocker); vdagent_disconnect(vd); if (vd->mouse_hs) { qemu_input_handler_unregister(vd->mouse_hs); } buffer_free(&vd->outbuf); - error_free(vd->migration_blocker); } static const TypeInfo vdagent_chr_type_info = { From 2c36076a1153e321e32a28b735f5c0fe70d8d10f Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 17 Oct 2023 16:38:55 -0400 Subject: [PATCH 03/17] migration: Fix parse_ramblock() on overwritten retvals It's possible that some errors can be overwritten with success retval later on, and then ignored. Always capture all errors and report. Reported by Coverity 1522861, but actually I spot one more in the same function. Fixes: CID 1522861 Signed-off-by: Peter Xu Reviewed-by: Fabiano Rosas Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231017203855.298260-1-peterx@redhat.com> --- migration/ram.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/migration/ram.c b/migration/ram.c index 16c30a9d7a..92769902bb 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3873,6 +3873,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) ret = qemu_ram_resize(block, length, &local_err); if (local_err) { error_report_err(local_err); + return ret; } } /* For postcopy we need to check hugepage sizes match */ @@ -3883,7 +3884,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) error_report("Mismatched RAM page size %s " "(local) %zd != %" PRId64, block->idstr, block->page_size, remote_page_size); - ret = -EINVAL; + return -EINVAL; } } if (migrate_ignore_shared()) { @@ -3893,7 +3894,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) error_report("Mismatched GPAs for block %s " "%" PRId64 "!= %" PRId64, block->idstr, (uint64_t)addr, (uint64_t)block->mr->addr); - ret = -EINVAL; + return -EINVAL; } } ret = rdma_block_notification_handle(f, block->idstr); From d9cda21303a2b92cf3be48b75d4201896aa06857 Mon Sep 17 00:00:00 2001 From: Steve Sistare Date: Wed, 7 Jun 2023 07:42:34 -0700 Subject: [PATCH 04/17] migration: simplify notifiers Pass the callback function to add_migration_state_change_notifier so that migration can initialize the notifier on add and clear it on delete, which simplifies the call sites. Shorten the function names so the extra arg can be added more legibly. Hide the global notifier list in a new function migration_call_notifiers, and make it externally visible so future live update code can call it. No functional change. Signed-off-by: Steve Sistare Reviewed-by: Peter Xu Tested-by: Michael Galaxy Reviewed-by: Michael Galaxy Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <1686148954-250144-1-git-send-email-steven.sistare@oracle.com> --- hw/net/virtio-net.c | 6 +++--- hw/vfio/migration.c | 6 +++--- include/migration/misc.h | 6 ++++-- migration/migration.c | 22 ++++++++++++++++------ net/vhost-vdpa.c | 7 ++++--- ui/spice-core.c | 3 +-- 6 files changed, 31 insertions(+), 19 deletions(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 29e33ea5ed..b85c7946a7 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -3624,8 +3624,8 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp) n->primary_listener.hide_device = failover_hide_primary_device; qatomic_set(&n->failover_primary_hidden, true); device_listener_register(&n->primary_listener); - n->migration_state.notify = virtio_net_migration_state_notifier; - add_migration_state_change_notifier(&n->migration_state); + migration_add_notifier(&n->migration_state, + virtio_net_migration_state_notifier); n->host_features |= (1ULL << VIRTIO_NET_F_STANDBY); } @@ -3788,7 +3788,7 @@ static void virtio_net_device_unrealize(DeviceState *dev) if (n->failover) { qobject_unref(n->primary_opts); device_listener_unregister(&n->primary_listener); - remove_migration_state_change_notifier(&n->migration_state); + migration_remove_notifier(&n->migration_state); } else { assert(n->primary_opts == NULL); } diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 0aaad65c06..28d422b39f 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -872,8 +872,8 @@ static int vfio_migration_init(VFIODevice *vbasedev) NULL; migration->vm_state = qdev_add_vm_change_state_handler_full( vbasedev->dev, vfio_vmstate_change, prepare_cb, vbasedev); - migration->migration_state.notify = vfio_migration_state_notifier; - add_migration_state_change_notifier(&migration->migration_state); + migration_add_notifier(&migration->migration_state, + vfio_migration_state_notifier); return 0; } @@ -882,7 +882,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev) { VFIOMigration *migration = vbasedev->migration; - remove_migration_state_change_notifier(&migration->migration_state); + migration_remove_notifier(&migration->migration_state); qemu_del_vm_change_state_handler(migration->vm_state); unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); vfio_migration_free(vbasedev); diff --git a/include/migration/misc.h b/include/migration/misc.h index 7dcc0b5c2c..673ac490fb 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -60,8 +60,10 @@ void migration_object_init(void); void migration_shutdown(void); bool migration_is_idle(void); bool migration_is_active(MigrationState *); -void add_migration_state_change_notifier(Notifier *notify); -void remove_migration_state_change_notifier(Notifier *notify); +void migration_add_notifier(Notifier *notify, + void (*func)(Notifier *notifier, void *data)); +void migration_remove_notifier(Notifier *notify); +void migration_call_notifiers(MigrationState *s); bool migration_in_setup(MigrationState *); bool migration_has_finished(MigrationState *); bool migration_has_failed(MigrationState *); diff --git a/migration/migration.c b/migration/migration.c index 818b02b707..67547eb6a1 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1207,7 +1207,7 @@ static void migrate_fd_cleanup(MigrationState *s) /* It is used on info migrate. We can't free it */ error_report_err(error_copy(s->error)); } - notifier_list_notify(&migration_state_notifiers, s); + migration_call_notifiers(s); block_cleanup_parameters(); yank_unregister_instance(MIGRATION_YANK_INSTANCE); } @@ -1311,14 +1311,24 @@ static void migrate_fd_cancel(MigrationState *s) } } -void add_migration_state_change_notifier(Notifier *notify) +void migration_add_notifier(Notifier *notify, + void (*func)(Notifier *notifier, void *data)) { + notify->notify = func; notifier_list_add(&migration_state_notifiers, notify); } -void remove_migration_state_change_notifier(Notifier *notify) +void migration_remove_notifier(Notifier *notify) { - notifier_remove(notify); + if (notify->notify) { + notifier_remove(notify); + notify->notify = NULL; + } +} + +void migration_call_notifiers(MigrationState *s) +{ + notifier_list_notify(&migration_state_notifiers, s); } bool migration_in_setup(MigrationState *s) @@ -2233,7 +2243,7 @@ static int postcopy_start(MigrationState *ms, Error **errp) * spice needs to trigger a transition now */ ms->postcopy_after_devices = true; - notifier_list_notify(&migration_state_notifiers, ms); + migration_call_notifiers(ms); ms->downtime = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - time_at_stop; @@ -3313,7 +3323,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) rate_limit = migrate_max_bandwidth(); /* Notify before starting migration thread */ - notifier_list_notify(&migration_state_notifiers, s); + migration_call_notifiers(s); } migration_rate_set(rate_limit); diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 939c984d5b..0f2e6fc58e 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -339,7 +339,8 @@ static void vhost_vdpa_net_data_start_first(VhostVDPAState *s) { struct vhost_vdpa *v = &s->vhost_vdpa; - add_migration_state_change_notifier(&s->migration_state); + migration_add_notifier(&s->migration_state, + vdpa_net_migration_state_notifier); if (v->shadow_vqs_enabled) { v->iova_tree = vhost_iova_tree_new(v->iova_range.first, v->iova_range.last); @@ -399,7 +400,7 @@ static void vhost_vdpa_net_client_stop(NetClientState *nc) assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA); if (s->vhost_vdpa.index == 0) { - remove_migration_state_change_notifier(&s->migration_state); + migration_remove_notifier(&s->migration_state); } dev = s->vhost_vdpa.dev; @@ -1456,7 +1457,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer, s->vhost_vdpa.device_fd = vdpa_device_fd; s->vhost_vdpa.index = queue_pair_index; s->always_svq = svq; - s->migration_state.notify = vdpa_net_migration_state_notifier; + s->migration_state.notify = NULL; s->vhost_vdpa.shadow_vqs_enabled = svq; s->vhost_vdpa.iova_range = iova_range; s->vhost_vdpa.shadow_data = svq; diff --git a/ui/spice-core.c b/ui/spice-core.c index 52a59386d7..db21db2c94 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -821,8 +821,7 @@ static void qemu_spice_init(void) }; using_spice = 1; - migration_state.notify = migration_state_notifier; - add_migration_state_change_notifier(&migration_state); + migration_add_notifier(&migration_state, migration_state_notifier); spice_migrate.base.sif = &migrate_interface.base; qemu_spice.add_interface(&spice_migrate.base); From 175e63c9829f9887c04bed8e0c4906e1166c9a87 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Thu, 12 Oct 2023 11:06:47 -0300 Subject: [PATCH 05/17] migration/multifd: Stop checking p->quit in multifd_send_thread We don't need to check p->quit in the multifd_send_thread() because it is shadowed by the 'exiting' flag. Ever since that flag was added p->quit became obsolete as a way to stop the thread. Since p->quit is set at multifd_send_terminate_threads() under the p->mutex lock, the thread will only see it once it loops, so 'exiting' will always be seen first. Note that setting p->quit at multifd_send_terminate_threads() still makes sense because we need a way to inform multifd_send_pages() that the channel has stopped. Signed-off-by: Fabiano Rosas Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231012140651.13122-3-farosas@suse.de> --- migration/multifd.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/migration/multifd.c b/migration/multifd.c index 1fe53d3b98..e2a45c667a 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -743,9 +743,6 @@ static void *multifd_send_thread(void *opaque) if (flags & MULTIFD_FLAG_SYNC) { qemu_sem_post(&p->sem_sync); } - } else if (p->quit) { - qemu_mutex_unlock(&p->mutex); - break; } else { qemu_mutex_unlock(&p->mutex); /* sometimes there are spurious wakeups */ From f4a7b30fcd5b60ca9b0215f9a6fa1faf9bd73f7b Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:30 -0300 Subject: [PATCH 06/17] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable We're adding support for testing migration using two different QEMU binaries. We'll provide the second binary in a new environment variable. Allow qtest_qemu_binary() to receive the name of the new variable. If the new environment variable is not set, that's not an error, we use QTEST_QEMU_BINARY as a fallback. Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Fabiano Rosas Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-2-farosas@suse.de> --- tests/qtest/libqtest.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index dc7a55634c..03fa644663 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -336,10 +336,17 @@ void qtest_remove_abrt_handler(void *data) } } -static const char *qtest_qemu_binary(void) +static const char *qtest_qemu_binary(const char *var) { const char *qemu_bin; + if (var) { + qemu_bin = getenv(var); + if (qemu_bin) { + return qemu_bin; + } + } + qemu_bin = getenv("QTEST_QEMU_BINARY"); if (!qemu_bin) { fprintf(stderr, "Environment variable QTEST_QEMU_BINARY required\n"); @@ -392,7 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...) va_start(ap, fmt); g_string_append_printf(command, CMD_EXEC "%s %s", - qtest_qemu_binary(), tracearg); + qtest_qemu_binary(NULL), tracearg); g_string_append_vprintf(command, fmt, ap); va_end(ap); @@ -905,7 +912,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...) const char *qtest_get_arch(void) { - const char *qemu = qtest_qemu_binary(); + const char *qemu = qtest_qemu_binary(NULL); const char *end = strrchr(qemu, '-'); if (!end) { From 9931215bd33b640dcc824e07431e67e6ddb6b1e1 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:31 -0300 Subject: [PATCH 07/17] tests/qtest: Introduce qtest_init_with_env Add a version of qtest_init() that takes an environment variable containing the path of the QEMU binary. This allows tests to use more than one QEMU binary. If no variable is provided or the environment variable does not exist, that is not an error. Fallback to using QTEST_QEMU_BINARY. Signed-off-by: Fabiano Rosas Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-3-farosas@suse.de> --- tests/qtest/libqtest.c | 26 +++++++++++++++++++------- tests/qtest/libqtest.h | 13 +++++++++++++ 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 03fa644663..9eebba8767 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -388,7 +388,8 @@ static pid_t qtest_create_process(char *cmd) } #endif /* _WIN32 */ -static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...) +static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin, + const char *fmt, ...) { va_list ap; QTestState *s = g_new0(QTestState, 1); @@ -398,8 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...) g_autoptr(GString) command = g_string_new(""); va_start(ap, fmt); - g_string_append_printf(command, CMD_EXEC "%s %s", - qtest_qemu_binary(NULL), tracearg); + g_string_append_printf(command, CMD_EXEC "%s %s", qemu_bin, tracearg); g_string_append_vprintf(command, fmt, ap); va_end(ap); @@ -438,7 +438,8 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...) return s; } -QTestState *qtest_init_without_qmp_handshake(const char *extra_args) +static QTestState *qtest_init_internal(const char *qemu_bin, + const char *extra_args) { QTestState *s; int sock, qmpsock, i; @@ -463,7 +464,8 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) sock = init_socket(socket_path); qmpsock = init_socket(qmp_socket_path); - s = qtest_spawn_qemu("-qtest unix:%s " + s = qtest_spawn_qemu(qemu_bin, + "-qtest unix:%s " "-qtest-log %s " "-chardev socket,path=%s,id=char0 " "-mon chardev=char0,mode=control " @@ -516,9 +518,14 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) return s; } -QTestState *qtest_init(const char *extra_args) +QTestState *qtest_init_without_qmp_handshake(const char *extra_args) { - QTestState *s = qtest_init_without_qmp_handshake(extra_args); + return qtest_init_internal(qtest_qemu_binary(NULL), extra_args); +} + +QTestState *qtest_init_with_env(const char *var, const char *extra_args) +{ + QTestState *s = qtest_init_internal(qtest_qemu_binary(var), extra_args); QDict *greeting; /* Read the QMP greeting and then do the handshake */ @@ -529,6 +536,11 @@ QTestState *qtest_init(const char *extra_args) return s; } +QTestState *qtest_init(const char *extra_args) +{ + return qtest_init_with_env(NULL, extra_args); +} + QTestState *qtest_vinitf(const char *fmt, va_list ap) { char *args = g_strdup_vprintf(fmt, ap); diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h index 5fe3d13466..76fc195f1c 100644 --- a/tests/qtest/libqtest.h +++ b/tests/qtest/libqtest.h @@ -55,6 +55,19 @@ QTestState *qtest_vinitf(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0); */ QTestState *qtest_init(const char *extra_args); +/** + * qtest_init_with_env: + * @var: Environment variable from where to take the QEMU binary + * @extra_args: Other arguments to pass to QEMU. CAUTION: these + * arguments are subject to word splitting and shell evaluation. + * + * Like qtest_init(), but use a different environment variable for the + * QEMU binary. + * + * Returns: #QTestState instance. + */ +QTestState *qtest_init_with_env(const char *var, const char *extra_args); + /** * qtest_init_without_qmp_handshake: * @extra_args: other arguments to pass to QEMU. CAUTION: these From 41b2eba4e5c480b8515db60f23b0cb5eeae0dc50 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:32 -0300 Subject: [PATCH 08/17] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary We're adding support for using more than one QEMU binary in tests. Modify qtest_get_machines() to take an environment variable that contains the QEMU binary path. Since the function keeps a cache of the machines list in the form of a static variable, refresh it any time the environment variable changes. Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Fabiano Rosas Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-4-farosas@suse.de> --- tests/qtest/libqtest.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 9eebba8767..3cc7bf3076 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -1468,13 +1468,26 @@ struct MachInfo { char *alias; }; +static void qtest_free_machine_list(struct MachInfo *machines) +{ + if (machines) { + for (int i = 0; machines[i].name != NULL; i++) { + g_free(machines[i].name); + g_free(machines[i].alias); + } + + g_free(machines); + } +} + /* * Returns an array with pointers to the available machine names. * The terminating entry has the name set to NULL. */ -static struct MachInfo *qtest_get_machines(void) +static struct MachInfo *qtest_get_machines(const char *var) { static struct MachInfo *machines; + static char *qemu_var; QDict *response, *minfo; QList *list; const QListEntry *p; @@ -1483,11 +1496,19 @@ static struct MachInfo *qtest_get_machines(void) QTestState *qts; int idx; + if (g_strcmp0(qemu_var, var)) { + qemu_var = g_strdup(var); + + /* new qemu, clear the cache */ + qtest_free_machine_list(machines); + machines = NULL; + } + if (machines) { return machines; } - qts = qtest_init("-machine none"); + qts = qtest_init_with_env(qemu_var, "-machine none"); response = qtest_qmp(qts, "{ 'execute': 'query-machines' }"); g_assert(response); list = qdict_get_qlist(response, "return"); @@ -1528,7 +1549,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine), struct MachInfo *machines; int i; - machines = qtest_get_machines(); + machines = qtest_get_machines(NULL); for (i = 0; machines[i].name != NULL; i++) { /* Ignore machines that cannot be used for qtests */ @@ -1549,7 +1570,7 @@ bool qtest_has_machine(const char *machine) struct MachInfo *machines; int i; - machines = qtest_get_machines(); + machines = qtest_get_machines(NULL); for (i = 0; machines[i].name != NULL; i++) { if (g_str_equal(machine, machines[i].name) || From 1027fc0ae4fcd24c11a43d1f217a1d11579a574b Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:33 -0300 Subject: [PATCH 09/17] tests/qtest: Introduce qtest_has_machine_with_env Add a variant of qtest_has_machine() that receives an environment variable containing an alternate QEMU binary path. Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Fabiano Rosas Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-5-farosas@suse.de> --- tests/qtest/libqtest.c | 9 +++++++-- tests/qtest/libqtest.h | 9 +++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 3cc7bf3076..603d900e7d 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -1565,12 +1565,12 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine), } } -bool qtest_has_machine(const char *machine) +bool qtest_has_machine_with_env(const char *var, const char *machine) { struct MachInfo *machines; int i; - machines = qtest_get_machines(NULL); + machines = qtest_get_machines(var); for (i = 0; machines[i].name != NULL; i++) { if (g_str_equal(machine, machines[i].name) || @@ -1582,6 +1582,11 @@ bool qtest_has_machine(const char *machine) return false; } +bool qtest_has_machine(const char *machine) +{ + return qtest_has_machine_with_env(NULL, machine); +} + bool qtest_has_device(const char *device) { static QList *list; diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h index 76fc195f1c..d16deb9891 100644 --- a/tests/qtest/libqtest.h +++ b/tests/qtest/libqtest.h @@ -930,6 +930,15 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine), */ bool qtest_has_machine(const char *machine); +/** + * qtest_has_machine_with_env: + * @var: Environment variable from where to take the QEMU binary + * @machine: The machine to look for + * + * Returns: true if the machine is available in the specified binary. + */ +bool qtest_has_machine_with_env(const char *var, const char *machine); + /** * qtest_has_device: * @device: The device to look for From a3c0ebc9b09cb15cac35690412eb66aaa5eb4f23 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:34 -0300 Subject: [PATCH 10/17] tests/qtest: Introduce qtest_resolve_machine_alias The migration tests are being enhanced to test migration between different QEMU versions. A requirement of migration is that the machine type between source and destination matches, including the version. We cannot hardcode machine types in the tests because those change with each release. QEMU provides a machine type alias that has a fixed name, but points to the latest machine type at each release. Add a helper to resolve the alias into the exact machine type. E.g. "-machine pc" resolves to "pc-i440fx-8.2" Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Fabiano Rosas Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-6-farosas@suse.de> --- tests/qtest/libqtest.c | 16 ++++++++++++++++ tests/qtest/libqtest.h | 10 ++++++++++ 2 files changed, 26 insertions(+) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 603d900e7d..c843c41188 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -1565,6 +1565,22 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine), } } +char *qtest_resolve_machine_alias(const char *var, const char *alias) +{ + struct MachInfo *machines; + int i; + + machines = qtest_get_machines(var); + + for (i = 0; machines[i].name != NULL; i++) { + if (machines[i].alias && g_str_equal(alias, machines[i].alias)) { + return g_strdup(machines[i].name); + } + } + + return NULL; +} + bool qtest_has_machine_with_env(const char *var, const char *machine) { struct MachInfo *machines; diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h index d16deb9891..6e3d3525bf 100644 --- a/tests/qtest/libqtest.h +++ b/tests/qtest/libqtest.h @@ -922,6 +922,16 @@ void qtest_qmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds, void qtest_cb_for_every_machine(void (*cb)(const char *machine), bool skip_old_versioned); +/** + * qtest_resolve_machine_alias: + * @var: Environment variable from where to take the QEMU binary + * @alias: The alias to resolve + * + * Returns: the machine type corresponding to the alias if any, + * otherwise NULL. + */ +char *qtest_resolve_machine_alias(const char *var, const char *alias); + /** * qtest_has_machine: * @machine: The machine to look for From dcf389cbc84c2b714d49887775918c5f03f73864 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:35 -0300 Subject: [PATCH 11/17] tests/qtest/migration: Introduce find_common_machine_version When using two different QEMU binaries for migration testing, we'll need to find what is the machine version that will work with both binaries. Add a helper for that. Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Fabiano Rosas Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-7-farosas@suse.de> --- tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++ tests/qtest/migration-helpers.h | 2 ++ 2 files changed, 28 insertions(+) diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index 0c185db450..13449c1fe1 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -240,3 +240,29 @@ void wait_for_migration_fail(QTestState *from, bool allow_active) g_assert(qdict_get_bool(rsp_return, "running")); qobject_unref(rsp_return); } + +char *find_common_machine_version(const char *mtype, const char *var1, + const char *var2) +{ + g_autofree char *type1 = qtest_resolve_machine_alias(var1, mtype); + g_autofree char *type2 = qtest_resolve_machine_alias(var2, mtype); + + g_assert(type1 && type2); + + if (g_str_equal(type1, type2)) { + /* either can be used */ + return g_strdup(type1); + } + + if (qtest_has_machine_with_env(var2, type1)) { + return g_strdup(type1); + } + + if (qtest_has_machine_with_env(var1, type2)) { + return g_strdup(type2); + } + + g_test_message("No common machine version for machine type '%s' between " + "binaries %s and %s", mtype, getenv(var1), getenv(var2)); + g_assert_not_reached(); +} diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h index 4f51d0f8bc..d1c2351d33 100644 --- a/tests/qtest/migration-helpers.h +++ b/tests/qtest/migration-helpers.h @@ -43,4 +43,6 @@ void wait_for_migration_complete(QTestState *who); void wait_for_migration_fail(QTestState *from, bool allow_active); +char *find_common_machine_version(const char *mtype, const char *var1, + const char *var2); #endif /* MIGRATION_HELPERS_H */ From c99613910106fd9a79005a3e8f47b1ce7d46dbc4 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:36 -0300 Subject: [PATCH 12/17] tests/qtest/migration: Define a machine for all architectures Stop relying on defaults and select a machine explicitly for every architecture. This is a prerequisite for being able to select machine types for migration using different QEMU binaries for source and destination. Signed-off-by: Fabiano Rosas Reviewed-by: Thomas Huth Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-8-farosas@suse.de> --- tests/qtest/migration-test.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 241b409857..dfea75b76f 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -743,6 +743,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, const char *kvm_opts = NULL; const char *arch = qtest_get_arch(); const char *memory_size; + const char *machine_alias, *machine_opts = ""; if (args->use_shmem) { if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) { @@ -755,11 +756,13 @@ static int test_migrate_start(QTestState **from, QTestState **to, got_dst_resume = false; if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { memory_size = "150M"; + machine_alias = "pc"; arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath); start_address = X86_TEST_MEM_START; end_address = X86_TEST_MEM_END; } else if (g_str_equal(arch, "s390x")) { memory_size = "128M"; + machine_alias = "s390-ccw-virtio"; arch_opts = g_strdup_printf("-bios %s", bootpath); start_address = S390_TEST_MEM_START; end_address = S390_TEST_MEM_END; @@ -771,11 +774,14 @@ static int test_migrate_start(QTestState **from, QTestState **to, "'nvramrc=hex .\" _\" begin %x %x " "do i c@ 1 + i c! 1000 +loop .\" B\" 0 " "until'", end_address, start_address); - arch_opts = g_strdup("-nodefaults -machine vsmt=8"); + machine_alias = "pseries"; + machine_opts = "vsmt=8"; + arch_opts = g_strdup("-nodefaults"); } else if (strcmp(arch, "aarch64") == 0) { memory_size = "150M"; - arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max " - "-kernel %s", bootpath); + machine_alias = "virt"; + machine_opts = "gic-version=max"; + arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath); start_address = ARM_TEST_MEM_START; end_address = ARM_TEST_MEM_END; } else { @@ -810,11 +816,13 @@ static int test_migrate_start(QTestState **from, QTestState **to, } cmd_source = g_strdup_printf("-accel kvm%s -accel tcg " + "-machine %s,%s " "-name source,debug-threads=on " "-m %s " "-serial file:%s/src_serial " "%s %s %s %s %s", kvm_opts ? kvm_opts : "", + machine_alias, machine_opts, memory_size, tmpfs, arch_opts ? arch_opts : "", arch_source ? arch_source : "", @@ -829,12 +837,14 @@ static int test_migrate_start(QTestState **from, QTestState **to, } cmd_target = g_strdup_printf("-accel kvm%s -accel tcg " + "-machine %s,%s " "-name target,debug-threads=on " "-m %s " "-serial file:%s/dest_serial " "-incoming %s " "%s %s %s %s %s", kvm_opts ? kvm_opts : "", + machine_alias, machine_opts, memory_size, tmpfs, uri, arch_opts ? arch_opts : "", arch_target ? arch_target : "", From 3cb9c6553be28b01761c97768abb41c4b5aaa333 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:37 -0300 Subject: [PATCH 13/17] tests/qtest/migration: Specify the geometry of the bootsector We're about to enable the x86_64 tests to run with the q35 machine, but that machine does not work with the program we use to dirty the memory for the tests. The issue is that QEMU needs to guess the geometry of the "disk" we give to it and the guessed geometry doesn't pass the sanity checks done by SeaBIOS. This causes SeaBIOS to interpret the geometry as if needing a translation from LBA to CHS and SeaBIOS ends up miscomputing the number of cylinders and aborting due to that. The reason things work with the "pc" machine is that is uses ATA instead of AHCI like q35 and SeaBIOS has an exception for ATA that ends up skipping the sanity checks and ignoring translation altogether. Workaround this situation by specifying a geometry in the command line. Signed-off-by: Fabiano Rosas Acked-by: Thomas Huth Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-9-farosas@suse.de> --- tests/qtest/migration-test.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index dfea75b76f..9959a0dc12 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -757,7 +757,9 @@ static int test_migrate_start(QTestState **from, QTestState **to, if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { memory_size = "150M"; machine_alias = "pc"; - arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath); + arch_opts = g_strdup_printf( + "-drive if=none,id=d0,file=%s,format=raw " + "-device ide-hd,drive=d0,secs=1,cyls=1,heads=1", bootpath); start_address = X86_TEST_MEM_START; end_address = X86_TEST_MEM_END; } else if (g_str_equal(arch, "s390x")) { From fa35b0cb25775abb95f61e219b14b63084fb7c5a Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:38 -0300 Subject: [PATCH 14/17] tests/qtest/migration: Set q35 as the default machine for x86_86 Change the x86_64 to use the q35 machines in tests from now on. Keep testing the pc macine on 32bit. Signed-off-by: Fabiano Rosas Reviewed-by: Thomas Huth Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-10-farosas@suse.de> --- tests/qtest/migration-test.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 9959a0dc12..03f3feac7b 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -756,7 +756,12 @@ static int test_migrate_start(QTestState **from, QTestState **to, got_dst_resume = false; if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { memory_size = "150M"; - machine_alias = "pc"; + + if (g_str_equal(arch, "i386")) { + machine_alias = "pc"; + } else { + machine_alias = "q35"; + } arch_opts = g_strdup_printf( "-drive if=none,id=d0,file=%s,format=raw " "-device ide-hd,drive=d0,secs=1,cyls=1,heads=1", bootpath); From 5050ad2a380832a62c7dedda147bbee06c8fe924 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:39 -0300 Subject: [PATCH 15/17] tests/qtest/migration: Support more than one QEMU binary We have strict rules around migration compatibility between different QEMU versions but no test to validate the migration state between different binaries. Add infrastructure to allow running the migration tests with two different QEMU binaries as migration source and destination. The code now recognizes two new environment variables QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of either of them, the test will use the QTEST_QEMU_BINARY variable. If both are missing then the tests are run with single binary as previously. The machine type is selected automatically as the latest machine type version that works with both binaries. Usage (only one of SRC|DST is allowed): QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \ QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \ ./tests/qtest/migration-test Reviewed-by: Juan Quintela Signed-off-by: Fabiano Rosas Reviewed-by: Thomas Huth Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-11-farosas@suse.de> --- tests/qtest/migration-test.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 03f3feac7b..4a5d37317a 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -71,6 +71,8 @@ static bool got_dst_resume; #define QEMU_VM_FILE_MAGIC 0x5145564d #define FILE_TEST_FILENAME "migfile" #define FILE_TEST_OFFSET 0x1000 +#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC" +#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST" #if defined(__linux__) #include @@ -744,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, const char *arch = qtest_get_arch(); const char *memory_size; const char *machine_alias, *machine_opts = ""; + g_autofree char *machine = NULL; if (args->use_shmem) { if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) { @@ -822,6 +825,10 @@ static int test_migrate_start(QTestState **from, QTestState **to, kvm_opts = ",dirty-ring-size=4096"; } + machine = find_common_machine_version(machine_alias, QEMU_ENV_SRC, + QEMU_ENV_DST); + g_test_message("Using machine type: %s", machine); + cmd_source = g_strdup_printf("-accel kvm%s -accel tcg " "-machine %s,%s " "-name source,debug-threads=on " @@ -829,7 +836,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, "-serial file:%s/src_serial " "%s %s %s %s %s", kvm_opts ? kvm_opts : "", - machine_alias, machine_opts, + machine, machine_opts, memory_size, tmpfs, arch_opts ? arch_opts : "", arch_source ? arch_source : "", @@ -837,7 +844,7 @@ static int test_migrate_start(QTestState **from, QTestState **to, args->opts_source ? args->opts_source : "", ignore_stderr); if (!args->only_target) { - *from = qtest_init(cmd_source); + *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source); qtest_qmp_set_event_callback(*from, migrate_watch_for_stop, &got_src_stop); @@ -851,14 +858,14 @@ static int test_migrate_start(QTestState **from, QTestState **to, "-incoming %s " "%s %s %s %s %s", kvm_opts ? kvm_opts : "", - machine_alias, machine_opts, + machine, machine_opts, memory_size, tmpfs, uri, arch_opts ? arch_opts : "", arch_target ? arch_target : "", shmem_opts ? shmem_opts : "", args->opts_target ? args->opts_target : "", ignore_stderr); - *to = qtest_init(cmd_target); + *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target); qtest_qmp_set_event_callback(*to, migrate_watch_for_resume, &got_dst_resume); @@ -2989,10 +2996,23 @@ int main(int argc, char **argv) bool has_uffd; const char *arch; g_autoptr(GError) err = NULL; + const char *qemu_src = getenv(QEMU_ENV_SRC); + const char *qemu_dst = getenv(QEMU_ENV_DST); int ret; g_test_init(&argc, &argv, NULL); + /* + * The default QTEST_QEMU_BINARY must always be provided because + * that is what helpers use to query the accel type and + * architecture. + */ + if (qemu_src && qemu_dst) { + g_test_message("Only one of %s, %s is allowed", + QEMU_ENV_SRC, QEMU_ENV_DST); + exit(1); + } + has_kvm = qtest_has_accel("kvm"); has_tcg = qtest_has_accel("tcg"); From 6c6d2330a07d8a1ba1c1613d8631599a072c5544 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:40 -0300 Subject: [PATCH 16/17] tests/qtest/migration: Allow user to specify a machine type Accept the QTEST_QEMU_MACHINE_TYPE environment variable to take a machine type to use in the tests. The full machine type is recognized (e.g. pc-q35-8.2). Aliases (e.g. pc) are also allowed and resolve to the latest machine version for that alias, or, if using two QEMU binaries, to the latest common machine version between the two. Signed-off-by: Fabiano Rosas Reviewed-by: Thomas Huth Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela Message-ID: <20231018192741.25885-12-farosas@suse.de> --- tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++ tests/qtest/migration-helpers.h | 2 ++ tests/qtest/migration-test.c | 5 +++-- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index 13449c1fe1..24fb7b3525 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -11,6 +11,7 @@ */ #include "qemu/osdep.h" +#include "qemu/ctype.h" #include "qapi/qmp/qjson.h" #include "migration-helpers.h" @@ -266,3 +267,28 @@ char *find_common_machine_version(const char *mtype, const char *var1, "binaries %s and %s", mtype, getenv(var1), getenv(var2)); g_assert_not_reached(); } + +char *resolve_machine_version(const char *alias, const char *var1, + const char *var2) +{ + const char *mname = g_getenv("QTEST_QEMU_MACHINE_TYPE"); + g_autofree char *machine_name = NULL; + + if (mname) { + const char *dash = strrchr(mname, '-'); + const char *dot = strrchr(mname, '.'); + + machine_name = g_strdup(mname); + + if (dash && dot) { + assert(qtest_has_machine(machine_name)); + return g_steal_pointer(&machine_name); + } + /* else: probably an alias, let it be resolved below */ + } else { + /* use the hardcoded alias */ + machine_name = g_strdup(alias); + } + + return find_common_machine_version(machine_name, var1, var2); +} diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h index d1c2351d33..e31dc85cc7 100644 --- a/tests/qtest/migration-helpers.h +++ b/tests/qtest/migration-helpers.h @@ -45,4 +45,6 @@ void wait_for_migration_fail(QTestState *from, bool allow_active); char *find_common_machine_version(const char *mtype, const char *var1, const char *var2); +char *resolve_machine_version(const char *alias, const char *var1, + const char *var2); #endif /* MIGRATION_HELPERS_H */ diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 4a5d37317a..bc70a14642 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -825,8 +825,9 @@ static int test_migrate_start(QTestState **from, QTestState **to, kvm_opts = ",dirty-ring-size=4096"; } - machine = find_common_machine_version(machine_alias, QEMU_ENV_SRC, - QEMU_ENV_DST); + machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC, + QEMU_ENV_DST); + g_test_message("Using machine type: %s", machine); cmd_source = g_strdup_printf("-accel kvm%s -accel tcg " From 7789331b03ae3bffcb2de925a093796b3b9907ff Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Wed, 18 Oct 2023 16:27:41 -0300 Subject: [PATCH 17/17] tests/qtest: Don't print messages from query instances Now that we can query more than one binary, the "starting QEMU..." message can get a little noisy. Mute those messages unless we're running with --verbose. Only affects qtest_init() calls from within libqtest. The tests continue to output as usual. Reviewed-by: Juan Quintela Reviewed-by: Thomas Huth Signed-off-by: Fabiano Rosas Message-ID: <20231018192741.25885-13-farosas@suse.de> Signed-off-by: Juan Quintela --- tests/qtest/libqtest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index c843c41188..f33a210861 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -91,6 +91,7 @@ struct QTestState static GHookList abrt_hooks; static void (*sighandler_old)(int); +static bool silence_spawn_log; static int qtest_query_target_endianness(QTestState *s); @@ -405,7 +406,9 @@ static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin, qtest_add_abrt_handler(kill_qemu_hook_func, s); - g_test_message("starting QEMU: %s", command->str); + if (!silence_spawn_log) { + g_test_message("starting QEMU: %s", command->str); + } #ifndef _WIN32 s->qemu_pid = fork(); @@ -1508,6 +1511,8 @@ static struct MachInfo *qtest_get_machines(const char *var) return machines; } + silence_spawn_log = !g_test_verbose(); + qts = qtest_init_with_env(qemu_var, "-machine none"); response = qtest_qmp(qts, "{ 'execute': 'query-machines' }"); g_assert(response); @@ -1539,6 +1544,8 @@ static struct MachInfo *qtest_get_machines(const char *var) qtest_quit(qts); qobject_unref(response); + silence_spawn_log = false; + memset(&machines[idx], 0, sizeof(struct MachInfo)); /* Terminating entry */ return machines; }