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

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.

> + virReportError(VIR_ERR_NO_SUPPORT, "%s", > + _("the running swtpm does not support migration with shared storage")); > + goto cleanup; > + } > + > if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { > ret = qemuMigrationSrcBeginResumePhase(conn, driver, vm, xmlin, > cookieout, cookieoutlen, flags); > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c > index a45ad599aa..7b0afe94ec 100644 > --- a/src/qemu/qemu_tpm.c > +++ b/src/qemu/qemu_tpm.c > @@ -557,6 +557,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, > int migpwdfile_fd = -1; > const unsigned char *secretuuid = NULL; > bool create_storage = true; > + bool on_shared_storage; > > if (!swtpm) > return NULL; > @@ -564,7 +565,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, > /* Do not create storage and run swtpm_setup on incoming migration over > * shared storage > */ > - if (incomingMigration && virFileIsSharedFS(tpm->data.emulator.storagepath)) > + on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath); > + if (incomingMigration && on_shared_storage) > create_storage = false; > > if (create_storage && > @@ -642,6 +644,31 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, > virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc", migpwdfile_fd); > } > > + /* If swtpm supports it and the TPM state is stored on shared storage, > + * start swtpm with --migration release-lock-outgoing so it can migrate > + * across shared storage if needed. > + */ > + QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage = false; If we don't introduce private data, then this line should be deleted. > + if (on_shared_storage && > + virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION)) { > + > + virCommandAddArg(cmd, "--migration"); > + virCommandAddArgFormat(cmd, "release-lock-outgoing%s", > + incomingMigration ? ",incoming": ""); > + QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage = true; > + } else { > + /* Report an error if there's an incoming migration across shared > + * storage and swtpm does not support the --migration option. > + */ > + if (incomingMigration && on_shared_storage) { > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, > + _("%s (on destination side) does not support the --migration option " > + "needed for migration with shared storage"), Don't hesitate to put whole error message onto one line. It's way easier to grep then.

> + swtpm); > + goto error; > + } > + } > + > return g_steal_pointer(&cmd); > > error: > @@ -962,6 +989,43 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, > } > > > +bool > +qemuTPMHasSharedStorage(virDomainDef *def) > +{ > + size_t i; > + > + for (i = 0; i < def->ntpms; i++) { > + virDomainTPMDef *tpm = def->tpms[i]; > + switch (tpm->type) { > + case VIR_DOMAIN_TPM_TYPE_EMULATOR: > + return virFileIsSharedFS(tpm->data.emulator.storagepath); > + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: > + case VIR_DOMAIN_TPM_TYPE_LAST: > + } > + } > + > + return false; > +} > + > + > +bool > +qemuTPMCanMigrateSharedStorage(virDomainDef *def) > +{ > + size_t i; > + > + for (i = 0; i < def->ntpms; i++) { > + virDomainTPMDef *tpm = def->tpms[i]; > + switch (tpm->type) { > + case VIR_DOMAIN_TPM_TYPE_EMULATOR: > + return QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage; If we don't introduce private data, then how about just: if (virFileIsSharedFS(tpm->data.emulator.storagepath) == 1 && !virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_MIGRATION)) return false;

> + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: > + case VIR_DOMAIN_TPM_TYPE_LAST: > + } > + } > + return true; > +} > + > + > /* --------------------- > * Module entry points > * --------------------- > diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h > index f068f3ca5a..9daa3e14df 100644 > --- a/src/qemu/qemu_tpm.h > +++ b/src/qemu/qemu_tpm.h > @@ -56,3 +56,9 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver, > virCgroup *cgroup) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > G_GNUC_WARN_UNUSED_RESULT; > + > +bool qemuTPMHasSharedStorage(virDomainDef *def) > + ATTRIBUTE_NONNULL(1); > + > +bool qemuTPMCanMigrateSharedStorage(virDomainDef *def) > + ATTRIBUTE_NONNULL(1); Michal