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

[Stefan Berger]

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