On 9/7/26 7:58 AM, Arun Menon via Devel wrote:
Hi Peter, Thank you for taking a look at this series.
On Mon, Sep 07, 2026 at 01:02:02PM +0200, Peter Krempa wrote:
On Mon, Sep 07, 2026 at 01:24:09 +0530, Arun Menon via Devel wrote:
From: Arun Menon <armenon@redhat.com>
Update logic for both creation and parsing the 2 new attributes backup and fsync introduced in swtpm.
Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 2 files changed, 33 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 34d3b00079..0326ad7c58 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11449,6 +11449,11 @@ virDomainSmartcardDefParseXML(virDomainXMLOption *xmlopt, * </backend> * </tpm> * + * Emulator state backup and fsync is supported with the following: + * <tpm model='tpm-crb'> + * <backend type='emulator' version='2.0' backup='yes' fsync='yes'> + * </tpm>
So usually the commit which implements the XML bits also adds docs. I'll borrow them from commit 3/4 here, because I wonder if these options even should exist.
(In fact I also think the RNG schema should be part of this commit instead of being separate)
I shall add all of that in a single commit. Thanks.
Specifically 'fsync' is documented as:
+``fsync`` + The ``fsync`` attribute indicates whether an fsync will be called on the + file and the directory of the file when the 'swtpm' TPM state is written + to disk. This option ensures that all data have been successfully written + to physical storage before the TPM processes the next command. It lowers + the probability of TPM state file corruption in case of a power loss. By + default the value is ``no``. This attribute only works with the directory + backend. The accepted values are ``yes`` and ``no``. :since:`Since 12.8.0`
And I wonder why would anyone ever want to select 'no' here based on this description. The 'yes' value seems to only hold advantages as documented.
I came across a discussion upstream in SWTPM, that said fsync is expensive and therefore it can cause a timeout while executing some TPM commands. I understand that it was fixed in libtpms, but swtpm lets user to set the option explicitly. Reference: https://github.com/stefanberger/swtpm/issues/1029#issuecomment-2886593075
I did not want to enable new features implicitly but let users decide on it.
Now with:
+``backup`` + The ``backup`` attribute indicates whether a backup of the 'swtpm' TPM + state is taken or not. This option can be used also for restoring + the state from the backup. By default the value is ``no``. + This attribute only works with the directory backend. The accepted values + are ``yes`` and ``no``. :since:`Since 12.8.0`
I'm a bit less sure, as the description is extremely vague. Anyways it still seems to be an option that IMO should be always enabled.
I agree, backup can be something that's always on. Its just that the backup option is exclusive for a TPM with a directory backend.
We can always add it explicitly during command construction, specifically to the directory backend code path.
Can you please elaborate why would anyone ever want to disable those?
+ * */ static virDomainTPMDef * virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, @@ -11465,6 +11470,8 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, g_autofree char *path = NULL; g_autofree char *secretuuid = NULL; g_autofree char *persistent_state = NULL; + g_autofree char *backup = NULL; + g_autofree char *fsync = NULL; g_autofree xmlNodePtr *backends = NULL; g_autofree xmlNodePtr *nodes = NULL; g_autofree char *type = NULL; @@ -11562,6 +11569,26 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, } }
+ backup = virXMLPropString(backends[0], "backup"); + if (backup) { + if (virStringParseYesNo(backup, + &def->data.emulator.backup) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid backup value, either 'yes' or 'no'")); + goto error; + } + }
Use virXMLPropTristateBool instead of open-coding it here. That also resolves a further complaint that VIR_ERR_INTERNAL_ERROR is inappropriate for user config errors.
See below on the question why not use 'bool' type.
+ + fsync = virXMLPropString(backends[0], "fsync"); + if (fsync) { + if (virStringParseYesNo(fsync, + &def->data.emulator.fsync) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid fsync value, either 'yes' or 'no'")); + goto error; + } + }
Same here.
+ if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) break; if (nnodes > 0) @@ -26662,6 +26689,10 @@ virDomainTPMDefFormat(virBuffer *buf, } if (def->data.emulator.persistent_state) virBufferAddLit(&backendAttrBuf, " persistent_state='yes'"); + if (def->data.emulator.backup) + virBufferAddLit(&backendAttrBuf, " backup='yes'"); + if (def->data.emulator.fsync) + virBufferAddLit(&backendAttrBuf, " fsync='yes'"); if (def->data.emulator.debug != 0) virBufferAsprintf(&backendAttrBuf, " debug='%u'", def->data.emulator.debug); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3732525af4..0d93487689 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1546,6 +1546,8 @@ struct _virDomainTPMEmulatorDef { char *name; /* name read from active profile */ virDomainTPMProfileRemoveDisabled removeDisabled; } profile; + bool backup; + bool fsync;
These should IMO be tristate so it's clear if the user used them or not.
We already had issues with cases where the 'no' option was default, but later qemu decided to change it to yes. Libvirt coudn't tell if the user requested 'no' or omitted the option. So we really must not do 'bool' for XML attributes ever again.
I understand. Thank you for the detailed explanation. I shall change it to use virXMLPropTristateBool.
All of the above is provided that either of the options in fact need to be configured, which I'm not sure about.
Swtpm is blocking on fsync on file and directory, so one reason not to enable it could be performance - supposedly fsync is quite time consuming since it waits until file buffers have been written to the physical disk, which was worse for non-SSDs, of course. I think users should have control over this option. Maybe that the backup option should be enabled by default and users can at least disabled it if they wanted to.
I think Stefan Berger, the upstream maintainer, can comment on this better. IMO, fsync can be left for the user to configure, and backup can be set 'yes' as default for directory backend.
CC: Stefan Berger <stefanb@us.ibm.com>
Regards, Arun Menon