[PATCH 0/4] qemu: Support backup and fsync optional attributes in SWTPM
In the upcoming release of swtpm, 2 new options, backup and fsync will be added to the directory backend [1] and [2]. This allows the user to backup and fsync the state file and its parent directory. It lowers the probability of TPM state file corruption in case of a power loss. Although this has not been added to the stable swtpm release yet, Libvirt will require some changes to incorporate these options in the XML config. Following is the sample xml configuration snip to enable backup and fsync: <tpm model='tpm-crb'> <backend type='emulator' version='2.0' backup='yes' fsync='yes'/> </tpm> Tested with swtpm commit #8ecf2c1 and libtpms commit #18e42c8d: $ ps aux | grep swtpm arun 33881 0.0 0.0 24080 10188 ? S 00:09 0:00 /usr/local/bin/swtpm socket --ctrl type=unixio,path=/run/user/1000/libvirt/qemu/run/swtpm/5-guest-vm-swtpm.sock,mode=0600 --tpmstate dir=/home/arun/.config/libvirt/qemu/swtpm/0a64239e-6311-4e53-8903-d1d49608a751/tpm2,mode=0600,lock,backup,fsync --log file=/home/arun/.cache/libvirt/qemu/log/guest-vm-swtpm.log --terminate --tpm2 $ ls -lrt $HOME/.config/libvirt/qemu/swtpm/0a64239e-6311-4e53-8903-d1d49608a751/tpm2 total 16 -rw-------. 1 arun arun 6807 Sep 7 00:14 tpm2-00.permall.bak -rw-------. 1 arun arun 6807 Sep 7 01:02 tpm2-00.permall [1] https://github.com/stefanberger/swtpm/commit/186f72a3a995fc68957ffc0b1438bb6... [2] https://github.com/stefanberger/swtpm/commit/795c1973c913da59925fa6c2d688c5e... Arun Menon (4): conf: Add backup and fsync optional swtpm attributes to RNG schema conf: Parse and read the xml attributes for swtpm backup and fsync qemu: tpm: Add backup and fsync options to the swtpm command qemuxmlconftest: Add example for backup and fsync tpmstate options docs/formatdomain.rst | 16 ++++++++++ src/conf/domain_conf.c | 31 +++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/conf/schemas/domaincommon.rng | 10 ++++++ src/qemu/qemu_tpm.c | 17 ++++++++-- src/util/virtpm.c | 2 ++ src/util/virtpm.h | 2 ++ .../tpm-emulator-tpm2-pstate.xml | 2 +- 8 files changed, 79 insertions(+), 3 deletions(-) -- 2.54.0
From: Arun Menon <armenon@redhat.com> SWTPM will support backup and fsync of the state file. [1] and [2] Libvirt needs to support these options while launching a VM. This commit adds these optional attributes to the RNG schema [1] https://github.com/stefanberger/swtpm/commit/186f72a3a995fc68957ffc0b1438bb6... [2] https://github.com/stefanberger/swtpm/commit/795c1973c913da59925fa6c2d688c5e... Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/schemas/domaincommon.rng | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 887fb8f808..0751d7ff47 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -6196,6 +6196,16 @@ <ref name="uint8"/> </attribute> </optional> + <optional> + <attribute name="backup"> + <ref name="virYesNo"/> + </attribute> + </optional> + <optional> + <attribute name="fsync"> + <ref name="virYesNo"/> + </attribute> + </optional> </group> <group> <attribute name="type"> -- 2.54.0
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> + * */ 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; + } + } + + 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; + } + } + 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; }; struct _virDomainTPMDef { -- 2.54.0
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) 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. 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. 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. All of the above is provided that either of the options in fact need to be configured, which I'm not sure about.
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
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.
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
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
On Tue, Sep 08, 2026 at 12:36:03 -0400, Stefan Berger wrote:
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> ---
[...]
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.
It only makes sense to expose an option if the user would be changing it. And for that to happen users need to know when to set given option. With the description above which only states positive features of the option I don't think anyone would ever set fsync to false. So strictly based on the description above the option doesn't need to exist. Now ...
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.
[...]
+ 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.
... with drawback being mentioned it (IMO barely) makes sense to have it as an option.
Maybe that the backup option should be enabled by default and users can at least disabled it if they wanted to.
Note that the usual libvirt approach is that if an option is not mentioned in the XML the default of qemu/swtpm/etc is used. In this case I presume it will mean that both backups and fsync will be disabled. Since either of those doesn't seem to impact guest ABI it's fine if that's changed on swtpms side. On libvirt's side I think we could technically pick a different default but I don't think we want to go that way for defaults. So both are fine as options if they are documented in a way that shows why it's an option.
On Wed, Sep 09, 2026 at 11:37:17AM +0200, Peter Krempa wrote:
On Tue, Sep 08, 2026 at 12:36:03 -0400, Stefan Berger wrote:
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> ---
[...]
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.
It only makes sense to expose an option if the user would be changing it. And for that to happen users need to know when to set given option.
With the description above which only states positive features of the option I don't think anyone would ever set fsync to false. So strictly based on the description above the option doesn't need to exist.
Now ...
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.
[...]
+ 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.
Thanks Stefan.
... with drawback being mentioned it (IMO barely) makes sense to have it as an option.
Maybe that the backup option should be enabled by default and users can at least disabled it if they wanted to.
Note that the usual libvirt approach is that if an option is not mentioned in the XML the default of qemu/swtpm/etc is used.
In this case I presume it will mean that both backups and fsync will be disabled. Since either of those doesn't seem to impact guest ABI it's fine if that's changed on swtpms side.
On libvirt's side I think we could technically pick a different default but I don't think we want to go that way for defaults.
So both are fine as options if they are documented in a way that shows why it's an option.
Thanks Peter, I understand now that we cannot force a default in libvirt. If the option is not set, the default set by the underlying swtpm should be honoured. Based on this discussion I will keep both the options, with the following documentation, if thats okay. ``backup`` The optional ``backup`` attribute controls whether swtpm creates a backup of its TPM state file before updating it. Enabling backup protects against TPM state file corruption if swtpm process crashes unexpectedly. Disabling backups avoids file-copying overhead. The accepted values are ``yes`` and ``no``. :since:`Since 12.8.0 ``fsync`` The optional ``fsync`` attribute controls whether swtpm explicitly flushes its state changes to the physical storage using fsync(). Enabling fsync ensures data remains consistent across host power loss, but might introduces I/O latency. Disabling fsync removes this latency at the expense of host-level crash resilience. Acceptable values are ``yes`` and ``no``. :since:`Since 12.8.0` Regards, Arun Menon
From: Arun Menon <armenon@redhat.com> This commit adds the backup and fsync options while constructing the swtpm command. These attributes only work with the directory backend. Signed-off-by: Arun Menon <armenon@redhat.com> --- docs/formatdomain.rst | 16 ++++++++++++++++ src/qemu/qemu_tpm.c | 17 +++++++++++++++-- src/util/virtpm.c | 2 ++ src/util/virtpm.h | 2 ++ 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 5a278f3717..e0b090b8cc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -8923,6 +8923,22 @@ Example: usage of external TPM emulator :since:`Since 9.0.0` This attribute only works with the ``emulator`` backend. The accepted values are ``yes`` and ``no``. :since:`Since 7.0.0` +``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` + +``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` + ``active_pcr_banks`` The ``active_pcr_banks`` node is used to define which of the PCR banks of a TPM 2.0 to activate. Valid names are for example sha1, sha256, sha384, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 34e11cc02f..379a590c0c 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -676,6 +676,8 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, const virQEMUDriverConfig *cfg) { const char *lock = ",lock"; + const char *backup = ",backup"; + const char *fsync = ",fsync"; if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) { if (qemuTPMHasSharedStorage(cfg, tpmDef)) @@ -684,6 +686,17 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, lock = ""; } + if (emulator->source_type == VIR_DOMAIN_TPM_SOURCE_TYPE_DIR) { + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_BACKUP)) { + VIR_WARN("This swtpm version doesn't support state file backup feature"); + backup = ""; + } + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_FSYNC)) { + VIR_WARN("This swtpm version doesn't support state file fsync feature"); + fsync = ""; + } + } + virCommandAddArg(cmd, "--tpmstate"); switch (emulator->source_type) { case VIR_DOMAIN_TPM_SOURCE_TYPE_FILE: @@ -693,8 +706,8 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, case VIR_DOMAIN_TPM_SOURCE_TYPE_DIR: case VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT: case VIR_DOMAIN_TPM_SOURCE_TYPE_LAST: - virCommandAddArgFormat(cmd, "dir=%s,mode=0600%s", - emulator->source_path, lock); + virCommandAddArgFormat(cmd, "dir=%s,mode=0600%s%s%s", + emulator->source_path, lock, backup, fsync); break; } } diff --git a/src/util/virtpm.c b/src/util/virtpm.c index cf0f20e009..2e890f94ef 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -44,6 +44,8 @@ VIR_ENUM_IMPL(virTPMSwtpmFeature, "nvram-backend-file", "cmdarg-print-info", "tpmstate-opt-lock", + "tpmstate-dir-backend-opt-backup", + "tpmstate-dir-backend-opt-fsync", ); VIR_ENUM_IMPL(virTPMSwtpmSetupFeature, diff --git a/src/util/virtpm.h b/src/util/virtpm.h index 2892dd307e..1d2c94d36b 100644 --- a/src/util/virtpm.h +++ b/src/util/virtpm.h @@ -35,6 +35,8 @@ typedef enum { VIR_TPM_SWTPM_FEATURE_NVRAM_BACKEND_FILE, VIR_TPM_SWTPM_FEATURE_CMDARG_PRINT_INFO, VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK, + VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_BACKUP, + VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_FSYNC, VIR_TPM_SWTPM_FEATURE_LAST } virTPMSwtpmFeature; -- 2.54.0
On Mon, Sep 07, 2026 at 01:24:10 +0530, Arun Menon via Devel wrote:
From: Arun Menon <armenon@redhat.com>
This commit adds the backup and fsync options while constructing the swtpm command. These attributes only work with the directory backend.
Signed-off-by: Arun Menon <armenon@redhat.com> --- docs/formatdomain.rst | 16 ++++++++++++++++ src/qemu/qemu_tpm.c | 17 +++++++++++++++-- src/util/virtpm.c | 2 ++ src/util/virtpm.h | 2 ++ 4 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 5a278f3717..e0b090b8cc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -8923,6 +8923,22 @@ Example: usage of external TPM emulator :since:`Since 9.0.0` This attribute only works with the ``emulator`` backend. The accepted values are ``yes`` and ``no``. :since:`Since 7.0.0`
+``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` + +``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` + ``active_pcr_banks`` The ``active_pcr_banks`` node is used to define which of the PCR banks of a TPM 2.0 to activate. Valid names are for example sha1, sha256, sha384, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 34e11cc02f..379a590c0c 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -676,6 +676,8 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, const virQEMUDriverConfig *cfg) { const char *lock = ",lock"; + const char *backup = ",backup"; + const char *fsync = ",fsync";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) { if (qemuTPMHasSharedStorage(cfg, tpmDef)) @@ -684,6 +686,17 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, lock = ""; }
+ if (emulator->source_type == VIR_DOMAIN_TPM_SOURCE_TYPE_DIR) { + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_BACKUP)) { + VIR_WARN("This swtpm version doesn't support state file backup feature"); + backup = ""; + } + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_FSYNC)) { + VIR_WARN("This swtpm version doesn't support state file fsync feature");
This should be a hard error. Not honouring user's config shoudn't happen. Additionally VIR_WARN is usually wrong. It only logs an entry which will get ignored by users. Don't use it, use VIR_DEGBUG for debug logs or report proper errors. The only acceptable place for VIR_WARN is on cleanup paths that don't have a reasonable way to report errors to the user.
Hi Peter, Thanks for the review. On Mon, Sep 07, 2026 at 01:04:51PM +0200, Peter Krempa wrote:
On Mon, Sep 07, 2026 at 01:24:10 +0530, Arun Menon via Devel wrote:
From: Arun Menon <armenon@redhat.com>
This commit adds the backup and fsync options while constructing the swtpm command. These attributes only work with the directory backend.
Signed-off-by: Arun Menon <armenon@redhat.com> --- docs/formatdomain.rst | 16 ++++++++++++++++ src/qemu/qemu_tpm.c | 17 +++++++++++++++-- src/util/virtpm.c | 2 ++ src/util/virtpm.h | 2 ++ 4 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 5a278f3717..e0b090b8cc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -8923,6 +8923,22 @@ Example: usage of external TPM emulator :since:`Since 9.0.0` This attribute only works with the ``emulator`` backend. The accepted values are ``yes`` and ``no``. :since:`Since 7.0.0`
+``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` + +``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` + ``active_pcr_banks`` The ``active_pcr_banks`` node is used to define which of the PCR banks of a TPM 2.0 to activate. Valid names are for example sha1, sha256, sha384, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 34e11cc02f..379a590c0c 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -676,6 +676,8 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, const virQEMUDriverConfig *cfg) { const char *lock = ",lock"; + const char *backup = ",backup"; + const char *fsync = ",fsync";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) { if (qemuTPMHasSharedStorage(cfg, tpmDef)) @@ -684,6 +686,17 @@ qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, lock = ""; }
+ if (emulator->source_type == VIR_DOMAIN_TPM_SOURCE_TYPE_DIR) { + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_BACKUP)) { + VIR_WARN("This swtpm version doesn't support state file backup feature"); + backup = ""; + } + if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_FSYNC)) { + VIR_WARN("This swtpm version doesn't support state file fsync feature");
This should be a hard error. Not honouring user's config shoudn't happen.
Additionally VIR_WARN is usually wrong. It only logs an entry which will get ignored by users. Don't use it, use VIR_DEGBUG for debug logs or report proper errors. The only acceptable place for VIR_WARN is on cleanup paths that don't have a reasonable way to report errors to the user.
Agreed. I will use VIR_ERR_ARGUMENT_UNSUPPORTED. Thank you. Regards, Arun Menon
From: Arun Menon <armenon@redhat.com> The test suite now validates the 2 options backup and fsync added to swtpm. Signed-off-by: Arun Menon <armenon@redhat.com> --- tests/qemuxmlconfdata/tpm-emulator-tpm2-pstate.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2-pstate.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2-pstate.xml index 6569b5e4dd..0385045954 100644 --- a/tests/qemuxmlconfdata/tpm-emulator-tpm2-pstate.xml +++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2-pstate.xml @@ -28,7 +28,7 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <tpm model='tpm-tis'> - <backend type='emulator' version='2.0' persistent_state='yes'/> + <backend type='emulator' version='2.0' persistent_state='yes' backup='yes' fsync='yes'/> </tpm> <audio id='1' type='none'/> <memballoon model='virtio'> -- 2.54.0
participants (3)
-
Arun Menon -
Peter Krempa -
Stefan Berger