On Wed, Aug 28, 2024 at 11:02:28AM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>
> Learn to parse a file path for the TPM state.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> ---
> docs/formatdomain.rst | 15 +++++++++++++++
> src/conf/domain_conf.c | 21 +++++++++++++++++++++
> src/conf/domain_conf.h | 6 ++++++
> src/conf/schemas/domaincommon.rng | 11 +++++++++++
> tests/qemuxmlconfdata/tpm-emulator-tpm2.xml | 1 +
> 5 files changed, 54 insertions(+)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 47d3e2125e..4818113bc2 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -8170,6 +8170,21 @@ Example: usage of the TPM Emulator
> The default version used depends on the combination of hypervisor, guest
> architecture, TPM model and backend.
>
> +``source``
> + The ``source`` element specifies the location of the TPM state storage . This
> + element only works with the ``emulator`` backend.
> +
> + If not specified, the storage configuration is left to libvirt discretion.
> +
> + The following attributes are supported:
> +
> + ``path``
> + The path to the TPM state storage file.
> +
> + This attribute requires that swtpm v0.7 or later is installed.
> +
> + :since:`Since v10.8.0`
> +
> ``persistent_state``
> The ``persistent_state`` attribute indicates whether 'swtpm' TPM state is
> kept or not when a transient domain is powered off or undefined. This
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5f0b35be5e..d62a0423d8 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10789,6 +10789,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
> int nbackends;
> int nnodes;
> size_t i;
> + xmlNodePtr source_node = NULL;
> g_autofree char *path = NULL;
> g_autofree char *secretuuid = NULL;
> g_autofree char *persistent_state = NULL;
> @@ -10862,6 +10863,18 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
> def->data.emulator.hassecretuuid = true;
> }
>
> + source_node = virXPathNode("./backend/source", ctxt);
> + if (source_node) {
> + path = virXMLPropString(source_node, "file");
> + if (!path) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing TPM file source"));
> + goto error;
> + }
> + def->data.emulator.storage_type = VIR_DOMAIN_TPM_STORAGE_FILE;
> + def->data.emulator.storagepath = g_steal_pointer(&path);
> + }
> +
> persistent_state = virXMLPropString(backends[0], "persistent_state");
> if (persistent_state) {
> if (virStringParseYesNo(persistent_state,
> @@ -25065,6 +25078,14 @@ virDomainTPMDefFormat(virBuffer *buf,
>
> virXMLFormatElement(&backendChildBuf, "active_pcr_banks", NULL, &activePcrBanksBuf);
> }
> + switch (def->data.emulator.storage_type) {
> + case VIR_DOMAIN_TPM_STORAGE_FILE:
> + virBufferAsprintf(&backendChildBuf, "<source file='%s'/>\n",
> + def->data.emulator.storagepath);
> + break;
> + case VIR_DOMAIN_TPM_STORAGE_DEFAULT:
> + break;
> + }
> break;
> case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
> if (def->data.external.source->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 659299bdd1..371e6ecf6c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1463,6 +1463,11 @@ typedef enum {
> VIR_DOMAIN_TPM_PCR_BANK_LAST
> } virDomainPcrBank;
>
> +typedef enum {
> + VIR_DOMAIN_TPM_STORAGE_DEFAULT,
> + VIR_DOMAIN_TPM_STORAGE_FILE,
> +} virDomainTPMStorage;
> +
> #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"
>
> struct _virDomainTPMDef {
> @@ -1478,6 +1483,7 @@ struct _virDomainTPMDef {
> struct {
> virDomainTPMVersion version;
> virDomainChrSourceDef *source;
> + virDomainTPMStorage storage_type;
> char *storagepath;
> char *logfile;
> unsigned int debug;
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index efb5f00d77..62d3f0e6fe 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -5923,6 +5923,7 @@
> <interleave>
> <ref name="tpm-backend-emulator-encryption"/>
> <ref name="tpm-backend-emulator-active-pcr-banks"/>
> + <ref name="tpm-backend-emulator-source"/>
> </interleave>
> <optional>
> <attribute name="persistent_state">
> @@ -5981,6 +5982,16 @@
> </optional>
> </define>
>
> + <define name="tpm-backend-emulator-source">
> + <optional>
> + <element name="source">
> + <attribute name="file">
> + <ref name="filePath"/>
> + </attribute>
> + </element>
> + </optional>
> + </define>
> +
> <define name="tpm-backend-emulator-encryption">
> <optional>
> <element name="encryption">
> diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> index 8a613db456..4c61e2645b 100644
> --- a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> +++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> @@ -34,6 +34,7 @@
> <sha256/>
> <sha512/>
> </active_pcr_banks>
> + <source file='/path/to/state'/>
> </backend>
> </tpm>
> <audio id='1' type='none'/>
So you're modifying an existing XML file, which is fine, but this
modification looks conceptually incomplete to me.
When we have two different backend options - in this case 'dir' and
'file', then we would normally have an "type" attribute on the parent
element, that dictates what child elements are permitted.
Picking a chunk from earlier in the patch:
> +typedef enum {
> + VIR_DOMAIN_TPM_STORAGE_DEFAULT,
> + VIR_DOMAIN_TPM_STORAGE_FILE,
> +} virDomainTPMStorage;
I would have expected to see
typedef enum {
VIR_DOMAIN_TPM_STORAGE_DIR,
VIR_DOMAIN_TPM_STORAGE_FILE,
} virDomainTPMStorage;
and have this enum be exposed as the 'type' attribute on <backend>.
Also, if we're going to expose the raw file path as a user configurable
optin for "file" type, then IMHO, we should also expose the dir path
for our existing backend. It doesn't make sense to allow one to be
configured, but not both.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
