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

Stefan Berger stefanb at linux.vnet.ibm.com
Wed Apr 25 18:24:51 UTC 2018


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 at linux.vnet.ibm.com>
>> ---
>>   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
>




More information about the libvir-list mailing list