[PATCH v2 0/4] qemu: Fix migration with custom XML
Version 2: - see patch 1/4 for more details - added two cleanups Jiri Denemark (4): qemu: Fix migration with custom XML NEWS: Mention migration bug with custom XML qemu: Change return type of qemuDomainUpdateCPU to void qemu: Change return type of qemuDomainFixupCPUs to void NEWS.rst | 8 +++++++ src/qemu/qemu_domain.c | 51 ++++++++++++++++++++--------------------- src/qemu/qemu_domain.h | 4 ++-- src/qemu/qemu_process.c | 29 ++++++++--------------- 4 files changed, 45 insertions(+), 47 deletions(-) -- 2.44.0
Ages ago origCPU in domain private data was introduced to provide backward compatibility when migrating to an old libvirt, which did not support fetching updated CPU definition from QEMU. Thus origCPU will contain the original CPU definition before such update. But only if the update actually changed anything. Let's always fill origCPU with the original definition when starting a domain so that we can rely on it being always set, even if it matches the updated definition. This fixes migration or save operations with custom domain XML after commit v10.1.0-88-g14d3517410, which expected origCPU to be always set to the CPU definition from inactive XML to check features explicitly requested by a user. https://issues.redhat.com/browse/RHEL-30622 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Tested-by: Han Han <hhan@redhat.com> --- Notes: Version 2: - virCPUDefCopy never returns NULL - do not remove !*origCPU check in qemuDomainFixupCPUs - make sure origCPU is set when reconnecting to running domains src/qemu/qemu_domain.c | 33 ++++++++++++++++++++------------- src/qemu/qemu_process.c | 19 +++++++------------ 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6b869797a8..d91c7d478b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10995,12 +10995,13 @@ virSaveCookieCallbacks virQEMUDriverDomainSaveCookie = { * @vm: domain which is being started * @cpu: CPU updated when the domain was running previously (before migration, * snapshot, or save) - * @origCPU: where to store the original CPU from vm->def in case @cpu was - * used instead + * @origCPU: where to store the original CPU from vm->def * - * Replace the CPU definition with the updated one when QEMU is new enough to - * allow us to check extra features it is about to enable or disable when - * starting a domain. The original CPU is stored in @origCPU. + * Save the original CPU definition from inactive XML in @origCPU so that we + * can safely update it and still be able to check what exactly a user asked + * for. The domain definition will either contain a copy of the original CPU + * definition or a copy of @cpu in case the domain was already running and + * we're just restoring a saved state or preparing for incoming migration. * * Returns 0 on success, -1 on error. */ @@ -11013,18 +11014,24 @@ qemuDomainUpdateCPU(virDomainObj *vm, *origCPU = NULL; - if (!cpu || !vm->def->cpu || - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION) || - virCPUDefIsEqual(vm->def->cpu, cpu, false)) + if (!vm->def->cpu) return 0; - if (!(cpu = virCPUDefCopy(cpu))) - return -1; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) + return 0; + + /* nothing to do if only topology part of CPU def is used */ + if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model) + return 0; + + VIR_DEBUG("Replacing CPU definition"); - VIR_DEBUG("Replacing CPU def with the updated one"); + *origCPU = g_steal_pointer(&vm->def->cpu); - *origCPU = vm->def->cpu; - vm->def->cpu = cpu; + if (cpu) + vm->def->cpu = virCPUDefCopy(cpu); + else + vm->def->cpu = virCPUDefCopy(*origCPU); return 0; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e4bcb628cf..f3e295ccf0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4437,8 +4437,6 @@ qemuProcessUpdateLiveGuestCPU(virDomainObj *vm, virCPUData *disabled) { virDomainDef *def = vm->def; - qemuDomainObjPrivate *priv = vm->privateData; - g_autoptr(virCPUDef) orig = NULL; int rc; if (!enabled) @@ -4449,19 +4447,11 @@ qemuProcessUpdateLiveGuestCPU(virDomainObj *vm, !def->cpu->model)) return 0; - orig = virCPUDefCopy(def->cpu); - - if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0) { + if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0) return -1; - } else if (rc == 0) { - /* Store the original CPU in priv if QEMU changed it and we didn't - * get the original CPU via migration, restore, or snapshot revert. - */ - if (!priv->origCPU && !virCPUDefIsEqual(def->cpu, orig, false)) - priv->origCPU = g_steal_pointer(&orig); + if (rc == 0) def->cpu->check = VIR_CPU_CHECK_FULL; - } return 0; } @@ -9147,6 +9137,11 @@ qemuProcessReconnect(void *opaque) qemuDomainVcpuPersistOrder(obj->def); + /* Make sure priv->origCPU is always set. */ + if (!priv->origCPU && + qemuDomainUpdateCPU(obj, NULL, &priv->origCPU) < 0) + goto error; + if (qemuProcessRefreshCPU(driver, obj) < 0) goto error; -- 2.44.0
On Wed, Apr 17, 2024 at 14:36:08 +0200, Jiri Denemark wrote:
Ages ago origCPU in domain private data was introduced to provide backward compatibility when migrating to an old libvirt, which did not support fetching updated CPU definition from QEMU. Thus origCPU will contain the original CPU definition before such update. But only if the update actually changed anything. Let's always fill origCPU with the original definition when starting a domain so that we can rely on it being always set, even if it matches the updated definition.
This fixes migration or save operations with custom domain XML after commit v10.1.0-88-g14d3517410, which expected origCPU to be always set to the CPU definition from inactive XML to check features explicitly requested by a user.
https://issues.redhat.com/browse/RHEL-30622
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Tested-by: Han Han <hhan@redhat.com> ---
[...]
@@ -9147,6 +9137,11 @@ qemuProcessReconnect(void *opaque)
qemuDomainVcpuPersistOrder(obj->def);
+ /* Make sure priv->origCPU is always set. */
Comment is misleading because there are situations when it still will be NULL.
+ if (!priv->origCPU && + qemuDomainUpdateCPU(obj, NULL, &priv->origCPU) < 0) + goto error; + if (qemuProcessRefreshCPU(driver, obj) < 0) goto error;
With the comment fixed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index c108e66f46..852dadf532 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -21,6 +21,14 @@ v10.3.0 (unreleased) * **Bug fixes** + * qemu: Fix migration with custom XML + + Libvirt 10.2.0 would sometimes complain about incompatible CPU definition + when trying to migrate or save a domain and passing a custom XML even + though such XML was properly generated as migratable. Hitting this bug + depends on the guest CPU definition and the host on which a particular + domain was running. + v10.2.0 (2024-04-02) ==================== -- 2.44.0
The function never fails. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++-------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_process.c | 8 +++----- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d91c7d478b..3dfabfda2f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11002,10 +11002,8 @@ virSaveCookieCallbacks virQEMUDriverDomainSaveCookie = { * for. The domain definition will either contain a copy of the original CPU * definition or a copy of @cpu in case the domain was already running and * we're just restoring a saved state or preparing for incoming migration. - * - * Returns 0 on success, -1 on error. */ -int +void qemuDomainUpdateCPU(virDomainObj *vm, virCPUDef *cpu, virCPUDef **origCPU) @@ -11015,14 +11013,14 @@ qemuDomainUpdateCPU(virDomainObj *vm, *origCPU = NULL; if (!vm->def->cpu) - return 0; + return; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) - return 0; + return; /* nothing to do if only topology part of CPU def is used */ if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model) - return 0; + return; VIR_DEBUG("Replacing CPU definition"); @@ -11032,8 +11030,6 @@ qemuDomainUpdateCPU(virDomainObj *vm, vm->def->cpu = virCPUDefCopy(cpu); else vm->def->cpu = virCPUDefCopy(*origCPU); - - return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index bd37cb245a..264817eef9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -980,7 +980,7 @@ virStorageSource *qemuDomainGetStorageSourceByDevstr(const char *devstr, virDomainDef *def, virDomainBackupDef *backupdef); -int +void qemuDomainUpdateCPU(virDomainObj *vm, virCPUDef *cpu, virCPUDef **origCPU); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f3e295ccf0..7fdb9ac23a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5674,8 +5674,7 @@ qemuProcessInit(virQEMUDriver *driver, if (qemuProcessPrepareQEMUCaps(vm, driver->qemuCapsCache) < 0) return -1; - if (qemuDomainUpdateCPU(vm, updatedCPU, &origCPU) < 0) - return -1; + qemuDomainUpdateCPU(vm, updatedCPU, &origCPU); if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, flags) < 0) return -1; @@ -9138,9 +9137,8 @@ qemuProcessReconnect(void *opaque) qemuDomainVcpuPersistOrder(obj->def); /* Make sure priv->origCPU is always set. */ - if (!priv->origCPU && - qemuDomainUpdateCPU(obj, NULL, &priv->origCPU) < 0) - goto error; + if (!priv->origCPU) + qemuDomainUpdateCPU(obj, NULL, &priv->origCPU); if (qemuProcessRefreshCPU(driver, obj) < 0) goto error; -- 2.44.0
On Wed, Apr 17, 2024 at 14:36:10 +0200, Jiri Denemark wrote:
The function never fails.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++-------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_process.c | 8 +++----- 3 files changed, 8 insertions(+), 14 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The function never fails. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: Version 2: - virCPUDefCopy never returns NULL - do not remove !*origCPU check in qemuDomainFixupCPUs - make sure origCPU is set when reconnecting to running domains src/qemu/qemu_domain.c | 12 ++++-------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_process.c | 8 +++----- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3dfabfda2f..3469f0d40c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11050,29 +11050,27 @@ qemuDomainUpdateCPU(virDomainObj *vm, * * This function can only be used on an active domain or when restoring a * domain which was running. - * - * Returns 0 on success, -1 on error. */ -int +void qemuDomainFixupCPUs(virDomainObj *vm, virCPUDef **origCPU) { virArch arch = vm->def->os.arch; if (!ARCH_IS_X86(arch)) - return 0; + return; if (!vm->def->cpu || vm->def->cpu->mode != VIR_CPU_MODE_CUSTOM || !vm->def->cpu->model) - return 0; + return; /* Missing origCPU means QEMU created exactly the same virtual CPU which * we asked for or libvirt was too old to mess up the translation from * host-model. */ if (!*origCPU) - return 0; + return; if (virCPUDefFindFeature(vm->def->cpu, "cmt")) { g_autoptr(virCPUDef) fixedCPU = virCPUDefCopyWithoutModel(vm->def->cpu); @@ -11093,8 +11091,6 @@ qemuDomainFixupCPUs(virDomainObj *vm, virCPUDefFree(*origCPU); *origCPU = g_steal_pointer(&fixedOrig); } - - return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 264817eef9..a3089ea449 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -985,7 +985,7 @@ qemuDomainUpdateCPU(virDomainObj *vm, virCPUDef *cpu, virCPUDef **origCPU); -int +void qemuDomainFixupCPUs(virDomainObj *vm, virCPUDef **origCPU); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7fdb9ac23a..686f6ed6c1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8278,9 +8278,8 @@ qemuProcessStartWithMemoryState(virConnectPtr conn, /* No cookie means libvirt which saved the domain was too old to mess up * the CPU definitions. */ - if (cookie && - qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - return -1; + if (cookie) + qemuDomainFixupCPUs(vm, &cookie->cpu); if (cookie && !cookie->slirpHelper) priv->disableSlirp = true; @@ -8926,8 +8925,7 @@ qemuProcessRefreshCPU(virQEMUDriver *driver, * case the host-model is known to not contain features which QEMU * doesn't know about. */ - if (qemuDomainFixupCPUs(vm, &priv->origCPU) < 0) - return -1; + qemuDomainFixupCPUs(vm, &priv->origCPU); } return 0; -- 2.44.0
participants (2)
-
Jiri Denemark -
Peter Krempa