[PATCH] qemu: tpm: Account for possible migration without actually sharing storage
From: Peter Krempa <pkrempa@redhat.com> The current logic in 'qemuTPMEmulatorBuildCommand' skips all setup if the *location* of the data is on what we'd consider shared storage. This means that if the location is not actually shared (e.g. it's shared betweeh some other hosts than the two doing the migration) and the path wasn't ever used (e.g. by migrating out) from the host where we're migrating into the complete setup of the location would be skipped even when it doesn't exist. Fix the logic by skipping only some of the setup steps so that 'qemuTPMEmulatorCreateStorage' can still create the storage if it doesn't exist. The rest of the code then needs to take the 'created' flag returned from 'qemuTPMEmulatorCreateStorage' into account. Fixes: 68103e9daf633b789428fedef56f816c92f6ee75 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 4c9445d72c..1ce6390fd5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -158,6 +158,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, /** * qemuTPMEmulatorCreateStorage: * @tpm: TPM definition for an emulator type + * @sharedStorageMigration: VM is being migrated with possibly shared storage * @created: a pointer to a bool that will be set to true if the * storage was created because it did not exist yet * @swtpm_user: The uid that needs to be able to access the directory @@ -169,6 +170,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, */ static int qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, + bool sharedStorageMigration, bool *created, uid_t swtpm_user, gid_t swtpm_group) @@ -187,8 +189,14 @@ qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, *created = false; if (!virFileExists(source_path) || - virDirIsEmpty(source_path, true) > 0) + virDirIsEmpty(source_path, true) > 0) { *created = true; + } else { + /* If the location exists and is shared, we don't need to create it + * during migration */ + if (sharedStorageMigration) + return 0; + } if (virDirCreate(source_path, 0700, swtpm_user, swtpm_group, VIR_DIR_CREATE_ALLOW_EXIST) < 0) { @@ -809,16 +817,13 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, run_setup = true; } - /* Do not create storage and run swtpm_setup on incoming migration over - * shared storage - */ on_shared_storage = virFileIsSharedFS(tpm->data.emulator.source_path, cfg->sharedFilesystems) == 1; - if (incomingMigration && on_shared_storage) - create_storage = false; if (create_storage) { - if (qemuTPMEmulatorCreateStorage(tpm, &created, + if (qemuTPMEmulatorCreateStorage(tpm, + incomingMigration && on_shared_storage, + &created, cfg->swtpm_user, cfg->swtpm_group) < 0) return NULL; run_setup = created; @@ -885,6 +890,9 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, /* 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. + * + * Note that if 'created' is true, the location didn't exist so the storage + * is not actually shared. */ QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage = false; if (on_shared_storage && @@ -892,13 +900,13 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandAddArg(cmd, "--migration"); virCommandAddArgFormat(cmd, "release-lock-outgoing%s", - incomingMigration ? ",incoming": ""); + incomingMigration && !created ? ",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) { + if (incomingMigration && on_shared_storage && !created) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, _("%1$s (on destination side) does not support the --migration option needed for migration with shared storage"), swtpm); -- 2.52.0
On 12/1/25 15:25, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
The current logic in 'qemuTPMEmulatorBuildCommand' skips all setup if the *location* of the data is on what we'd consider shared storage.
This means that if the location is not actually shared (e.g. it's shared betweeh some other hosts than the two doing the migration) and the path wasn't ever used (e.g. by migrating out) from the host where we're migrating into the complete setup of the location would be skipped even when it doesn't exist.
Fix the logic by skipping only some of the setup steps so that 'qemuTPMEmulatorCreateStorage' can still create the storage if it doesn't exist.
The rest of the code then needs to take the 'created' flag returned from 'qemuTPMEmulatorCreateStorage' into account.
Fixes: 68103e9daf633b789428fedef56f816c92f6ee75 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 4c9445d72c..1ce6390fd5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -158,6 +158,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, /** * qemuTPMEmulatorCreateStorage: * @tpm: TPM definition for an emulator type + * @sharedStorageMigration: VM is being migrated with possibly shared storage * @created: a pointer to a bool that will be set to true if the * storage was created because it did not exist yet * @swtpm_user: The uid that needs to be able to access the directory @@ -169,6 +170,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, */ static int qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, + bool sharedStorageMigration, bool *created, uid_t swtpm_user, gid_t swtpm_group) @@ -187,8 +189,14 @@ qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, *created = false;
if (!virFileExists(source_path) || - virDirIsEmpty(source_path, true) > 0) + virDirIsEmpty(source_path, true) > 0) { *created = true; + } else { + /* If the location exists and is shared, we don't need to create it + * during migration */ + if (sharedStorageMigration)
Can you please put a short VIR_DEBUG("Skipping TPM creation due to shared storage migration"); here? Or something among those lines. I feel this is something we might find useful later when reading logs from a migration.
+ return 0; + }
if (virDirCreate(source_path, 0700, swtpm_user, swtpm_group, VIR_DIR_CREATE_ALLOW_EXIST) < 0) { @@ -809,16 +817,13 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, run_setup = true; }
- /* Do not create storage and run swtpm_setup on incoming migration over - * shared storage - */ on_shared_storage = virFileIsSharedFS(tpm->data.emulator.source_path, cfg->sharedFilesystems) == 1; - if (incomingMigration && on_shared_storage) - create_storage = false;
if (create_storage) { - if (qemuTPMEmulatorCreateStorage(tpm, &created, + if (qemuTPMEmulatorCreateStorage(tpm, + incomingMigration && on_shared_storage, + &created, cfg->swtpm_user, cfg->swtpm_group) < 0) return NULL; run_setup = created; @@ -885,6 +890,9 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, /* 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. + * + * Note that if 'created' is true, the location didn't exist so the storage + * is not actually shared. */ QEMU_DOMAIN_TPM_PRIVATE(tpm)->swtpm.can_migrate_shared_storage = false; if (on_shared_storage && @@ -892,13 +900,13 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
virCommandAddArg(cmd, "--migration"); virCommandAddArgFormat(cmd, "release-lock-outgoing%s", - incomingMigration ? ",incoming": ""); + incomingMigration && !created ? ",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) { + if (incomingMigration && on_shared_storage && !created) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, _("%1$s (on destination side) does not support the --migration option needed for migration with shared storage"), swtpm);
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Mon, Dec 08, 2025 at 11:26:20 +0100, Michal Prívozník wrote:
On 12/1/25 15:25, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
The current logic in 'qemuTPMEmulatorBuildCommand' skips all setup if the *location* of the data is on what we'd consider shared storage.
This means that if the location is not actually shared (e.g. it's shared betweeh some other hosts than the two doing the migration) and the path wasn't ever used (e.g. by migrating out) from the host where we're migrating into the complete setup of the location would be skipped even when it doesn't exist.
Fix the logic by skipping only some of the setup steps so that 'qemuTPMEmulatorCreateStorage' can still create the storage if it doesn't exist.
The rest of the code then needs to take the 'created' flag returned from 'qemuTPMEmulatorCreateStorage' into account.
Fixes: 68103e9daf633b789428fedef56f816c92f6ee75 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 4c9445d72c..1ce6390fd5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -158,6 +158,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, /** * qemuTPMEmulatorCreateStorage: * @tpm: TPM definition for an emulator type + * @sharedStorageMigration: VM is being migrated with possibly shared storage * @created: a pointer to a bool that will be set to true if the * storage was created because it did not exist yet * @swtpm_user: The uid that needs to be able to access the directory @@ -169,6 +170,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, */ static int qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, + bool sharedStorageMigration, bool *created, uid_t swtpm_user, gid_t swtpm_group) @@ -187,8 +189,14 @@ qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, *created = false;
if (!virFileExists(source_path) || - virDirIsEmpty(source_path, true) > 0) + virDirIsEmpty(source_path, true) > 0) { *created = true; + } else { + /* If the location exists and is shared, we don't need to create it + * during migration */ + if (sharedStorageMigration)
Can you please put a short VIR_DEBUG("Skipping TPM creation due to shared storage migration"); here? Or something among those lines. I feel this is something we might find useful later when reading logs from a migration.
You're right this might come in handy. How about: 'Skipping TPM storage creation. Path '%s' already exists and is on shared storage.' So that the specific case is known without grepping the code?
On 12/8/25 11:31, Peter Krempa wrote:
On Mon, Dec 08, 2025 at 11:26:20 +0100, Michal Prívozník wrote:
On 12/1/25 15:25, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
The current logic in 'qemuTPMEmulatorBuildCommand' skips all setup if the *location* of the data is on what we'd consider shared storage.
This means that if the location is not actually shared (e.g. it's shared betweeh some other hosts than the two doing the migration) and the path wasn't ever used (e.g. by migrating out) from the host where we're migrating into the complete setup of the location would be skipped even when it doesn't exist.
Fix the logic by skipping only some of the setup steps so that 'qemuTPMEmulatorCreateStorage' can still create the storage if it doesn't exist.
The rest of the code then needs to take the 'created' flag returned from 'qemuTPMEmulatorCreateStorage' into account.
Fixes: 68103e9daf633b789428fedef56f816c92f6ee75 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 4c9445d72c..1ce6390fd5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -158,6 +158,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, /** * qemuTPMEmulatorCreateStorage: * @tpm: TPM definition for an emulator type + * @sharedStorageMigration: VM is being migrated with possibly shared storage * @created: a pointer to a bool that will be set to true if the * storage was created because it did not exist yet * @swtpm_user: The uid that needs to be able to access the directory @@ -169,6 +170,7 @@ qemuTPMEmulatorGetPid(const char *swtpmStateDir, */ static int qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, + bool sharedStorageMigration, bool *created, uid_t swtpm_user, gid_t swtpm_group) @@ -187,8 +189,14 @@ qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, *created = false;
if (!virFileExists(source_path) || - virDirIsEmpty(source_path, true) > 0) + virDirIsEmpty(source_path, true) > 0) { *created = true; + } else { + /* If the location exists and is shared, we don't need to create it + * during migration */ + if (sharedStorageMigration)
Can you please put a short VIR_DEBUG("Skipping TPM creation due to shared storage migration"); here? Or something among those lines. I feel this is something we might find useful later when reading logs from a migration.
You're right this might come in handy. How about:
'Skipping TPM storage creation. Path '%s' already exists and is on shared storage.'
So that the specific case is known without grepping the code?
Sounds perfect! Michal
participants (2)
-
Michal Prívozník -
Peter Krempa