Re: [PATCH v2 6/9] qemu: tpm: Require UNDEFINE_TPM to be set to remove TPM state
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(a)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