Pass a parameter indicating the reason for the removal of a domain so that the TPM driver can determine whether to delete the TPM state directory structure. It may only do this when a domain is undefined as part of a command like 'virsh undefine' and it must not do this when a VM is migrated across a shared storage setup for the TPM and it is removed from the source host. Therefore, the call locations that correspond to a 'virsh undefine' pass the value 'QEMU_DOMAIN_UNDEFINE_DOMAIN' while all other ones pass the unspecific value 'QEMU_DOMAIN_UNDEFINE_UNSPEC'. Signed-off-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; --- src/qemu/qemu_domain.c | 12 +++++++----- src/qemu/qemu_domain.h | 8 +++++++- src/qemu/qemu_driver.c | 20 ++++++++++---------- src/qemu/qemu_extdevice.c | 5 +++-- src/qemu/qemu_extdevice.h | 3 ++- src/qemu/qemu_migration.c | 13 +++++++------ src/qemu/qemu_process.c | 4 ++-- src/qemu/qemu_snapshot.c | 4 ++-- src/qemu/qemu_tpm.c | 9 ++++++--- src/qemu/qemu_tpm.h | 3 ++- 10 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 45f00e162d..ac8ab142b5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7143,7 +7143,8 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, static void qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + qemuDomainUndefineReason undefReason) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *snapDir = NULL; @@ -7169,7 +7170,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, if (rmdir(chkDir) < 0 && errno != ENOENT) VIR_WARN("unable to remove checkpoint directory %s", chkDir); } - qemuExtDevicesCleanupHost(driver, vm->def); + qemuExtDevicesCleanupHost(driver, vm->def, undefReason); } @@ -7180,14 +7181,15 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, */ void qemuDomainRemoveInactive(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + qemuDomainUndefineReason undefReason) { if (vm->persistent) { /* Short-circuit, we don't want to remove a persistent domain */ return; } - qemuDomainRemoveInactiveCommon(driver, vm); + qemuDomainRemoveInactiveCommon(driver, vm, undefReason); virDomainObjListRemove(driver->domains, vm); } @@ -7209,7 +7211,7 @@ qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, return; } - qemuDomainRemoveInactiveCommon(driver, vm); + qemuDomainRemoveInactiveCommon(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); virDomainObjListRemoveLocked(driver->domains, vm); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 592ee9805b..8e5dacf624 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -682,8 +682,14 @@ int qemuDomainMomentDiscardAll(void *payload, int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, virDomainObj *vm); +typedef enum { + QEMU_DOMAIN_UNDEFINE_UNSPEC = 0, + QEMU_DOMAIN_UNDEFINE_DOMAIN, /* virsh undefine type of command */ +} qemuDomainUndefineReason; + void qemuDomainRemoveInactive(virQEMUDriver *driver, - virDomainObj *vm); + virDomainObj *vm, + qemuDomainUndefineReason undefReason); void qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 637a748c85..fe5bbe2216 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1624,7 +1624,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, goto cleanup; if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); goto cleanup; } @@ -1633,7 +1633,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); qemuProcessEndJob(vm); goto cleanup; } @@ -2119,7 +2119,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: if (ret == 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_DOMAIN); qemuDomainObjEndJob(vm); cleanup: @@ -2738,7 +2738,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, } qemuDomainObjEndAsyncJob(vm); if (ret == 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); cleanup: virQEMUSaveDataFree(data); @@ -3285,7 +3285,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, qemuDomainObjEndAsyncJob(vm); if (ret == 0 && flags & VIR_DUMP_CRASH) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); cleanup: virDomainObjEndAPI(&vm); @@ -3597,7 +3597,7 @@ processGuestPanicEvent(virQEMUDriver *driver, endjob: qemuDomainObjEndAsyncJob(vm); if (removeInactive) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } @@ -4075,7 +4075,7 @@ processMonitorEOFEvent(virQEMUDriver *driver, virObjectEventStateQueue(driver->domainEventState, event); endjob: - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); qemuDomainObjEndJob(vm); } @@ -6005,7 +6005,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virFileWrapperFdFree(wrapperFd); virQEMUSaveDataFree(data); if (vm && ret < 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); virDomainObjEndAPI(&vm); return ret; } @@ -6696,7 +6696,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } } @@ -6836,7 +6836,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, */ vm->persistent = 0; if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_DOMAIN); ret = 0; endjob: diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index b8e3c1000a..b5bfd6cfd2 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -151,7 +151,8 @@ qemuExtDevicesPrepareHost(virQEMUDriver *driver, void qemuExtDevicesCleanupHost(virQEMUDriver *driver, - virDomainDef *def) + virDomainDef *def, + qemuDomainUndefineReason undefReason) { size_t i; @@ -159,7 +160,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver, return; for (i = 0; i < def->ntpms; i++) { - qemuExtTPMCleanupHost(def->tpms[i]); + qemuExtTPMCleanupHost(def->tpms[i], undefReason); } } diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h index 43d2a4dfff..252238c47f 100644 --- a/src/qemu/qemu_extdevice.h +++ b/src/qemu/qemu_extdevice.h @@ -41,7 +41,8 @@ int qemuExtDevicesPrepareHost(virQEMUDriver *driver, G_GNUC_WARN_UNUSED_RESULT; void qemuExtDevicesCleanupHost(virQEMUDriver *driver, - virDomainDef *def) + virDomainDef *def, + qemuDomainUndefineReason undefReason) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtDevicesStart(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b3b25d78b4..b15c1ccc22 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3408,7 +3408,7 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, * and there is no 'goto cleanup;' in the middle of those */ VIR_FREE(priv->origname); virDomainObjRemoveTransientDef(vm); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } virDomainObjEndAPI(&vm); virErrorRestore(&origErr); @@ -4054,7 +4054,8 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, + QEMU_DOMAIN_UNDEFINE_UNSPEC); } cleanup: @@ -6003,7 +6004,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } virErrorRestore(&orig_err); @@ -6130,7 +6131,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); return ret; } @@ -6667,7 +6668,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); virErrorRestore(&orig_err); return NULL; @@ -6805,7 +6806,7 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver, qemuMigrationJobFinish(vm); if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5c8413a6b6..cd038195d3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8460,7 +8460,7 @@ qemuProcessAutoDestroy(virDomainObj *dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - qemuDomainRemoveInactive(driver, dom); + qemuDomainRemoveInactive(driver, dom, QEMU_DOMAIN_UNDEFINE_UNSPEC); qemuDomainObjEndJob(dom); @@ -8926,7 +8926,7 @@ qemuProcessReconnect(void *opaque) if (jobStarted) qemuDomainObjEndJob(obj); if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactive(driver, obj); + qemuDomainRemoveInactive(driver, obj, QEMU_DOMAIN_UNDEFINE_UNSPEC); virDomainObjEndAPI(&obj); virIdentitySetCurrent(NULL); return; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6033deafed..2a4d2a9d9e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2103,7 +2103,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, } if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); return -1; } @@ -2125,7 +2125,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); return -1; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 584c787b70..d2ae3b9824 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -701,11 +701,13 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, /** * qemuTPMEmulatorCleanupHost: * @tpm: TPM definition + * @undefReason: The reason why the domain is removed * * Clean up persistent storage for the swtpm. */ static void -qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm) +qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, + qemuDomainUndefineReason undefReason G_GNUC_UNUSED) { if (!tpm->data.emulator.persistent_state) qemuTPMEmulatorDeleteStorage(tpm); @@ -1003,9 +1005,10 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver, void -qemuExtTPMCleanupHost(virDomainTPMDef *tpm) +qemuExtTPMCleanupHost(virDomainTPMDef *tpm, + qemuDomainUndefineReason undefReason) { - qemuTPMEmulatorCleanupHost(tpm); + qemuTPMEmulatorCleanupHost(tpm, undefReason); } diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index 9951f025a6..37c8e080d7 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -35,7 +35,8 @@ int qemuExtTPMPrepareHost(virQEMUDriver *driver, ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -void qemuExtTPMCleanupHost(virDomainTPMDef *tpm) +void qemuExtTPMCleanupHost(virDomainTPMDef *tpm, + qemuDomainUndefineReason undefReason) ATTRIBUTE_NONNULL(1); int qemuExtTPMStart(virQEMUDriver *driver, -- 2.37.1

Add a shared_storage field to the emulator part of the virDomainTPMDef used for indicating whether shared storage for TPM state is setup between hosts. Do not create storage if shared_storage flag is set and there's an incoming migration since the storage directory in this case must already exist. As a consequence also do not run swtpm_setup in this case. Signed-off-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; --- src/conf/domain_conf.h | 1 + src/qemu/qemu_tpm.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a1f6cf7a6f..29dc17a299 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1459,6 +1459,7 @@ struct _virDomainTPMDef { unsigned char secretuuid[VIR_UUID_BUFLEN]; bool hassecretuuid; bool persistent_state; + bool shared_storage; virBitmap *activePcrBanks; } emulator; } data; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d2ae3b9824..280307a14e 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -562,11 +562,19 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, int pwdfile_fd = -1; int migpwdfile_fd = -1; const unsigned char *secretuuid = NULL; + bool create_storage = true; if (!swtpm) return NULL; - if (qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0) + /* Do not create storage and run swtpm_setup on incoming migration over + * shared storage + */ + if (incomingMigration && tpm->data.emulator.shared_storage) + create_storage = false; + + if (create_storage && + qemuTPMEmulatorCreateStorage(tpm, &created, swtpm_user, swtpm_group) < 0) return NULL; if (tpm->data.emulator.hassecretuuid) -- 2.37.1

When using shared storage there is no need to apply security labels on the storage since the files have to have been labeled already on the source side and we must assume that the source and destination side have been setup to use the same uid and gid for running swtpm as well as share the same security labels. whether the security labels can be used at all depends on the shared storage and whether and how it supports them. Signed-off-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; --- src/qemu/qemu_tpm.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 8b3ef4e34e..20c7e92766 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -924,10 +924,19 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); - if (qemuSecurityStartTPMEmulator(driver, vm, cmd, - cfg->swtpm_user, cfg->swtpm_group, - NULL, &cmdret) < 0) - return -1; + if (incomingMigration && + tpm->data.emulator.shared_storage) { + /* security labels must have been set up on source already */ + if (qemuSecurityCommandRun(driver, vm, cmd, + cfg->swtpm_user, cfg->swtpm_group, + NULL, &cmdret) < 0) { + goto error; + } + } else if (qemuSecurityStartTPMEmulator(driver, vm, cmd, + cfg->swtpm_user, cfg->swtpm_group, + NULL, &cmdret) < 0) { + goto error; + } if (cmdret < 0) { /* virCommandRun() hidden in qemuSecurityStartTPMEmulator() -- 2.37.1

On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote: I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag VIR_DOMAIN_UNDEFINE_KEEP_TPM and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Aug 22, 2022 at 03:17:34PM -0400, Stefan Berger wrote: 'persistent_state' is something that applies to transient VMs. What I'm suggesting is that we should have some control over this for persistent VMs, when calling 'undefine'. ie virsh undefine --keep-nvram --keep-tpm $VMNAME if we have this feature in the public API, then its impl should support both this feature and your future patch for 'persistent_state'. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|