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

[Eiichi Tsukata]

Hi Michal

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

Thank you very much for detailed review!
Actually, this is second contribution :-)
The first one was 9 years ago:
https://libvirt.org/git/?p=libvirt.git;a=commit;h=0ac3baee2c2fd56ef89f24f5ea484e39d2bf35f5

Eiichi