1:34 p.m.
https://issues.redhat.com/browse/RHEL-80155
Martin Kletzander (3):
qemu_tpm: Rename qemuTPMHasSharedStorage ->
qemuTPMDomainHasSharedStorage
qemu_tpm: Extract per-TPM functionality from
qemuTPMDomainHasSharedStorage
qemu_tpm: Only warn about missing locking feature on shared
filesystems
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_tpm.c | 75 +++++++++++++++++++++++----------------
src/qemu/qemu_tpm.h | 4 +--
3 files changed, 47 insertions(+), 34 deletions(-)
--
2.50.1
1:34 p.m.
New subject: [PATCH 1/3] qemu_tpm: Rename qemuTPMHasSharedStorage -> qemuTPMDomainHasSharedStorage
From: Martin Kletzander <mkletzan(a)redhat.com>
The function deals with the whole domain and the part that handles one
TPM will be useful elsewhere and hence extracted later. This rename
makes it possible for the new function to use the original name of this
renamed one.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_tpm.c | 8 ++++----
src/qemu/qemu_tpm.h | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index c8974dbc5b97..b22248e3b92f 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1721,7 +1721,7 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm,
}
}
- if (qemuTPMHasSharedStorage(driver, vm->def) &&
+ if (qemuTPMDomainHasSharedStorage(driver, vm->def) &&
!qemuTPMCanMigrateSharedStorage(vm->def)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("the running swtpm does not support migration with
shared storage"));
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index b2f76e6b8b31..8c104ab1b303 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -1150,7 +1150,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virCommandSetPidFile(cmd, pidfile);
virCommandSetErrorFD(cmd, &errfd);
- if (incomingMigration && qemuTPMHasSharedStorage(driver, vm->def)) {
+ if (incomingMigration && qemuTPMDomainHasSharedStorage(driver, vm->def))
{
/* If the TPM is being migrated over shared storage, we can't
* lock all files before labeling them: the source swtpm
* process is still holding on to the lock file, and it will
@@ -1219,8 +1219,8 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
bool
-qemuTPMHasSharedStorage(virQEMUDriver *driver,
- virDomainDef *def)
+qemuTPMDomainHasSharedStorage(virQEMUDriver *driver,
+ virDomainDef *def)
{
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
size_t i;
@@ -1346,7 +1346,7 @@ qemuExtTPMStop(virQEMUDriver *driver,
return;
qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
- if (migration && qemuTPMHasSharedStorage(driver, vm->def))
+ if (migration && qemuTPMDomainHasSharedStorage(driver, vm->def))
restoreTPMStateLabel = false;
if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel, false) < 0)
diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h
index f0f16392a165..2d633fe36b84 100644
--- a/src/qemu/qemu_tpm.h
+++ b/src/qemu/qemu_tpm.h
@@ -61,8 +61,8 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
G_GNUC_WARN_UNUSED_RESULT;
-bool qemuTPMHasSharedStorage(virQEMUDriver *driver,
- virDomainDef *def)
+bool qemuTPMDomainHasSharedStorage(virQEMUDriver *driver,
+ virDomainDef *def)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
G_GNUC_WARN_UNUSED_RESULT;
--
2.50.1
1:34 p.m.
New subject: [PATCH 2/3] qemu_tpm: Extract per-TPM functionality from qemuTPMDomainHasSharedStorage
From: Martin Kletzander <mkletzan(a)redhat.com>
This way we can do the check for a particular TPM also elsewhere in the
code, especially in places where we're dealing with only one TPM. The
semantics is changed a little bit in a way that the function will check
all the TPMs as opposed to stopping on the first one which is of the
emulator type, but since a domain can currently only have one of these
it was not an issue.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_tpm.c | 59 ++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 8c104ab1b303..855d732e60d0 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -205,6 +205,40 @@ qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm,
}
+static bool
+qemuTPMHasSharedStorage(const virQEMUDriverConfig *cfg,
+ const virDomainTPMDef *tpm)
+{
+ switch (tpm->type) {
+ case VIR_DOMAIN_TPM_TYPE_EMULATOR:
+ return virFileIsSharedFS(tpm->data.emulator.source_path,
+ cfg->sharedFilesystems) == 1;
+ case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
+ case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
+ case VIR_DOMAIN_TPM_TYPE_LAST:
+ break;
+ }
+
+ return false;
+}
+
+
+bool
+qemuTPMDomainHasSharedStorage(virQEMUDriver *driver,
+ virDomainDef *def)
+{
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+ size_t i;
+
+ for (i = 0; i < def->ntpms; i++) {
+ if (qemuTPMHasSharedStorage(cfg, def->tpms[i]))
+ return true;
+ }
+
+ return false;
+}
+
+
/**
* qemuTPMEmulatorDeleteStorage:
* @tpm: TPM definition
@@ -1218,31 +1252,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
}
-bool
-qemuTPMDomainHasSharedStorage(virQEMUDriver *driver,
- virDomainDef *def)
-{
- g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- 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.source_path,
- cfg->sharedFilesystems) == 1;
- case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
- case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
- case VIR_DOMAIN_TPM_TYPE_LAST:
- break;
- }
- }
-
- return false;
-}
-
-
bool
qemuTPMCanMigrateSharedStorage(virDomainDef *def)
{
--
2.50.1
1:34 p.m.
New subject: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on shared filesystems
From: Martin Kletzander <mkletzan(a)redhat.com>
The warning pollutes the logs and might give a bad impression on someone
reading them even though the locking is not always needed. This way we
at least limit the logging in unnecessary cases.
Resolves: https://issues.redhat.com/browse/RHEL-80155
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_tpm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 855d732e60d0..cdbd6e3993b2 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
static void
qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd,
- const virDomainTPMEmulatorDef *emulator)
+ const virDomainTPMEmulatorDef *emulator,
+ const virDomainTPMDef *tpmDef,
+ const virQEMUDriverConfig *cfg)
{
const char *lock = ",lock";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) {
- VIR_WARN("This swtpm version doesn't support explicit locking");
+ if (qemuTPMHasSharedStorage(cfg, tpmDef))
+ VIR_WARN("This swtpm version doesn't support explicit
locking");
+
lock = "";
}
@@ -721,7 +725,7 @@ qemuTPMEmulatorUpdateProfileName(virDomainTPMEmulatorDef *emulator,
virCommandAddArgList(cmd, "socket", "--print-info",
"0x20", "--tpm2", NULL);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator);
+ qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator, persistentTPMDef, cfg);
if (qemuTPMVirCommandSwtpmAddEncryption(cmd, emulator, swtpm) < 0)
return -1;
@@ -848,7 +852,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600",
tpm->data.emulator.source->data.nix.path);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator);
+ qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator, persistentTPMDef,
cfg);
virCommandAddArg(cmd, "--log");
if (tpm->data.emulator.debug != 0)
--
2.50.1
1:57 p.m.
New subject: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on
shared filesystems
On a Thursday in 2025, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan(a)redhat.com>
The warning pollutes the logs and might give a bad impression on someone
reading them even though the locking is not always needed. This way we
at least limit the logging in unnecessary cases.
Resolves: https://issues.redhat.com/browse/RHEL-80155
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_tpm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 855d732e60d0..cdbd6e3993b2 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
static void
qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd,
- const virDomainTPMEmulatorDef *emulator)
+ const virDomainTPMEmulatorDef *emulator,
+ const virDomainTPMDef *tpmDef,
+ const virQEMUDriverConfig *cfg)
Interesting decision that we need to pass the QEMU driver config just to check
for an error...
{
const char *lock = ",lock";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) {
... but nothing to get the capability.
Jano
- VIR_WARN("This swtpm version doesn't support
explicit locking");
+ if (qemuTPMHasSharedStorage(cfg, tpmDef))
+ VIR_WARN("This swtpm version doesn't support explicit
locking");
+
lock = "";
}
12:03 p.m.
New subject: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on
shared filesystems
On Thu, Jul 17, 2025 at 12:34:43PM +0200, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan(a)redhat.com>
The warning pollutes the logs and might give a bad impression on someone
reading them even though the locking is not always needed. This way we
at least limit the logging in unnecessary cases.
Resolves: https://issues.redhat.com/browse/RHEL-80155
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_tpm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 855d732e60d0..cdbd6e3993b2 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
static void
qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd,
- const virDomainTPMEmulatorDef *emulator)
+ const virDomainTPMEmulatorDef *emulator,
+ const virDomainTPMDef *tpmDef,
+ const virQEMUDriverConfig *cfg)
{
const char *lock = ",lock";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) {
- VIR_WARN("This swtpm version doesn't support explicit locking");
+ if (qemuTPMHasSharedStorage(cfg, tpmDef))
+ VIR_WARN("This swtpm version doesn't support explicit
locking");
+
lock = "";
}
@@ -721,7 +725,7 @@ qemuTPMEmulatorUpdateProfileName(virDomainTPMEmulatorDef *emulator,
virCommandAddArgList(cmd, "socket", "--print-info",
"0x20", "--tpm2", NULL);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator);
+ qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator, persistentTPMDef, cfg);
if (qemuTPMVirCommandSwtpmAddEncryption(cmd, emulator, swtpm) < 0)
return -1;
@@ -848,7 +852,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600",
tpm->data.emulator.source->data.nix.path);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator);
+ qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator, persistentTPMDef,
cfg);
Coverity complains here that the `persistentTPMDef` can be NULL and that
it is dereferenced in `qemuTPMHasSharedStorage`.
This is called from `qemuExtDevicesStart` where the for loop calling
`qemuExtTPMStart` can actually pass NULL.
Not sure if it can happen with any real VM.
Pavel
virCommandAddArg(cmd, "--log");
if (tpm->data.emulator.debug != 0)
--
2.50.1
4:06 p.m.
New subject: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on
shared filesystems
On Fri, Jul 18, 2025 at 11:03:56AM +0200, Pavel Hrdina wrote:
On Thu, Jul 17, 2025 at 12:34:43PM +0200, Martin Kletzander via Devel
wrote:
> From: Martin Kletzander <mkletzan(a)redhat.com>
>
> The warning pollutes the logs and might give a bad impression on someone
> reading them even though the locking is not always needed. This way we
> at least limit the logging in unnecessary cases.
>
> Resolves: https://issues.redhat.com/browse/RHEL-80155
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_tpm.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 855d732e60d0..cdbd6e3993b2 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
>
> static void
> qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd,
> - const virDomainTPMEmulatorDef *emulator)
> + const virDomainTPMEmulatorDef *emulator,
> + const virDomainTPMDef *tpmDef,
> + const virQEMUDriverConfig *cfg)
> {
> const char *lock = ",lock";
>
> if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) {
> - VIR_WARN("This swtpm version doesn't support explicit
locking");
> + if (qemuTPMHasSharedStorage(cfg, tpmDef))
> + VIR_WARN("This swtpm version doesn't support explicit
locking");
> +
> lock = "";
> }
>
> @@ -721,7 +725,7 @@ qemuTPMEmulatorUpdateProfileName(virDomainTPMEmulatorDef
*emulator,
>
> virCommandAddArgList(cmd, "socket", "--print-info",
"0x20", "--tpm2", NULL);
>
> - qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator);
> + qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator, persistentTPMDef, cfg);
>
> if (qemuTPMVirCommandSwtpmAddEncryption(cmd, emulator, swtpm) < 0)
> return -1;
> @@ -848,7 +852,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
> virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600",
> tpm->data.emulator.source->data.nix.path);
>
> - qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator);
> + qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator,
persistentTPMDef, cfg);
Coverity complains here that the `persistentTPMDef` can be NULL and that
it is dereferenced in `qemuTPMHasSharedStorage`.
This is called from `qemuExtDevicesStart` where the for loop calling
`qemuExtTPMStart` can actually pass NULL.
Not sure if it can happen with any real VM.
It can, yes. I have a fix ready. Thanks for catching that.
Pavel
>
> virCommandAddArg(cmd, "--log");
> if (tpm->data.emulator.debug != 0)
> --
> 2.50.1
>
1:46 p.m.
New subject: [PATCH 0/3] qemu_tpm: Do not pollute logs with unnecessary
warning
On Thu, Jul 17, 2025 at 12:34:40 +0200, Martin Kletzander via Devel wrote:
https://issues.redhat.com/browse/RHEL-80155
Martin Kletzander (3):
qemu_tpm: Rename qemuTPMHasSharedStorage ->
qemuTPMDomainHasSharedStorage
qemu_tpm: Extract per-TPM functionality from
qemuTPMDomainHasSharedStorage
qemu_tpm: Only warn about missing locking feature on shared
filesystems
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_tpm.c | 75 +++++++++++++++++++++++----------------
src/qemu/qemu_tpm.h | 4 +--
3 files changed, 47 insertions(+), 34 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
1:55 p.m.
New subject: [PATCH 0/3] qemu_tpm: Do not pollute logs with unnecessary
warning
On a Thursday in 2025, Martin Kletzander via Devel wrote:
https://issues.redhat.com/browse/RHEL-80155
Martin Kletzander (3):
qemu_tpm: Rename qemuTPMHasSharedStorage ->
qemuTPMDomainHasSharedStorage
qemu_tpm: Extract per-TPM functionality from
qemuTPMDomainHasSharedStorage
qemu_tpm: Only warn about missing locking feature on shared
filesystems
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_tpm.c | 75 +++++++++++++++++++++++----------------
src/qemu/qemu_tpm.h | 4 +--
3 files changed, 47 insertions(+), 34 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
0
days inactive
1
days old
8 comments
4 participants
participants (4)
-
Ján Tomko -
Martin Kletzander -
Pavel Hrdina -
Peter Krempa