Re: [PATCH v2 1/6] tpm: Enable TPM CRB interface

Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> · Wed, 25 Apr 2018 14:24:51 -0400

On 04/25/2018 01:13 PM, John Ferlan wrote:

On 04/10/2018 10:50 PM, Stefan Berger wrote:
Enable the TPM CRB interface added in QEMU 2.12. the TPM CRB
interface is a simpler interface than the TPM TIS and is only
available for TPM 2.

Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
---
  docs/formatdomain.html.in                         |  2 ++
  docs/schemas/domaincommon.rng                     |  5 +++-
  src/conf/domain_conf.c                            |  5 ++--
  src/conf/domain_conf.h                            |  1 +
  src/qemu/qemu_capabilities.c                      |  5 ++++
  src/qemu/qemu_capabilities.h                      |  1 +
  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml |  1 +
  tests/qemuxml2argvdata/tpm-passthrough-crb.args   | 24 +++++++++++++++
  tests/qemuxml2argvdata/tpm-passthrough-crb.xml    | 32 ++++++++++++++++++++
  tests/qemuxml2argvtest.c                          |  3 ++
  tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml  | 36 +++++++++++++++++++++++
  11 files changed, 111 insertions(+), 4 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args
  create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.xml
  create mode 100644 tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml

TPM is not in my wheelhouse of knowledge, but I'm willing to learn...
Still this patch seems to be more about a new model...

The TPM has different hardware interface. One is called the TIS (TPM 
Interface Specification) and the more recent one, typically only found 
with a TPM 2 underneath, is the CRB (Command Response Buffer) Interface. 
Both are MMIO interfaces, just work a little different. TIS was already 
used with a TPM 1.2 but can also be the interface of a TPM 2.


Fist off - while somewhat painful, separating the XML from the
capabilities and capabilities from the qemu specific changes is
generally preferred. That way the XML can be agreed upon and it
shouldn't interfere with pure XML processing or xml2xml testing. Since
capabilities go through periods of flux and extreme change - so
separating it makes lagged reviews possible.

Thus it seems this patch gets split in at least 2 parts and perhaps a
3rd patch to alter qemu_command for TPM_CRB specific changes needs to be
added.

You will also need to alter qemuxml2xmltest in order to really test that
xml2xmloutdata file.

Using DO_TEST_CAPS_LATEST is the "new" methodology behind testing
capability requirements for xml2argv - so you'll have some file name
differences in your .args output.

Finally since there's quite a few capabilities adjustments since you
posted, I cannot git am -3 apply the series at all. So this review is
just "by sight". Hazards of not reviewing promptly as of late I'm
afraid. Hopefully things won't be as crazy with capabilities adjustments
for the next few months.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 08dc74b..16fc7db 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7628,6 +7628,8 @@ qemu-kvm -net nic,model=? /dev/null
            The <code>model</code> attribute specifies what device
            model QEMU provides to the guest. If no model name is provided,
            <code>tpm-tis</code> will automatically be chosen.
+          Another available choice is the <code>tpm-crb</code>, which
+          should only be used when the backend is a TPM 2.
You know what this means, but does the "general" reader know what this
means? I have no idea what a "TPM 2" is or means...

Now that of course depends on how much background we want to give on 
this page. What would it make clearer for you considering what I wrote 
above ?


Also needs a <since... /> type tag which will be at least 4.4.0 as we're
close to locking 4.3.0 down...

I will add it.


          </p>
        </dd>
        <dt><code>backend</code></dt>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 8165e69..be5c628 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4112,7 +4112,10 @@
      <element name="tpm">
        <optional>
          <attribute name="model">
-          <value>tpm-tis</value>
+          <choice>
+            <value>tpm-tis</value>
+            <value>tpm-crb</value>
+          </choice>
          </attribute>
        </optional>
        <ref name="tpm-backend"/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ae7c0d9..232174a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -858,7 +858,8 @@ VIR_ENUM_IMPL(virDomainRNGBackend,
                "egd");
  
  VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST,
-              "tpm-tis")
+              "tpm-tis",
+              "tpm-crb")
  
  VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST,
                "passthrough")
@@ -12549,8 +12550,6 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                         _("Unknown TPM frontend model '%s'"), model);
          goto error;
-    } else {
-        def->model = VIR_DOMAIN_TPM_MODEL_TIS;
      }
Should be OK since the default is TIS (e.g. model = 0 by default).

  
      ctxt->node = node;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 61379e5..1724340 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1277,6 +1277,7 @@ struct _virDomainHubDef {
  
  typedef enum {
      VIR_DOMAIN_TPM_MODEL_TIS,
+    VIR_DOMAIN_TPM_MODEL_CRB,
  
      VIR_DOMAIN_TPM_MODEL_LAST
  } virDomainTPMModel;
So model=0 is TIS and is the default...

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e54dde6..0952663 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -466,6 +466,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
                /* 285 */
                "virtio-mouse-ccw",
                "virtio-tablet-ccw",
+              "tpm-crb",
      );
  
  
@@ -3104,6 +3105,10 @@ const struct tpmTypeToCaps virQEMUCapsTPMModelsToCaps[] = {
          .type = VIR_DOMAIN_TPM_MODEL_TIS,
          .caps = QEMU_CAPS_DEVICE_TPM_TIS,
      },
+    {
+        .type = VIR_DOMAIN_TPM_MODEL_CRB,
+        .caps = QEMU_CAPS_DEVICE_TPM_CRB,
+    },
  };
  
  static int
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 3f3c29f..604525a 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -450,6 +450,7 @@ typedef enum {
      /* 285 */
      QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */
      QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */
+    QEMU_CAPS_DEVICE_TPM_CRB, /* -device tpm-crb */
  
      QEMU_CAPS_LAST /* this must always be the last item */
  } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
index 334296e..39ee4f4 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
@@ -225,6 +225,7 @@
    <flag name='iscsi.password-secret'/>
    <flag name='isa-serial'/>
    <flag name='dump-completed'/>
+  <flag name='tpm-crb'/>
    <version>2011090</version>
    <kvmVersion>0</kvmVersion>
    <microcodeVersion>390060</microcodeVersion>
diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
new file mode 100644
index 0000000..ae052b4
--- /dev/null
+++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
@@ -0,0 +1,24 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name TPM-VM \
+-S \
+-M pc-0.12 \
+-m 2048 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \
+-nographic \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-boot c \
+-usb \
+-tpmdev passthrough,id=tpm-tpm0,path=/dev/tpm0,\
+cancel-path=/sys/class/misc/tpm0/device/cancel \
+-device tpm-crb,tpmdev=tpm-tpm0,id=tpm0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
FWIW: Since there was no alteration to qemu_command.c this "worked" when
QEMU_CAPS_DEVICE_TPM_TIS was configured due to how qemuBuildTPMDevStr is
coded.

diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.xml b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml
new file mode 100644
index 0000000..d4f3873
--- /dev/null
+++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml
@@ -0,0 +1,32 @@
+<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-0.12'>hvm</type>
!! That's an old machine type !!  Hazards of copying old file ;-) /me
wonders if that'd even work w/ 2.12!

It would work with QEMU 2.12. Now the question is what to write there. 
When I write pc-q35-2.12 I get the following error:

568) QEMU XML-2-ARGV tpm-passthrough-crb                               
... libvirt: QEMU Driver error : unsupported configuration: The 
'i82801b11-bridge' device is not supported by this QEMU binary
FAILED

I am not sure what to change so this DMI to PCI bridge doesn't get used, 
although '-device \?' on this executable does show this device.



+    <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-crb'>
+      <backend type='passthrough'>
+        <device path='/dev/tpm0'/>
+      </backend>
+    </tpm>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 308d71f..2992197 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2134,6 +2134,9 @@ mymain(void)
  
      DO_TEST("tpm-passthrough",
              QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS);
+    DO_TEST("tpm-passthrough-crb",
+            QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS,
IOW: This would have failed if you didn't add the TPM_TIS here...

Yes. In that regard I was wondering how to go about these attributes. We 
don't want to start QEMU but only create its command line. Can we expect 
that someone running the tests has the latest QEMU on the system ? I can 
remove it when I copy /usr/local/bin/qemu-system... to /usr/bin/.


+            QEMU_CAPS_DEVICE_TPM_CRB);
      DO_TEST_PARSE_ERROR("tpm-no-backend-invalid",
                          QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS);
  
diff --git a/tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml b/tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml
new file mode 100644
index 0000000..ad094a4
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml
@@ -0,0 +1,36 @@
+<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-0.12'>hvm</type>
Perhaps a more recent machine type value?

And that creates some issues with that bridge error above.


+    <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'>
+      <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-crb'>
+      <backend type='passthrough'>
+        <device path='/dev/tpm0'/>
+      </backend>
+    </tpm>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+    </memballoon>
+  </devices>
+</domain>

So patch 1 has:

     docs/*
     src/conf/*
     tests/qemuxml2xmltest.c
     tests/qemuxml2argvdata/tpm-passthrough-crb.xml
     tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml

and patch 2 has:

     src/qemu/qemu_capabilities.c
     src/qemu/qemu_capabilities.h
     tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xm

and patch 3 has:

     src/qemu/qemu_command.c
     tests/qemuxml2argvtest.c
     tests/qemuxml2argvdata/tpm-passthrough-crb.x86_64-latest.args
Ok. I will split it like that and post it as a separate series.

Thanks for looking at it.
   Stefan


John


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list






