1. Patchew
  2. Libvirt
  3. View series

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

Eiichi Tsukata posted 1 patch 3 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210104023159.181913-1-eiichi.tsukata@nutanix.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
[PATCH v2] conf: Add support for keeping TPM emulator state
Posted by Eiichi Tsukata 3 years, 3 months ago 
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@nutanix.com>
Reviewed-by: Stefan Berger <stefanb@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

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``.
+
 ``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);
-- 
2.28.0
Re: [PATCH v2] conf: Add support for keeping TPM emulator state
Posted by Daniel Henrique Barboza 3 years, 3 months ago 

On 1/3/21 11:31 PM, 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@nutanix.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.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
> 
> 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``.
> +
>   ``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);
>
Re: [PATCH v2] conf: Add support for keeping TPM emulator state
Posted by Michal Privoznik 3 years, 3 months ago 
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@nutanix.com>
> Reviewed-by: Stefan Berger <stefanb@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@redhat.com>

Michal
Re: [PATCH v2] conf: Add support for keeping TPM emulator state
Posted by Eiichi Tsukata 3 years, 3 months ago 
Hi Michal

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.