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