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

[Daniel P. Berrangé]

On Mon, Aug 22, 2022 at 03:17:34PM -0400, Stefan Berger wrote:
> 
> 
> 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.

'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'. 

> 
> 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
> 

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 :|