Re: [PATCH 1/2] qemu: Add UNDEFINE_TPM and UNDEFINE_KEEP_TPM flags

3 Oct 2022

On 10/3/22 11:20, Michal Prívozník wrote:
...
On 8/23/22 18:28, Stefan Berger wrote:
...
Add UNDEFINE_TPM and UNDEFINE_KEEP_TPM flags to qemuDomainUndefineFlags()
API and --tpm and --keep-tpm to 'virsh undefine'. Pass the
virDomainUndefineFlagsValues via qemuDomainRemoveInactive()
from qemuDomainUndefineFlags() all the way down to
qemuTPMEmulatorCleanupHost() and delete TPM storage there considering that
the UNDEFINE_TPM flag has priority over the persistent_state attribute
from the domain XML. Pass 0 in all other API call sites to
qemuDomainRemoveInactive() for now.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
  include/libvirt/libvirt-domain.h |  6 ++++++
  src/qemu/qemu_domain.c           | 12 +++++++-----
  src/qemu/qemu_domain.h           |  3 ++-
  src/qemu/qemu_driver.c           | 31 ++++++++++++++++++++-----------
  src/qemu/qemu_extdevice.c        |  5 +++--
  src/qemu/qemu_extdevice.h        |  3 ++-
  src/qemu/qemu_migration.c        | 12 ++++++------
  src/qemu/qemu_process.c          |  4 ++--
  src/qemu/qemu_snapshot.c         |  4 ++--
  src/qemu/qemu_tpm.c              | 14 ++++++++++----
  src/qemu/qemu_tpm.h              |  3 ++-
  tools/virsh-domain.c             | 15 +++++++++++++++
  12 files changed, 77 insertions(+), 35 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 7430a08619..5f12c673d6 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2267,9 +2267,15 @@ typedef enum {
      VIR_DOMAIN_UNDEFINE_CHECKPOINTS_METADATA = (1 << 4), /* If last use of domain,
                                                              then also remove any
                                                              checkpoint metadata (Since: 5.6.0) */
+    VIR_DOMAIN_UNDEFINE_TPM                = (1 << 5), /* Also remove any
+                                                          TPM state (Since: 8.8.0) */
+    VIR_DOMAIN_UNDEFINE_KEEP_TPM           = (1 << 6), /* Keep TPM state (Since: 8.8.0) */
+    VIR_DOMAIN_UNDEFINE_TPM_MASK           = (3 << 5), /* TPM flags mask (Since: 8.8.0) */
I believe this _MASK is not something we want to expose to users. It's
not like both _KEEP_TPM and _TPM can be passed at the same time.
I will remove it...
...
...
+    /* Future undefine control flags should come here. */
  } virDomainUndefineFlagsValues;
+
  int                     virDomainUndefineFlags   (virDomainPtr domain,
                                                    unsigned int flags);
  int                     virConnectNumOfDefinedDomains  (virConnectPtr conn);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d5fef76211..47eabd0eec 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,
+                               virDomainUndefineFlagsValues flags)
I'd rather use unsigned int for flags. In the end,
qemuDomainUndefineFlags() uses uint and passes it to
qemuDomainRemoveInactive() so there's not much value in keeping the type.
Should I rename flags to undefine_flags in this patch just so these flags are different from other sets of flags? To make them distinguishable was the primary reason to keep the type around.
...
...
qemuTPMEmulatorDeleteStorage(tpm);
+    } else if (!tpm->data.emulator.persistent_state &&
+             (flags & VIR_DOMAIN_UNDEFINE_TPM_MASK) == 0) {
+        qemuTPMEmulatorDeleteStorage(tpm);
Here, we should do something similar to NVRAM: fail if the storage
exists and KEEP_TPM wasn't specified. Which is going to break existing
workloads where undefine worked even without the flag. So maybe just
need this?
if ((tpm->data.emulator.persistent_state && flags &
VIR_DOMAIN_UNDEFINE_TPM) ||
         !tpm->data.emulator.persistent_state && !(flags &
VIR_DOMAIN_UNDEFINE_KEEP_TPM)) {
         qemuTPMEmulatorDeleteStorage(tpm);
     }
I'll have a look at this.
...
...
+    }
  }
@@ -993,9 +998,10 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver,
void
-qemuExtTPMCleanupHost(virDomainTPMDef *tpm)
+qemuExtTPMCleanupHost(virDomainTPMDef *tpm,
+                      virDomainUndefineFlagsValues flags)
  {
-    qemuTPMEmulatorCleanupHost(tpm);
+    qemuTPMEmulatorCleanupHost(tpm, flags);
  }
diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h
index 9951f025a6..f068f3ca5a 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,
+                           virDomainUndefineFlagsValues flags)
      ATTRIBUTE_NONNULL(1);
int qemuExtTPMStart(virQEMUDriver *driver,
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d2ea4d1c7b..ff9c081d71 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3650,6 +3650,14 @@ static const vshCmdOptDef opts_undefine[] = {
       .type = VSH_OT_BOOL,
       .help = N_("keep nvram file")
      },
+    {.name = "tpm",
+     .type = VSH_OT_BOOL,
+     .help = N_("remove TPM state")
+    },
+    {.name = "keep-tpm",
+     .type = VSH_OT_BOOL,
+     .help = N_("keep TPM state")
+    },
These new arguments should be documented in virsh manpage.
Yes, right.

   Stefan
...
Michal


    