Add a test case for the formation of the tpm-crb QEMU device
command line.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 src/qemu/qemu_command.c                         | 16 ++++++++++++++-
 tests/qemuxml2argvdata/tpm-passthrough-crb.args | 26 +++++++++++++++++++++++++
 tests/qemuxml2argvtest.c                        |  2 ++
 3 files changed, 43 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 418729b..198b44e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9387,8 +9387,22 @@ qemuBuildTPMDevStr(const virDomainDef *def,
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     const virDomainTPMDef *tpm = def->tpm;
     const char *model = virDomainTPMModelTypeToString(tpm->model);
+    virQEMUCapsFlags flag;
 
-    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_TIS)) {
+    switch (tpm->model) {
+    case VIR_DOMAIN_TPM_MODEL_TIS:
+        flag = QEMU_CAPS_DEVICE_TPM_TIS;
+        break;
+    case VIR_DOMAIN_TPM_MODEL_CRB:
+        flag = QEMU_CAPS_DEVICE_TPM_CRB;
+        break;
+    case VIR_DOMAIN_TPM_MODEL_LAST:
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unknown TPM device model %d"), tpm->model);
+        goto error;
+    }
+
+    if (!virQEMUCapsGet(qemuCaps, flag)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("The QEMU executable %s does not support TPM "
                        "model %s"),
diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
new file mode 100644
index 0000000..010495d
--- /dev/null
+++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
@@ -0,0 +1,26 @@
+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 \
+-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \
+-m 2048 -\
+smp 1,sockets=1,cores=1,threads=1 \
+-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot order=c,menu=on \
+-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
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 5b3bd4a..fe2cca4 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2001,6 +2001,8 @@ 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_CRB);
     DO_TEST_PARSE_ERROR("tpm-no-backend-invalid",
                         QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS);
 
-- 
2.5.5

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

I'll change the $subj to be:

qemu: Add tpm-crb QEMU device to the command line

On 04/26/2018 01:42 PM, Stefan Berger wrote:
> Add a test case for the formation of the tpm-crb QEMU device
> command line.

And the commit msg changes to:

Alter qemuBuildTPMDevStr to format the tpm-crb on the command line
and use the enum range checking for valid model.

Add a test case for the formation of the tpm-crb QEMU device
command line. The qemuxml2argvtest changes cannot use the newer
DO_TEST_CAPS_LATEST since building of the command line involves
calling qemuBuildTPMBackendStr which attempts to open the
path to the device (e.g. /dev/tmp0).

> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_command.c                         | 16 ++++++++++++++-
>  tests/qemuxml2argvdata/tpm-passthrough-crb.args | 26 +++++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                        |  2 ++
>  3 files changed, 43 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args
> 

So this does bring up a couple of interesting notes for "additional"
work or checks...

   1. More recent adjustments for libvirt have begun to use the
qemuProcessPrepare* type functions in order to perform similar things
like the open on the /dev/tpm0 in qemuBuildTPMBackendStr. This leaves
only building the command line in the qemu_command.c code. I'm not even
sure this works with the TPM model given how the code passes the tpmfd
to the command line. Still see qemuProcessPrepareChardevDevice (and it's
corollary qemuProcessCleanupChardevDevice). There is another series
upstream with a similar problem, see:

https://www.redhat.com/archives/libvir-list/2018-April/msg01797.html

and in particular patch 2 of the series. That may "solve" this open as well.

   2. Does /dev/tpm0 need to play nicely in the QEMU name space code?
See qemuDomainNamespace{Setup|Teardown}* functions.  I have a feeling it
may, but I'm not 100% sure. Michal Privoznik would be the contact point.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 418729b..198b44e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9387,8 +9387,22 @@ qemuBuildTPMDevStr(const virDomainDef *def,
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      const virDomainTPMDef *tpm = def->tpm;
>      const char *model = virDomainTPMModelTypeToString(tpm->model);
> +    virQEMUCapsFlags flag;
>  
> -    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_TIS)) {
> +    switch (tpm->model) {
> +    case VIR_DOMAIN_TPM_MODEL_TIS:
> +        flag = QEMU_CAPS_DEVICE_TPM_TIS;
> +        break;
> +    case VIR_DOMAIN_TPM_MODEL_CRB:
> +        flag = QEMU_CAPS_DEVICE_TPM_CRB;
> +        break;
> +    case VIR_DOMAIN_TPM_MODEL_LAST:

Added the default: case; otherwise, ... [1]

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unknown TPM device model %d"), tpm->model);

change this to

        virReportEnumRangeError(virDomainTPMModel, tpm->model);

> +        goto error;
> +    }
> +
> +    if (!virQEMUCapsGet(qemuCaps, flag)) {

[1] ... this failed during my build because @flag wouldn't be defined
supposedly ...

>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("The QEMU executable %s does not support TPM "
>                         "model %s"),
> diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
> new file mode 100644
> index 0000000..010495d
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args

syntax-check tells me there's some long lines in here that need to be
wrapped using:

tests/test-wrap-argv.pl -i tests/qemuxml2argvdata/tpm-passthrough-crb.args

I've made the adjustments and will push the series once 4.4.0 opens
(assuming no one else jumps in and comes up with something I've missed).

Reviewed-by: John Ferlan <jferlan@redhat.com>

John



> @@ -0,0 +1,26 @@
> +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 \
> +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \
> +-m 2048 -\
> +smp 1,sockets=1,cores=1,threads=1 \
> +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \
> +-display none \
> +-no-user-config \
> +-nodefaults \
> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,server,nowait \
> +-mon chardev=charmonitor,id=monitor,mode=control \
> +-rtc base=utc \
> +-no-shutdown \
> +-boot order=c,menu=on \
> +-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
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 5b3bd4a..fe2cca4 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2001,6 +2001,8 @@ 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_CRB);
>      DO_TEST_PARSE_ERROR("tpm-no-backend-invalid",
>                          QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS);
>  
> 

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

On 05/01/2018 09:14 AM, John Ferlan wrote:
> I'll change the $subj to be:
>
> qemu: Add tpm-crb QEMU device to the command line
>
> On 04/26/2018 01:42 PM, Stefan Berger wrote:
>> Add a test case for the formation of the tpm-crb QEMU device
>> command line.
> And the commit msg changes to:
>
> Alter qemuBuildTPMDevStr to format the tpm-crb on the command line
> and use the enum range checking for valid model.
>
> Add a test case for the formation of the tpm-crb QEMU device
> command line. The qemuxml2argvtest changes cannot use the newer
> DO_TEST_CAPS_LATEST since building of the command line involves
> calling qemuBuildTPMBackendStr which attempts to open the
> path to the device (e.g. /dev/tmp0).
>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   src/qemu/qemu_command.c                         | 16 ++++++++++++++-
>>   tests/qemuxml2argvdata/tpm-passthrough-crb.args | 26 +++++++++++++++++++++++++
>>   tests/qemuxml2argvtest.c                        |  2 ++
>>   3 files changed, 43 insertions(+), 1 deletion(-)
>>   create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args
>>
> So this does bring up a couple of interesting notes for "additional"
> work or checks...
>
>     1. More recent adjustments for libvirt have begun to use the
> qemuProcessPrepare* type functions in order to perform similar things
> like the open on the /dev/tpm0 in qemuBuildTPMBackendStr. This leaves
> only building the command line in the qemu_command.c code. I'm not even
> sure this works with the TPM model given how the code passes the tpmfd
> to the command line. Still see qemuProcessPrepareChardevDevice (and it's
> corollary qemuProcessCleanupChardevDevice). There is another series
> upstream with a similar problem, see:
>
> https://www.redhat.com/archives/libvir-list/2018-April/msg01797.html
>
> and in particular patch 2 of the series. That may "solve" this open as well.
>
>     2. Does /dev/tpm0 need to play nicely in the QEMU name space code?
> See qemuDomainNamespace{Setup|Teardown}* functions.  I have a feeling it
> may, but I'm not 100% sure. Michal Privoznik would be the contact point.

I just tried it using the passthrough device. I don't have a TPM 2 on my 
machine, so I had to use the TPM 1.2 /dev/tpm0. I had to stop SELinux 
due to the below missing rule but QEMU at least started up. Since TPM 
1.2 +CRB combination isn't typically seen in the field, software doesn't 
support it, but at least QEMU talks to it.

missing SELinux rule: allow svirt_t tpm_device_t:chr_file { read write };

The QEMU command line parameters looked like this:
[...] -tpmdev 
passthrough,id=tpm-tpm0,path=/dev/fdset/1,cancel-path=/dev/fdset/2 
-add-fd set=1,fd=29 -add-fd set=2,fd=30 -device 
tpm-crb,tpmdev=tpm-tpm0,id=tpm0 [...]

This looks ok.

>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 418729b..198b44e 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -9387,8 +9387,22 @@ qemuBuildTPMDevStr(const virDomainDef *def,
>>       virBuffer buf = VIR_BUFFER_INITIALIZER;
>>       const virDomainTPMDef *tpm = def->tpm;
>>       const char *model = virDomainTPMModelTypeToString(tpm->model);
>> +    virQEMUCapsFlags flag;
>>   
>> -    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_TIS)) {
>> +    switch (tpm->model) {
>> +    case VIR_DOMAIN_TPM_MODEL_TIS:
>> +        flag = QEMU_CAPS_DEVICE_TPM_TIS;
>> +        break;
>> +    case VIR_DOMAIN_TPM_MODEL_CRB:
>> +        flag = QEMU_CAPS_DEVICE_TPM_CRB;
>> +        break;
>> +    case VIR_DOMAIN_TPM_MODEL_LAST:
> Added the default: case; otherwise, ... [1]
>
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Unknown TPM device model %d"), tpm->model);
> change this to
>
>          virReportEnumRangeError(virDomainTPMModel, tpm->model);
>
>> +        goto error;
>> +    }
>> +
>> +    if (!virQEMUCapsGet(qemuCaps, flag)) {
> [1] ... this failed during my build because @flag wouldn't be defined
> supposedly ...

I saw that later on, too. FC23 compiler was fine without it, FC27 complains.

>
>>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>                          _("The QEMU executable %s does not support TPM "
>>                          "model %s"),
>> diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
>> new file mode 100644
>> index 0000000..010495d
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args
> syntax-check tells me there's some long lines in here that need to be
> wrapped using:
>
> tests/test-wrap-argv.pl -i tests/qemuxml2argvdata/tpm-passthrough-crb.args
>
> I've made the adjustments and will push the series once 4.4.0 opens
> (assuming no one else jumps in and comes up with something I've missed).

Thank you.
     Stefan

>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
>
>
>> @@ -0,0 +1,26 @@
>> +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 \
>> +-machine pc-i440fx-2.12,accel=tcg,usb=off,dump-guest-core=off \
>> +-m 2048 -\
>> +smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \
>> +-display none \
>> +-no-user-config \
>> +-nodefaults \
>> +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,server,nowait \
>> +-mon chardev=charmonitor,id=monitor,mode=control \
>> +-rtc base=utc \
>> +-no-shutdown \
>> +-boot order=c,menu=on \
>> +-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
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 5b3bd4a..fe2cca4 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2001,6 +2001,8 @@ 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_CRB);
>>       DO_TEST_PARSE_ERROR("tpm-no-backend-invalid",
>>                           QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS);
>>   
>>

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