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

[Stefan Berger]

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 at linux.ibm.com>
---
  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