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 <[email protected]> > > > > > > > > > > Update logic for both creation and parsing the 2 new attributes backup > > > > > and fsync introduced in swtpm. > > > > > > > > > > Signed-off-by: Arun Menon <[email protected]> > > > > > --- > > [...] > > > > > 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 >
