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

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

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

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

> </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!

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

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

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

John