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

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(a)linux.vnet.ibm.com&gt; >>> --- >>> 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.

>>> + <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.