Hi Stefan
> On Dec 26, 2020, at 14:15, Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
>
> On 12/22/20 7:12 PM, Eiichi Tsukata wrote:
>> Currently, swtpm TPM state file is removed when the transient domain is
>> powered off or the domain is 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>
>> ---
>> 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
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 512939679b..9e5f6340fb 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 the transient domain is powered off or undefined. This
>
> Nit: the transient domain -> a transient domain
>
Thanks, I’ll fix it.
>
>> + option can be used for storing TPM state on shared storage. By default the
>
> Nit: storing -> preserving
>
> But this is not only related to shared storage, is it? Can we remove 'on shared storage'?
>
Sure.
>
>> + value is ``no``. This attribute only works with the ``emulator`` backend.
>> + The accepted values are ``yes`` and ``no``.
>> +
>> ``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>
>> </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;
>> + }
>> break;
>> case VIR_DOMAIN_TPM_TYPE_LAST:
>> goto error;
>> @@ -26952,6 +26971,8 @@ virDomainTPMDefFormat(virBufferPtr buf,
>> case VIR_DOMAIN_TPM_TYPE_EMULATOR:
>> virBufferAsprintf(buf, " version='%s'",
>> virDomainTPMVersionTypeToString(def->version));
>> + if (def->data.emulator.persistent_state)
>> + virBufferAddLit(buf, " persistent_state='yes'");
>> if (def->data.emulator.hassecretuuid) {
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> virBufferAddLit(buf, ">\n");
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 72771c46b9..109625828a 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1362,6 +1362,7 @@ struct _virDomainTPMDef {
>> char *logfile;
>> unsigned char secretuuid[VIR_UUID_BUFLEN];
>> bool hassecretuuid;
>> + bool persistent_state;
>> } emulator;
>> } data;
>> };
>> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
>> index 872be16570..532e0912bd 100644
>> --- a/src/qemu/qemu_tpm.c
>> +++ b/src/qemu/qemu_tpm.c
>> @@ -729,7 +729,8 @@ qemuExtTPMCleanupHost(virDomainDefPtr def)
>> if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR)
>> continue;
>>
>> - qemuTPMDeleteEmulatorStorage(def->tpms[i]);
>> + if (!def->tpms[i]->data.emulator.persistent_state)
>> + qemuTPMDeleteEmulatorStorage(def->tpms[i]);
>> }
>> }
>>
>> diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.x86_64-latest.args b/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.x86_64-latest.args
>> new file mode 100644
>> index 0000000000..90505c7a76
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.x86_64-latest.args
>> @@ -0,0 +1,38 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/tmp/lib/domain--1-TPM-VM \
>> +USER=test \
>> +LOGNAME=test \
>> +XDG_DATA_HOME=/tmp/lib/domain--1-TPM-VM/.local/share \
>> +XDG_CACHE_HOME=/tmp/lib/domain--1-TPM-VM/.cache \
>> +XDG_CONFIG_HOME=/tmp/lib/domain--1-TPM-VM/.config \
>> +QEMU_AUDIO_DRV=none \
>> +/usr/bin/qemu-system-x86_64 \
>> +-name guest=TPM-VM,debug-threads=on \
>> +-S \
>> +-object secret,id=masterKey0,format=raw,\
>> +file=/tmp/lib/domain--1-TPM-VM/master-key.aes \
>> +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off,\
>> +memory-backend=pc.ram \
>> +-cpu qemu64 \
>> +-m 2048 \
>> +-object memory-backend-ram,id=pc.ram,size=2147483648 \
>> +-overcommit mem-lock=off \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \
>> +-display none \
>> +-no-user-config \
>> +-nodefaults \
>> +-chardev socket,id=charmonitor,fd=1729,server,nowait \
>> +-mon chardev=charmonitor,id=monitor,mode=control \
>> +-rtc base=utc \
>> +-no-shutdown \
>> +-boot menu=on,strict=on \
>> +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
>> +-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \
>> +-chardev socket,id=chrtpm,path=/dev/test \
>> +-device tpm-tis,tpmdev=tpm-tpm0,id=tpm0 \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
>> +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
>> +resourcecontrol=deny \
>> +-msg timestamp=on
>> diff --git a/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml b/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
>> new file mode 100644
>> index 0000000000..45fc4c0e1a
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
>> @@ -0,0 +1,30 @@
>> +<domain type='qemu'>
>> + <name>TPM-VM</name>
>> + <uuid>11d7cd22-da89-3094-6212-079a48a309a1</uuid>
>> + <memory unit='KiB'>2097152</memory>
>> + <currentMemory unit='KiB'>512288</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='pc-i440fx-2.12'>hvm</type>
>> + <boot dev='hd'/>
>> + <bootmenu enable='yes'/>
>> + </os>
>> + <features>
>> + <acpi/>
>> + </features>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + <controller type='usb' index='0'/>
>> + <controller type='pci' index='0' model='pci-root'/>
>> + <input type='mouse' bus='ps2'/>
>> + <input type='keyboard' bus='ps2'/>
>> + <tpm model='tpm-tis'>
>> + <backend type='emulator' version='2.0' persistent_state='yes'/>
>> + </tpm>
>> + <memballoon model='virtio'/>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 9b853c6d59..e96a51d18b 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2460,6 +2460,7 @@ mymain(void)
>> DO_TEST_CAPS_LATEST("tpm-emulator");
>> DO_TEST_CAPS_LATEST("tpm-emulator-tpm2");
>> DO_TEST_CAPS_LATEST("tpm-emulator-tpm2-enc");
>> + DO_TEST_CAPS_LATEST("tpm-emulator-tpm2-pstate");
>> DO_TEST_CAPS_LATEST_PPC64("tpm-emulator-spapr");
>>
>> DO_TEST_PARSE_ERROR("pci-domain-invalid", NONE);
>> diff --git a/tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml b/tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml
>> new file mode 100644
>> index 0000000000..08bc8d690c
>> --- /dev/null
>> +++ b/tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml
>> @@ -0,0 +1,37 @@
>> +<domain type='qemu'>
>> + <name>TPM-VM</name>
>> + <uuid>11d7cd22-da89-3094-6212-079a48a309a1</uuid>
>> + <memory unit='KiB'>2097152</memory>
>> + <currentMemory unit='KiB'>512288</currentMemory>
>> + <vcpu placement='static'>1</vcpu>
>> + <os>
>> + <type arch='x86_64' machine='pc-i440fx-2.12'>hvm</type>
>> + <boot dev='hd'/>
>> + <bootmenu enable='yes'/>
>> + </os>
>> + <features>
>> + <acpi/>
>> + </features>
>> + <cpu mode='custom' match='exact' check='none'>
>> + <model fallback='forbid'>qemu64</model>
>> + </cpu>
>> + <clock offset='utc'/>
>> + <on_poweroff>destroy</on_poweroff>
>> + <on_reboot>restart</on_reboot>
>> + <on_crash>destroy</on_crash>
>> + <devices>
>> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
>> + <controller type='usb' index='0' model='piix3-uhci'>
>> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
>> + </controller>
>> + <controller type='pci' index='0' model='pci-root'/>
>> + <input type='mouse' bus='ps2'/>
>> + <input type='keyboard' bus='ps2'/>
>> + <tpm model='tpm-tis'>
>> + <backend type='emulator' version='2.0' persistent_state='yes'/>
>> + </tpm>
>> + <memballoon model='virtio'>
>> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
>> + </memballoon>
>> + </devices>
>> +</domain>
>> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
>> index 1968be6782..f8bca9f559 100644
>> --- a/tests/qemuxml2xmltest.c
>> +++ b/tests/qemuxml2xmltest.c
>> @@ -761,6 +761,7 @@ mymain(void)
>> DO_TEST_CAPS_LATEST("tpm-emulator");
>> DO_TEST_CAPS_LATEST("tpm-emulator-tpm2");
>> DO_TEST_CAPS_LATEST("tpm-emulator-tpm2-enc");
>> + DO_TEST_CAPS_LATEST("tpm-emulator-tpm2-pstate");
>>
>> DO_TEST("metadata", NONE);
>> DO_TEST("metadata-duplicate", NONE);
>
>
> Otherwise looks good to me.
>
> Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
Thanks for the review.
I’ll fix the patch according to your suggestions and send v2 next year.
By the way, I’m having trouble with my company’s SMTP server and it seems that
I couldn’t send my patch to libvir-list from git send-email. Sorry for confusions.
Best
Eiichi
