[PATCH v2 6/9] qemu: tpm: Require UNDEFINE_TPM to be set to remove TPM state

[Stefan Berger]

On 10/6/22 09:26, Michal Prívozník wrote:
> On 10/5/22 16:02, Stefan Berger wrote:
>> When migrating the TPM in a setup that has shared storage for the TPM state
>> files setup between hosts we never remove the state.
>>
>> Signed-off-by: Stefan Berger <stefanb at linux.ibm.com>
>> ---
>>   src/qemu/qemu_tpm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> index 2b2d2eba5a..59de13061a 100644
>> --- a/src/qemu/qemu_tpm.c
>> +++ b/src/qemu/qemu_tpm.c
>> @@ -737,6 +737,10 @@ static void
>>   qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm,
>>                              virDomainUndefineFlagsValues flags)
>>   {
>> +    /* Never remove the state in case of migration with shared storage. */
>> +    if ((flags & VIR_MIGRATE_TPM_SHARED_STORAGE))
>> +        return;
> 
> This is testing a flag from a different enum. If there's ever an

Uuuh, right.

If we had a function that we could always call to determine whether we migrate across shared storage then any access to VIR_MIGRATE_TPM_SHARED_STORAGE could be replaced with a call to this function.

> undefine flag like:
> 
>    VIR_DOMAIN_UNDEFINE_EXAMPLE = (1<<21)
> 
> then this is going to be wrongly evaluated. Can't callers just pass
> VIR_DOMAIN_UNDEFINE_KEEP_TPM?


We have this here in this function.

     /*
      * remove TPM state if:
      * - persistent_state flag is set and the UNDEFINE_TPM flag is set
      * - persistent_state flag is not set and the KEEP_TPM flag is not set
      */
     if ((tpm->data.emulator.persistent_state && (flags & VIR_DOMAIN_UNDEFINE_TPM)) ||
         (!tpm->data.emulator.persistent_state && !(flags & VIR_DOMAIN_UNDEFINE_KEEP_TPM))) {
         qemuTPMEmulatorDeleteStorage(tpm);
     }


> 
> Alternatively, if we invent private data (see my comment to one of
> previous patches), this can be plain:
> 
>    if (QEMU_DOMAIN_TPM_PRIVATE(tpm)->migrating)
>      return;

Iirc you responded to me asking for being able to store info whether the currently running version of swtpm is able to handle shared storage migration (due to the additional flags for the release of the lock) and you suggested that boolean could be stored there but this boolean is NOT an indicator for whether shared storage is actually set up but whether it could handle shared storage migration if it had to.

--> QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.shared_storage_migration

    stefan

> 
> (or whatever member I suggested).
> 
> Michal
>