Re: [PATCH v3 4/6] qemu: tpm: Pass --migration option to swtpm if supported and needed

On 10/21/22 13:36, Stefan Berger wrote: > > > On 10/21/22 06:55, Michal Prívozník wrote: >> On 10/18/22 19:04, Stefan Berger wrote: >>> Pass the --migration option to swtpm if swptm supports it (starting >>> with v0.8) and if the TPM's state is written on shared storage. If this >>> is the case apply the 'release-lock-outgoing' parameter with this >>> option and apply the 'incoming' parameter for incoming migration so that >>> swtpm releases the file lock on the source side when the state is >>> migrated >>> and locks the file on the destination side when the state is received. >>> >>> If a started swtpm instance is running with the necessary options of >>> migrating with share storage then remember this with a flag in the >>> virDomainTPMPrivateDef. >>> >>> Report an error if swtpm does not support the --migration option and an >>> incoming migration across shared storage is requested. >>> >>> Signed-off-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; >>> --- >>>   src/qemu/qemu_migration.c |  8 +++++ >>>   src/qemu/qemu_tpm.c       | 66 ++++++++++++++++++++++++++++++++++++++- >>>   src/qemu/qemu_tpm.h       |  6 ++++ >>>   3 files changed, 79 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c >>> index 33105cf07b..5b4f4615ee 100644 >>> --- a/src/qemu/qemu_migration.c >>> +++ b/src/qemu/qemu_migration.c >>> @@ -38,6 +38,7 @@ >>>   #include "qemu_security.h" >>>   #include "qemu_slirp.h" >>>   #include "qemu_block.h" >>> +#include "qemu_tpm.h" >>>     #include "domain_audit.h" >>>   #include "virlog.h" >>> @@ -2789,6 +2790,13 @@ qemuMigrationSrcBegin(virConnectPtr conn, >>>           goto cleanup; >>>       } >>>   +    if (qemuTPMHasSharedStorage(vm->def) && >>> +        !qemuTPMCanMigrateSharedStorage(vm->def)) { >> >> This works, but only because qemuValidateDomainDefTPMs() denies two >> emulated TPMs. I think we can call just qemuTPMCanMigrateSharedStorage() >> since it already iterates over all TPMs and checks each one individually. > > I am only starting swtpm with the necessary command line options for > shared storage migration if a) shared storage is detected at the time of > the start of the VM and b) if swtpm supports shared storage migration at > all. If it doesn't support it I let you start the VM but won't let you > migrate. > > > qemuTPMCanMigrateSharedStorage() checks whether the running swtpm > instance can do b). It doesn't check the swtpm executable that you may > have upgraded or downgraded in the meantime. That's why I need the > private data. > > So the check above tests whether there is shared storage detected for > the TPM_EMULATOR (swtpm; only one allowed) and if so whether the > *running swtpm instance* supports shared storage migration. I think it > is correct this way. Ah, good point. So let me fix the other small nits I've raised and merge these.