On 1/4/21 3:31 AM, Eiichi Tsukata wrote:
Currently, swtpm TPM state file is removed when a transient domain is
powered off or undefined. When we store TPM state on a shared storage
such as NFS and use transient domain, TPM states should be kept as it is.
Add per-TPM emulator option `persistent_sate` for keeping TPM state.
This option only works for the emulator type backend and looks as follows:
<tpm model='tpm-tis'>
<backend type='emulator' persistent_state='yes'/>
</tpm>
Signed-off-by: Eiichi Tsukata <eiichi.tsukata@xxxxxxxxxxx>
Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
---
docs/formatdomain.rst | 7 ++++
docs/schemas/domaincommon.rng | 12 ++++++
src/conf/domain_conf.c | 21 ++++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_tpm.c | 3 +-
...pm-emulator-tpm2-pstate.x86_64-latest.args | 38 +++++++++++++++++++
.../tpm-emulator-tpm2-pstate.xml | 30 +++++++++++++++
tests/qemuxml2argvtest.c | 1 +
...tpm-emulator-tpm2-pstate.x86_64-latest.xml | 37 ++++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
10 files changed, 150 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml
Couple of comments.
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 512939679b..87bbd1a4f1 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -6986,6 +6986,13 @@ Example: usage of the TPM Emulator
- '1.2' : creates a TPM 1.2
- '2.0' : creates a TPM 2.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
+ option can be used for preserving TPM state. By default the value is ``no``.
+ This attribute only works with the ``emulator`` backend. The accepted values
+ are ``yes`` and ``no``.
It would be nice to have 'since 7.0.0' here so that users with older
libvirt know they need a newer version.
+
``encryption``
The ``encryption`` element allows the state of a TPM emulator to be
encrypted. The ``secret`` must reference a secret object that holds the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 795b654feb..d7cedc014c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4780,6 +4780,18 @@
</optional>
</group>
</choice>
+ <choice>
+ <group>
+ <optional>
+ <attribute name="persistent_state">
+ <choice>
+ <value>yes</value>
+ <value>no</value>
+ </choice>
+ </attribute>
+ </optional>
+ </group>
+ </choice>
This looks needlessly complicated. I'd just put <optional>... under
type="emulator" case. I know you were trying to mimic what version
attribute does, but that's wrong too :-)
</element>
</define>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 23415b323c..82c3a68347 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13178,6 +13178,12 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
* <encryption secret='32ee7e76-2178-47a1-ab7b-269e6e348015'/>
* </backend>
* </tpm>
+ *
+ * Emulator persistent_state is supported with the following:
+ *
+ * <tpm model='tpm-tis'>
+ * <backend type='emulator' version='2.0' persistent_state='yes'>
+ * </tpm>
*/
static virDomainTPMDefPtr
virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
@@ -13193,6 +13199,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
g_autofree char *backend = NULL;
g_autofree char *version = NULL;
g_autofree char *secretuuid = NULL;
+ g_autofree char *persistent_state = NULL;
g_autofree xmlNodePtr *backends = NULL;
def = g_new0(virDomainTPMDef, 1);
@@ -13265,6 +13272,18 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->data.emulator.hassecretuuid = true;
}
+
+ persistent_state = virXMLPropString(backends[0], "persistent_state");
+ if (persistent_state) {
+ if (virStringParseYesNo(persistent_state,
+ &def->data.emulator.persistent_state) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid persistent_state value, either 'yes' or 'no'"));
+ goto error;
+ }
+ } else {
+ def->data.emulator.persistent_state = false;
This is redundant. g_new0() makes sure the memory is zero initialized,
and this this is already false.
The rest looks good.
I'm fixing all the small nits I've raised and pushing. Congratulations
on your first libvirt contribution!
Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
Michal
