[libvirt] [PATCH v2 1/6] tpm: Enable TPM CRB interface
Stefan Berger
stefanb at linux.vnet.ibm.com
Thu Apr 26 01:00:01 UTC 2018
On 04/25/2018 03:13 PM, John Ferlan wrote:
>
> On 04/25/2018 02:24 PM, Stefan Berger wrote:
>> 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 ?
>>
> Ah so the "2" is a version number where it can be 1.5 or 2 and CRB is
> only supported on 2, while TIS is supported on both (whether the next
> set of patches are required for TPM 2 and TIS isn't clear to me).
>
> To a degree I suspect this is only used by someone who knows what they
> are doing, but since I didn't know I figured I'd ask. Not sure if
> there's a happy medium. Perhaps what you wrote above is "enough" to add
> to the general description for TPM device in this section of the docs.
> Something that would go after "TPM functionality" in the first line.
> Enough to give a glimmer of understanding. Of course the next set of
> patches w/ emulator will make this even more "interesting" to describe.
>
>>> 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.
>>
> I mainly noted it because it looked odd... The whole machine thing is a
> big black box and incurs seemingly endless discussions.
I rebased and am now using ' <type arch='x86_64'
machine='pc-i440fx-2.12'>hvm</type>'. This works fine now.
>
>>>> + <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/.
>>
> Check out the DO_TEST_CAPS_LATEST macro... Note the only consumer is
> 'disk-drive-write-cache' and it uses 'pc-i440fx-2.6' for a machine type
> - hopefully this helps with the other quandary.
I see that test case now that I rebased. Do I really need the
DO_TEST_CAPS_LATEST? It works without it. Maybe due to some recent
changes in libvirt it doesn't seem to require this capability in the
local QEMU executable?
Stefan
More information about the libvir-list
mailing list