[libvirt] [PATCH 0/7] qemu: Update host-model CPUs on reconnect
This series fixes migration of a domain with a host-model CPU started by an old libvirt (< 2.3). https://bugzilla.redhat.com/show_bug.cgi?id=1463957 Jiri Denemark (7): qemu: Add qemuProcessFetchGuestCPU qemu: Add qemuProcessVerifyCPU qemu: Rename qemuProcessUpdateLiveGuestCPU qemu: Add qemuProcessUpdateLiveGuestCPU qemu: Export virQEMUCapsGuestIsNative qemu: Move qemuProcessReconnect to the end of qemu_process.c qemu: Update host-model CPUs on reconnect src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_process.c | 856 ++++++++++++++++++++++++------------------- 3 files changed, 489 insertions(+), 372 deletions(-) -- 2.13.2
Separated from qemuProcessUpdateLiveGuestCPU. Its purpose is to fetch guest CPU data from a running QEMU process. The data can later be used to verify and update the active guest CPU definition. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e6522a294..b2d27b6be 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3945,6 +3945,47 @@ qemuProcessVerifyCPUFeatures(virDomainDefPtr def, static int +qemuProcessFetchGuestCPU(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + virCPUDataPtr *enabled, + virCPUDataPtr *disabled) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virCPUDataPtr dataEnabled = NULL; + virCPUDataPtr dataDisabled = NULL; + int rc; + + *enabled = NULL; + *disabled = NULL; + + if (!ARCH_IS_X86(vm->def->os.arch)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto error; + + rc = qemuMonitorGetGuestCPU(priv->mon, vm->def->os.arch, + &dataEnabled, &dataDisabled); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto error; + + if (rc == -1) + goto error; + + *enabled = dataEnabled; + *disabled = dataDisabled; + return 0; + + error: + virCPUDataFree(dataEnabled); + virCPUDataFree(dataDisabled); + return -1; +} + + +static int qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob) @@ -3957,21 +3998,10 @@ qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver, int ret = -1; virCPUDefPtr orig = NULL; - if (ARCH_IS_X86(def->os.arch)) { - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - goto cleanup; - - rc = qemuMonitorGetGuestCPU(priv->mon, def->os.arch, &cpu, &disabled); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - - if (rc < 0) { - if (rc == -2) - ret = 0; - goto cleanup; - } + if (qemuProcessFetchGuestCPU(driver, vm, asyncJob, &cpu, &disabled) < 0) + goto cleanup; + if (cpu) { if (qemuProcessVerifyKVMFeatures(def, cpu) < 0 || qemuProcessVerifyHypervFeatures(def, cpu) < 0) goto cleanup; -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:47PM +0200, Jiri Denemark wrote:
Separated from qemuProcessUpdateLiveGuestCPU. Its purpose is to fetch guest CPU data from a running QEMU process. The data can later be used to verify and update the active guest CPU definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 14 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Separated from qemuProcessUpdateLiveGuestCPU. The function makes sure a guest CPU provides all features required by a domain definition. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b2d27b6be..198f68d93 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3986,6 +3986,31 @@ qemuProcessFetchGuestCPU(virQEMUDriverPtr driver, static int +qemuProcessVerifyCPU(virDomainObjPtr vm, + virCPUDataPtr cpu) +{ + virDomainDefPtr def = vm->def; + + if (!cpu) + return 0; + + if (qemuProcessVerifyKVMFeatures(def, cpu) < 0 || + qemuProcessVerifyHypervFeatures(def, cpu) < 0) + return -1; + + if (!def->cpu || + (def->cpu->mode == VIR_CPU_MODE_CUSTOM && + !def->cpu->model)) + return 0; + + if (qemuProcessVerifyCPUFeatures(def, cpu) < 0) + return -1; + + return 0; +} + + +static int qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob) @@ -4001,11 +4026,10 @@ qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver, if (qemuProcessFetchGuestCPU(driver, vm, asyncJob, &cpu, &disabled) < 0) goto cleanup; - if (cpu) { - if (qemuProcessVerifyKVMFeatures(def, cpu) < 0 || - qemuProcessVerifyHypervFeatures(def, cpu) < 0) - goto cleanup; + if (qemuProcessVerifyCPU(vm, cpu) < 0) + goto cleanup; + if (cpu) { if (!def->cpu || (def->cpu->mode == VIR_CPU_MODE_CUSTOM && !def->cpu->model)) { @@ -4013,9 +4037,6 @@ qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver, goto cleanup; } - if (qemuProcessVerifyCPUFeatures(def, cpu) < 0) - goto cleanup; - if (!(orig = virCPUDefCopy(def->cpu))) goto cleanup; -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:48PM +0200, Jiri Denemark wrote:
Separated from qemuProcessUpdateLiveGuestCPU. The function makes sure a guest CPU provides all features required by a domain definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b2d27b6be..198f68d93 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3986,6 +3986,31 @@ qemuProcessFetchGuestCPU(virQEMUDriverPtr driver,
static int +qemuProcessVerifyCPU(virDomainObjPtr vm, + virCPUDataPtr cpu) +{ + virDomainDefPtr def = vm->def; + + if (!cpu) + return 0; + + if (qemuProcessVerifyKVMFeatures(def, cpu) < 0 || + qemuProcessVerifyHypervFeatures(def, cpu) < 0) + return -1; + + if (!def->cpu || + (def->cpu->mode == VIR_CPU_MODE_CUSTOM && + !def->cpu->model))
This condition is now on two places, it would be worth to create a macro/function with some description why this specific condition.
+ return 0; + + if (qemuProcessVerifyCPUFeatures(def, cpu) < 0) + return -1; + + return 0; +} +
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Wed, Jul 12, 2017 at 15:14:20 +0200, Pavel Hrdina wrote:
On Wed, Jul 12, 2017 at 02:56:48PM +0200, Jiri Denemark wrote:
Separated from qemuProcessUpdateLiveGuestCPU. The function makes sure a guest CPU provides all features required by a domain definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b2d27b6be..198f68d93 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3986,6 +3986,31 @@ qemuProcessFetchGuestCPU(virQEMUDriverPtr driver,
static int +qemuProcessVerifyCPU(virDomainObjPtr vm, + virCPUDataPtr cpu) +{ + virDomainDefPtr def = vm->def; + + if (!cpu) + return 0; + + if (qemuProcessVerifyKVMFeatures(def, cpu) < 0 || + qemuProcessVerifyHypervFeatures(def, cpu) < 0) + return -1; + + if (!def->cpu || + (def->cpu->mode == VIR_CPU_MODE_CUSTOM && + !def->cpu->model))
This condition is now on two places, it would be worth to create a macro/function with some description why this specific condition.
Oh yeah, good idea. This condition is very likely repeated in a lot of places which need to check whether the CPU def contains only topology data. I'll try to come up with a good name for the function and make a patch replacing all open coded conditions with it. Jirka
In addition to updating a guest CPU definition the function verifies that all required features are provided to the guest. Let's make it obvious by calling it qemuProcessUpdateAndVerifyCPU. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 198f68d93..ebd13057b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4011,7 +4011,7 @@ qemuProcessVerifyCPU(virDomainObjPtr vm, static int -qemuProcessUpdateLiveGuestCPU(virQEMUDriverPtr driver, +qemuProcessUpdateAndVerifyCPU(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob) { @@ -5928,7 +5928,7 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; VIR_DEBUG("Verifying and updating provided guest CPU"); - if (qemuProcessUpdateLiveGuestCPU(driver, vm, asyncJob) < 0) + if (qemuProcessUpdateAndVerifyCPU(driver, vm, asyncJob) < 0) goto cleanup; VIR_DEBUG("Setting up post-init cgroup restrictions"); -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:49PM +0200, Jiri Denemark wrote:
In addition to updating a guest CPU definition the function verifies that all required features are provided to the guest. Let's make it obvious by calling it qemuProcessUpdateAndVerifyCPU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Separated from qemuProcessUpdateAndVerifyCPU to handle updating of an active guest CPU definition according to live data from QEMU. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 70 +++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ebd13057b..926c64197 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4011,17 +4011,53 @@ qemuProcessVerifyCPU(virDomainObjPtr vm, static int +qemuProcessUpdateLiveGuestCPU(virDomainObjPtr vm, + virCPUDataPtr enabled, + virCPUDataPtr disabled) +{ + virDomainDefPtr def = vm->def; + qemuDomainObjPrivatePtr priv = vm->privateData; + virCPUDefPtr orig = NULL; + int rc; + int ret = -1; + + if (!enabled || + !def->cpu || + (def->cpu->mode == VIR_CPU_MODE_CUSTOM && + !def->cpu->model)) + return 0; + + if (!(orig = virCPUDefCopy(def->cpu))) + goto cleanup; + + if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0) { + goto cleanup; + } 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)) + VIR_STEAL_PTR(priv->origCPU, orig); + + def->cpu->check = VIR_CPU_CHECK_FULL; + } + + ret = 0; + + cleanup: + virCPUDefFree(orig); + return ret; +} + + +static int qemuProcessUpdateAndVerifyCPU(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob) { - virDomainDefPtr def = vm->def; virCPUDataPtr cpu = NULL; virCPUDataPtr disabled = NULL; - qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; int ret = -1; - virCPUDefPtr orig = NULL; if (qemuProcessFetchGuestCPU(driver, vm, asyncJob, &cpu, &disabled) < 0) goto cleanup; @@ -4029,36 +4065,14 @@ qemuProcessUpdateAndVerifyCPU(virQEMUDriverPtr driver, if (qemuProcessVerifyCPU(vm, cpu) < 0) goto cleanup; - if (cpu) { - if (!def->cpu || - (def->cpu->mode == VIR_CPU_MODE_CUSTOM && - !def->cpu->model)) { - ret = 0; - goto cleanup; - } - - if (!(orig = virCPUDefCopy(def->cpu))) - goto cleanup; - - if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, cpu, disabled)) < 0) { - goto cleanup; - } 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)) - VIR_STEAL_PTR(priv->origCPU, orig); - - def->cpu->check = VIR_CPU_CHECK_FULL; - } - } + if (qemuProcessUpdateLiveGuestCPU(vm, cpu, disabled) < 0) + goto cleanup; ret = 0; cleanup: virCPUDataFree(cpu); virCPUDataFree(disabled); - virCPUDefFree(orig); return ret; } -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:50PM +0200, Jiri Denemark wrote:
Separated from qemuProcessUpdateAndVerifyCPU to handle updating of an active guest CPU definition according to live data from QEMU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 70 +++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ebd13057b..926c64197 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4011,17 +4011,53 @@ qemuProcessVerifyCPU(virDomainObjPtr vm,
static int +qemuProcessUpdateLiveGuestCPU(virDomainObjPtr vm, + virCPUDataPtr enabled, + virCPUDataPtr disabled) +{ + virDomainDefPtr def = vm->def; + qemuDomainObjPrivatePtr priv = vm->privateData; + virCPUDefPtr orig = NULL; + int rc; + int ret = -1; + + if (!enabled || + !def->cpu || + (def->cpu->mode == VIR_CPU_MODE_CUSTOM && + !def->cpu->model))
Now the condition is extended by another check, this makes the code fragile. I would prefer separating the "!enabled".
+ return 0; + + if (!(orig = virCPUDefCopy(def->cpu))) + goto cleanup; + + if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0) { + goto cleanup; + } 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)) + VIR_STEAL_PTR(priv->origCPU, orig); + + def->cpu->check = VIR_CPU_CHECK_FULL; + } + + ret = 0; + + cleanup: + virCPUDefFree(orig); + return ret; +}
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Wed, Jul 12, 2017 at 15:18:58 +0200, Pavel Hrdina wrote:
On Wed, Jul 12, 2017 at 02:56:50PM +0200, Jiri Denemark wrote:
Separated from qemuProcessUpdateAndVerifyCPU to handle updating of an active guest CPU definition according to live data from QEMU.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 70 +++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ebd13057b..926c64197 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4011,17 +4011,53 @@ qemuProcessVerifyCPU(virDomainObjPtr vm,
static int +qemuProcessUpdateLiveGuestCPU(virDomainObjPtr vm, + virCPUDataPtr enabled, + virCPUDataPtr disabled) +{ + virDomainDefPtr def = vm->def; + qemuDomainObjPrivatePtr priv = vm->privateData; + virCPUDefPtr orig = NULL; + int rc; + int ret = -1; + + if (!enabled || + !def->cpu || + (def->cpu->mode == VIR_CPU_MODE_CUSTOM && + !def->cpu->model))
Now the condition is extended by another check, this makes the code fragile. I would prefer separating the "!enabled".
OK Jirka
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_capabilities.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index db9f9b8b1..7cce4b18d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -545,7 +545,7 @@ static const char *virQEMUCapsArchToString(virArch arch) /* Checks whether a domain with @guest arch can run natively on @host. */ -static bool +bool virQEMUCapsGuestIsNative(virArch host, virArch guest) { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index fb22815e9..275e33c72 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -547,4 +547,7 @@ int virQEMUCapsFillDomainCaps(virCapsPtr caps, virFirmwarePtr *firmwares, size_t nfirmwares); +bool virQEMUCapsGuestIsNative(virArch host, + virArch guest); + #endif /* __QEMU_CAPABILITIES_H__*/ -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:51PM +0200, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_capabilities.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
qemuProcessReconnect will need to call additional functions which were originally defined further in qemu_process.c. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 645 ++++++++++++++++++++++++------------------------ 1 file changed, 323 insertions(+), 322 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 926c64197..2339de41c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3371,328 +3371,6 @@ qemuProcessBuildDestroyHugepagesPath(virQEMUDriverPtr driver, } -struct qemuProcessReconnectData { - virConnectPtr conn; - virQEMUDriverPtr driver; - virDomainObjPtr obj; -}; -/* - * Open an existing VM's monitor, re-detect VCPU threads - * and re-reserve the security labels in use - * - * We own the virConnectPtr we are passed here - whoever started - * this thread function has increased the reference counter to it - * so that we now have to close it. - * - * This function also inherits a locked and ref'd domain object. - * - * This function needs to: - * 1. Enter job - * 1. just before monitor reconnect do lightweight MonitorEnter - * (increase VM refcount and unlock VM) - * 2. reconnect to monitor - * 3. do lightweight MonitorExit (lock VM) - * 4. continue reconnect process - * 5. EndJob - * - * We can't do normal MonitorEnter & MonitorExit because these two lock the - * monitor lock, which does not exists in this early phase. - */ -static void -qemuProcessReconnect(void *opaque) -{ - struct qemuProcessReconnectData *data = opaque; - virQEMUDriverPtr driver = data->driver; - virDomainObjPtr obj = data->obj; - qemuDomainObjPrivatePtr priv; - virConnectPtr conn = data->conn; - struct qemuDomainJobObj oldjob; - int state; - int reason; - virQEMUDriverConfigPtr cfg; - size_t i; - unsigned int stopFlags = 0; - bool jobStarted = false; - virCapsPtr caps = NULL; - - VIR_FREE(data); - - qemuDomainObjRestoreJob(obj, &oldjob); - if (oldjob.asyncJob == QEMU_ASYNC_JOB_MIGRATION_IN) - stopFlags |= VIR_QEMU_PROCESS_STOP_MIGRATED; - - cfg = virQEMUDriverGetConfig(driver); - priv = obj->privateData; - - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto error; - - if (qemuDomainObjBeginJob(driver, obj, QEMU_JOB_MODIFY) < 0) - goto error; - jobStarted = true; - - /* XXX If we ever gonna change pid file pattern, come up with - * some intelligence here to deal with old paths. */ - if (!(priv->pidfile = virPidFileBuildPath(cfg->stateDir, obj->def->name))) - goto error; - - /* Restore the masterKey */ - if (qemuDomainMasterKeyReadFile(priv) < 0) - goto error; - - virNWFilterReadLockFilterUpdates(); - - VIR_DEBUG("Reconnect monitor to %p '%s'", obj, obj->def->name); - - /* XXX check PID liveliness & EXE path */ - if (qemuConnectMonitor(driver, obj, QEMU_ASYNC_JOB_NONE, NULL) < 0) - goto error; - - if (qemuHostdevUpdateActiveDomainDevices(driver, obj->def) < 0) - goto error; - - if (qemuConnectCgroup(driver, obj) < 0) - goto error; - - if (qemuDomainPerfRestart(obj) < 0) - goto error; - - /* XXX: Need to change as long as lock is introduced for - * qemu_driver->sharedDevices. - */ - for (i = 0; i < obj->def->ndisks; i++) { - virDomainDeviceDef dev; - - if (virStorageTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) - goto error; - - /* XXX we should be able to restore all data from XML in the future. - * This should be the only place that calls qemuDomainDetermineDiskChain - * with @report_broken == false to guarantee best-effort domain - * reconnect */ - if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], - true, false) < 0) - goto error; - - dev.type = VIR_DOMAIN_DEVICE_DISK; - dev.data.disk = obj->def->disks[i]; - if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0) - goto error; - } - - if (qemuProcessUpdateState(driver, obj) < 0) - goto error; - - state = virDomainObjGetState(obj, &reason); - if (state == VIR_DOMAIN_SHUTOFF || - (state == VIR_DOMAIN_PAUSED && - reason == VIR_DOMAIN_PAUSED_STARTING_UP)) { - VIR_DEBUG("Domain '%s' wasn't fully started yet, killing it", - obj->def->name); - goto error; - } - - /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - !(priv->qemuCaps = virQEMUCapsCacheLookupCopy(caps, - driver->qemuCapsCache, - obj->def->emulator, - obj->def->os.machine))) - goto error; - - /* In case the domain shutdown while we were not running, - * we need to finish the shutdown process. And we need to do it after - * we have virQEMUCaps filled in. - */ - if (state == VIR_DOMAIN_SHUTDOWN || - (state == VIR_DOMAIN_PAUSED && - reason == VIR_DOMAIN_PAUSED_SHUTTING_DOWN)) { - VIR_DEBUG("Finishing shutdown sequence for domain %s", - obj->def->name); - qemuProcessShutdownOrReboot(driver, obj); - goto cleanup; - } - - if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0) - goto error; - - if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, - driver, obj, false)) < 0) { - goto error; - } - - /* if domain requests security driver we haven't loaded, report error, but - * do not kill the domain - */ - ignore_value(qemuSecurityCheckAllLabel(driver->securityManager, - obj->def)); - - if (qemuDomainRefreshVcpuInfo(driver, obj, QEMU_ASYNC_JOB_NONE, true) < 0) - goto error; - - qemuDomainVcpuPersistOrder(obj->def); - - if (qemuSecurityReserveLabel(driver->securityManager, obj->def, obj->pid) < 0) - goto error; - - qemuProcessNotifyNets(obj->def); - - if (qemuProcessFiltersInstantiate(obj->def)) - goto error; - - if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) - goto error; - - if (qemuBlockNodeNamesDetect(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) - goto error; - - if (qemuRefreshVirtioChannelState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) - goto error; - - /* If querying of guest's RTC failed, report error, but do not kill the domain. */ - qemuRefreshRTC(driver, obj); - - if (qemuProcessRefreshBalloonState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) - goto error; - - if (qemuProcessRecoverJob(driver, obj, conn, &oldjob, &stopFlags) < 0) - goto error; - - if (qemuProcessUpdateDevices(driver, obj) < 0) - goto error; - - qemuProcessReconnectCheckMemAliasOrderMismatch(obj); - - if (qemuConnectAgent(driver, obj) < 0) - goto error; - - /* update domain state XML with possibly updated state in virDomainObj */ - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj, driver->caps) < 0) - goto error; - - /* Run an hook to allow admins to do some magic */ - if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - char *xml = qemuDomainDefFormatXML(driver, obj->def, 0); - int hookret; - - hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, obj->def->name, - VIR_HOOK_QEMU_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, NULL); - VIR_FREE(xml); - - /* - * If the script raised an error abort the launch - */ - if (hookret < 0) - goto error; - } - - if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); - - cleanup: - if (jobStarted) - qemuDomainObjEndJob(driver, obj); - if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactive(driver, obj); - virDomainObjEndAPI(&obj); - virObjectUnref(conn); - virObjectUnref(cfg); - virObjectUnref(caps); - virNWFilterUnlockFilterUpdates(); - return; - - error: - if (virDomainObjIsActive(obj)) { - /* We can't get the monitor back, so must kill the VM - * to remove danger of it ending up running twice if - * user tries to start it again later - */ - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) { - /* If we couldn't get the monitor and qemu supports - * no-shutdown, we can safely say that the domain - * crashed ... */ - state = VIR_DOMAIN_SHUTOFF_CRASHED; - } else { - /* ... but if it doesn't we can't say what the state - * really is and FAILED means "failed to start" */ - state = VIR_DOMAIN_SHUTOFF_UNKNOWN; - } - /* If BeginJob failed, we jumped here without a job, let's hope another - * thread didn't have a chance to start playing with the domain yet - * (it's all we can do anyway). - */ - qemuProcessStop(driver, obj, state, QEMU_ASYNC_JOB_NONE, stopFlags); - } - goto cleanup; -} - -static int -qemuProcessReconnectHelper(virDomainObjPtr obj, - void *opaque) -{ - virThread thread; - struct qemuProcessReconnectData *src = opaque; - struct qemuProcessReconnectData *data; - - /* If the VM was inactive, we don't need to reconnect */ - if (!obj->pid) - return 0; - - if (VIR_ALLOC(data) < 0) - return -1; - - memcpy(data, src, sizeof(*data)); - data->obj = obj; - - /* this lock and reference will be eventually transferred to the thread - * that handles the reconnect */ - virObjectLock(obj); - virObjectRef(obj); - - /* Since we close the connection later on, we have to make sure that the - * threads we start see a valid connection throughout their lifetime. We - * simply increase the reference counter here. - */ - virObjectRef(data->conn); - - if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not create thread. QEMU initialization " - "might be incomplete")); - /* We can't spawn a thread and thus connect to monitor. Kill qemu. - * It's safe to call qemuProcessStop without a job here since there - * is no thread that could be doing anything else with the same domain - * object. - */ - qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, - QEMU_ASYNC_JOB_NONE, 0); - qemuDomainRemoveInactive(src->driver, obj); - - virDomainObjEndAPI(&obj); - virObjectUnref(data->conn); - VIR_FREE(data); - return -1; - } - - return 0; -} - -/** - * qemuProcessReconnectAll - * - * Try to re-open the resources for live VMs that we care - * about. - */ -void -qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver) -{ - struct qemuProcessReconnectData data = {.conn = conn, .driver = driver}; - virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data); -} - static int qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, virDomainGraphicsDefPtr graphics, @@ -7012,3 +6690,326 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver, virHashFree(table); return ret; } + + +struct qemuProcessReconnectData { + virConnectPtr conn; + virQEMUDriverPtr driver; + virDomainObjPtr obj; +}; +/* + * Open an existing VM's monitor, re-detect VCPU threads + * and re-reserve the security labels in use + * + * We own the virConnectPtr we are passed here - whoever started + * this thread function has increased the reference counter to it + * so that we now have to close it. + * + * This function also inherits a locked and ref'd domain object. + * + * This function needs to: + * 1. Enter job + * 1. just before monitor reconnect do lightweight MonitorEnter + * (increase VM refcount and unlock VM) + * 2. reconnect to monitor + * 3. do lightweight MonitorExit (lock VM) + * 4. continue reconnect process + * 5. EndJob + * + * We can't do normal MonitorEnter & MonitorExit because these two lock the + * monitor lock, which does not exists in this early phase. + */ +static void +qemuProcessReconnect(void *opaque) +{ + struct qemuProcessReconnectData *data = opaque; + virQEMUDriverPtr driver = data->driver; + virDomainObjPtr obj = data->obj; + qemuDomainObjPrivatePtr priv; + virConnectPtr conn = data->conn; + struct qemuDomainJobObj oldjob; + int state; + int reason; + virQEMUDriverConfigPtr cfg; + size_t i; + unsigned int stopFlags = 0; + bool jobStarted = false; + virCapsPtr caps = NULL; + + VIR_FREE(data); + + qemuDomainObjRestoreJob(obj, &oldjob); + if (oldjob.asyncJob == QEMU_ASYNC_JOB_MIGRATION_IN) + stopFlags |= VIR_QEMU_PROCESS_STOP_MIGRATED; + + cfg = virQEMUDriverGetConfig(driver); + priv = obj->privateData; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto error; + + if (qemuDomainObjBeginJob(driver, obj, QEMU_JOB_MODIFY) < 0) + goto error; + jobStarted = true; + + /* XXX If we ever gonna change pid file pattern, come up with + * some intelligence here to deal with old paths. */ + if (!(priv->pidfile = virPidFileBuildPath(cfg->stateDir, obj->def->name))) + goto error; + + /* Restore the masterKey */ + if (qemuDomainMasterKeyReadFile(priv) < 0) + goto error; + + virNWFilterReadLockFilterUpdates(); + + VIR_DEBUG("Reconnect monitor to %p '%s'", obj, obj->def->name); + + /* XXX check PID liveliness & EXE path */ + if (qemuConnectMonitor(driver, obj, QEMU_ASYNC_JOB_NONE, NULL) < 0) + goto error; + + if (qemuHostdevUpdateActiveDomainDevices(driver, obj->def) < 0) + goto error; + + if (qemuConnectCgroup(driver, obj) < 0) + goto error; + + if (qemuDomainPerfRestart(obj) < 0) + goto error; + + /* XXX: Need to change as long as lock is introduced for + * qemu_driver->sharedDevices. + */ + for (i = 0; i < obj->def->ndisks; i++) { + virDomainDeviceDef dev; + + if (virStorageTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) + goto error; + + /* XXX we should be able to restore all data from XML in the future. + * This should be the only place that calls qemuDomainDetermineDiskChain + * with @report_broken == false to guarantee best-effort domain + * reconnect */ + if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], + true, false) < 0) + goto error; + + dev.type = VIR_DOMAIN_DEVICE_DISK; + dev.data.disk = obj->def->disks[i]; + if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0) + goto error; + } + + if (qemuProcessUpdateState(driver, obj) < 0) + goto error; + + state = virDomainObjGetState(obj, &reason); + if (state == VIR_DOMAIN_SHUTOFF || + (state == VIR_DOMAIN_PAUSED && + reason == VIR_DOMAIN_PAUSED_STARTING_UP)) { + VIR_DEBUG("Domain '%s' wasn't fully started yet, killing it", + obj->def->name); + goto error; + } + + /* If upgrading from old libvirtd we won't have found any + * caps in the domain status, so re-query them + */ + if (!priv->qemuCaps && + !(priv->qemuCaps = virQEMUCapsCacheLookupCopy(caps, + driver->qemuCapsCache, + obj->def->emulator, + obj->def->os.machine))) + goto error; + + /* In case the domain shutdown while we were not running, + * we need to finish the shutdown process. And we need to do it after + * we have virQEMUCaps filled in. + */ + if (state == VIR_DOMAIN_SHUTDOWN || + (state == VIR_DOMAIN_PAUSED && + reason == VIR_DOMAIN_PAUSED_SHUTTING_DOWN)) { + VIR_DEBUG("Finishing shutdown sequence for domain %s", + obj->def->name); + qemuProcessShutdownOrReboot(driver, obj); + goto cleanup; + } + + if (qemuProcessBuildDestroyHugepagesPath(driver, obj, NULL, true) < 0) + goto error; + + if ((qemuDomainAssignAddresses(obj->def, priv->qemuCaps, + driver, obj, false)) < 0) { + goto error; + } + + /* if domain requests security driver we haven't loaded, report error, but + * do not kill the domain + */ + ignore_value(qemuSecurityCheckAllLabel(driver->securityManager, + obj->def)); + + if (qemuDomainRefreshVcpuInfo(driver, obj, QEMU_ASYNC_JOB_NONE, true) < 0) + goto error; + + qemuDomainVcpuPersistOrder(obj->def); + + if (qemuSecurityReserveLabel(driver->securityManager, obj->def, obj->pid) < 0) + goto error; + + qemuProcessNotifyNets(obj->def); + + if (qemuProcessFiltersInstantiate(obj->def)) + goto error; + + if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) + goto error; + + if (qemuBlockNodeNamesDetect(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) + goto error; + + if (qemuRefreshVirtioChannelState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) + goto error; + + /* If querying of guest's RTC failed, report error, but do not kill the domain. */ + qemuRefreshRTC(driver, obj); + + if (qemuProcessRefreshBalloonState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) + goto error; + + if (qemuProcessRecoverJob(driver, obj, conn, &oldjob, &stopFlags) < 0) + goto error; + + if (qemuProcessUpdateDevices(driver, obj) < 0) + goto error; + + qemuProcessReconnectCheckMemAliasOrderMismatch(obj); + + if (qemuConnectAgent(driver, obj) < 0) + goto error; + + /* update domain state XML with possibly updated state in virDomainObj */ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, obj, driver->caps) < 0) + goto error; + + /* Run an hook to allow admins to do some magic */ + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { + char *xml = qemuDomainDefFormatXML(driver, obj->def, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, obj->def->name, + VIR_HOOK_QEMU_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + VIR_FREE(xml); + + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto error; + } + + if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) + driver->inhibitCallback(true, driver->inhibitOpaque); + + cleanup: + if (jobStarted) + qemuDomainObjEndJob(driver, obj); + if (!virDomainObjIsActive(obj)) + qemuDomainRemoveInactive(driver, obj); + virDomainObjEndAPI(&obj); + virObjectUnref(conn); + virObjectUnref(cfg); + virObjectUnref(caps); + virNWFilterUnlockFilterUpdates(); + return; + + error: + if (virDomainObjIsActive(obj)) { + /* We can't get the monitor back, so must kill the VM + * to remove danger of it ending up running twice if + * user tries to start it again later + */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) { + /* If we couldn't get the monitor and qemu supports + * no-shutdown, we can safely say that the domain + * crashed ... */ + state = VIR_DOMAIN_SHUTOFF_CRASHED; + } else { + /* ... but if it doesn't we can't say what the state + * really is and FAILED means "failed to start" */ + state = VIR_DOMAIN_SHUTOFF_UNKNOWN; + } + /* If BeginJob failed, we jumped here without a job, let's hope another + * thread didn't have a chance to start playing with the domain yet + * (it's all we can do anyway). + */ + qemuProcessStop(driver, obj, state, QEMU_ASYNC_JOB_NONE, stopFlags); + } + goto cleanup; +} + +static int +qemuProcessReconnectHelper(virDomainObjPtr obj, + void *opaque) +{ + virThread thread; + struct qemuProcessReconnectData *src = opaque; + struct qemuProcessReconnectData *data; + + /* If the VM was inactive, we don't need to reconnect */ + if (!obj->pid) + return 0; + + if (VIR_ALLOC(data) < 0) + return -1; + + memcpy(data, src, sizeof(*data)); + data->obj = obj; + + /* this lock and reference will be eventually transferred to the thread + * that handles the reconnect */ + virObjectLock(obj); + virObjectRef(obj); + + /* Since we close the connection later on, we have to make sure that the + * threads we start see a valid connection throughout their lifetime. We + * simply increase the reference counter here. + */ + virObjectRef(data->conn); + + if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not create thread. QEMU initialization " + "might be incomplete")); + /* We can't spawn a thread and thus connect to monitor. Kill qemu. + * It's safe to call qemuProcessStop without a job here since there + * is no thread that could be doing anything else with the same domain + * object. + */ + qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, + QEMU_ASYNC_JOB_NONE, 0); + qemuDomainRemoveInactive(src->driver, obj); + + virDomainObjEndAPI(&obj); + virObjectUnref(data->conn); + VIR_FREE(data); + return -1; + } + + return 0; +} + +/** + * qemuProcessReconnectAll + * + * Try to re-open the resources for live VMs that we care + * about. + */ +void +qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver) +{ + struct qemuProcessReconnectData data = {.conn = conn, .driver = driver}; + virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data); +} -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:52PM +0200, Jiri Denemark wrote:
qemuProcessReconnect will need to call additional functions which were originally defined further in qemu_process.c.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 645 ++++++++++++++++++++++++------------------------ 1 file changed, 323 insertions(+), 322 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
When libvirt starts a new QEMU domain, it replaces host-model CPUs with the appropriate custom CPU definition. However, when reconnecting to a domain started by older libvirt (< 2.3), the domain would still have a host-model CPU in its active definition. https://bugzilla.redhat.com/show_bug.cgi?id=1463957 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2339de41c..0193c3591 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3756,6 +3756,30 @@ qemuProcessUpdateAndVerifyCPU(virQEMUDriverPtr driver, static int +qemuProcessUpdateCPU(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + virCPUDataPtr cpu = NULL; + virCPUDataPtr disabled = NULL; + int ret = -1; + + if (qemuProcessFetchGuestCPU(driver, vm, asyncJob, &cpu, &disabled) < 0) + goto cleanup; + + if (qemuProcessUpdateLiveGuestCPU(vm, cpu, disabled) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virCPUDataFree(cpu); + virCPUDataFree(disabled); + return ret; +} + + +static int qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm) { @@ -6850,6 +6874,30 @@ qemuProcessReconnect(void *opaque) ignore_value(qemuSecurityCheckAllLabel(driver->securityManager, obj->def)); + /* If the domain with a host-model CPU was started by an old libvirt + * (< 2.3) which didn't replace the CPU with a custom one, let's do it now + * since the rest of our code does not really expect a host-model CPU in a + * running domain. + */ + if (virQEMUCapsGuestIsNative(caps->host.arch, obj->def->os.arch) && + caps->host.cpu && + obj->def->cpu && + obj->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) { + virCPUDefPtr host; + + if (!(host = virCPUCopyMigratable(caps->host.cpu->arch, caps->host.cpu))) + goto error; + + if (virCPUUpdate(obj->def->os.arch, obj->def->cpu, host) < 0) { + virCPUDefFree(host); + goto error; + } + virCPUDefFree(host); + + if (qemuProcessUpdateCPU(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) + goto error; + } + if (qemuDomainRefreshVcpuInfo(driver, obj, QEMU_ASYNC_JOB_NONE, true) < 0) goto error; -- 2.13.2
On Wed, Jul 12, 2017 at 02:56:53PM +0200, Jiri Denemark wrote:
When libvirt starts a new QEMU domain, it replaces host-model CPUs with the appropriate custom CPU definition. However, when reconnecting to a domain started by older libvirt (< 2.3), the domain would still have a host-model CPU in its active definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1463957
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Wed, Jul 12, 2017 at 16:42:36 +0200, Pavel Hrdina wrote:
On Wed, Jul 12, 2017 at 02:56:53PM +0200, Jiri Denemark wrote:
When libvirt starts a new QEMU domain, it replaces host-model CPUs with the appropriate custom CPU definition. However, when reconnecting to a domain started by older libvirt (< 2.3), the domain would still have a host-model CPU in its active definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1463957
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_process.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Thanks for the review. Pushed. Jirka
participants (2)
-
Jiri Denemark -
Pavel Hrdina