Re: [PATCH 6/7] qemu: tpm: Remove TPM state files and directory only when undefining a VM

On 8/22/22 12:46, Daniel P. Berrangé wrote: > On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote: > > When share storage for the TPM state files has been setup betwen hosts then > > remove the TPM state files and directory only when undefining a VM and only > > if the attribute persistent_state is not set. Avoid removing the TPM state > > files and directory structure when a VM is migrated and shared storage is > > used since this would also remove those files and directory structure on > > the destination side. > > 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. I think the granularity encoded in the reason is necessary for the following patch I was going to post later on: Subject: [PATCH] qemu: tpm: Remove TPM state with non-shared storage This patch 'fixes' the behavior of the persistent_state TPM domain XML attribute that intends to preserve the state of the TPM but should not keep the state around on all the hosts a VM has been migrated to. It removes the TPM state directory structure from the source host upon successful migration when non-shared storage is used. Similarly, it removes it from the destination host upon migration failure when non-shared storage is used.

Signed-off-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; --- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_tpm.c | 3 +++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8e5dacf624..4cf6a640c2 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -685,6 +685,8 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, typedef enum { QEMU_DOMAIN_UNDEFINE_UNSPEC = 0, QEMU_DOMAIN_UNDEFINE_DOMAIN, /* virsh undefine type of command */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC, /* undefine due to successful migration on src */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_DST, /* undefine due to failed migration on dst */ } qemuDomainUndefineReason; void qemuDomainRemoveInactive(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b15c1ccc22..f50a488072 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4055,7 +4055,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, vm->persistent = 0; } qemuDomainRemoveInactive(driver, vm, - QEMU_DOMAIN_UNDEFINE_UNSPEC); + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC); } cleanup: @@ -6668,7 +6668,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_MIGRATION_DST); virErrorRestore(&orig_err); return NULL; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d1639318e7..ad9bc9f1a5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -739,6 +739,9 @@ qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } + } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || + undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { + qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } -- 2.37.1 The complete qemuTPMEmulatorCleanupHost handling shared and non-shared storage along with the persistent_state attribute now looks like this: static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, qemuDomainUndefineReason undefReason) { if (tpm->data.emulator.shared_storage) { /* When using shared storage remove the domain only if this is due to * a 'virsh undefine' type of command and only if persistent_state == * false. Avoid removal of the state files/directory during migration. */ if (undefReason == QEMU_DOMAIN_UNDEFINE_DOMAIN && !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } } Regards, Stefan