Re: [PATCH v2] conf: Add support for keeping TPM emulator state

On Jan 6, 2021, at 19:45, Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: 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(a)nutanix.com&gt; > Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; > --- > 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(a)redhat.com&gt; Michal