* Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem
2023-09-27 13:30 [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem Thomas Huth
@ 2023-09-27 19:13 ` Markus Armbruster
2023-09-28 5:28 ` Markus Armbruster
2023-10-04 14:04 ` Juan Quintela
2023-10-05 5:03 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2023-09-27 19:13 UTC (permalink / raw)
To: Thomas Huth
Cc: Yuval Shaia, Marcel Apfelbaum, qemu-devel, Markus Armbruster,
Zhijian Li, Peter Xu, Daniel P . Berrangé, libvir-list,
Juan Quintela, Eric Auger, Fabiano Rosas,
Philippe Mathieu-Daudé, Michael Tokarev
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Wed, Sep 27, 2023 at 12:49:08PM -0400, James Bottomley wrote:
>> From: James Bottomley <James.Bottomley@HansenPartnership.com>
>>
>> The Microsoft Simulator (mssim) is the reference emulation platform
>> for the TCG TPM 2.0 specification.
>>
>> https://github.com/Microsoft/ms-tpm-20-ref.git
>>
>> It exports a fairly simple network socket based protocol on two
>> sockets, one for command (default 2321) and one for control (default
>> 2322). This patch adds a simple backend that can speak the mssim
>> protocol over the network. It also allows the two sockets to be
>> specified on the command line. The benefits are twofold: firstly it
>> gives us a backend that actually speaks a standard TPM emulation
>> protocol instead of the linux specific TPM driver format of the
>> current emulated TPM backend and secondly, using the microsoft
>> protocol, the end point of the emulator can be anywhere on the
>> network, facilitating the cloud use case where a central TPM service
>> can be used over a control network.
>>
>> The implementation does basic control commands like power off/on, but
>> doesn't implement cancellation or startup. The former because
>> cancellation is pretty much useless on a fast operating TPM emulator
>> and the latter because this emulator is designed to be used with OVMF
>> which itself does TPM startup and I wanted to validate that.
>>
>> To run this, simply download an emulator based on the MS specification
>> (package ibmswtpm2 on openSUSE) and run it, then add these two lines
>> to the qemu command and it will use the emulator.
>>
>> -tpmdev mssim,id=tpm0 \
>> -device tpm-crb,tpmdev=tpm0 \
>>
>> to use a remote emulator replace the first line with
>>
>> -tpmdev "{'type':'mssim','id':'tpm0','command':{'type':inet,'host':'remote','port':'2321'}}"
>>
>> tpm-tis also works as the backend.
>>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Acked-by: Markus Armbruster <armbru@redhat.com>
[...]
>> diff --git a/backends/tpm/tpm_mssim.c b/backends/tpm/tpm_mssim.c
>> new file mode 100644
>> index 0000000000..b8a12dce04
>> --- /dev/null
>> +++ b/backends/tpm/tpm_mssim.c
>> @@ -0,0 +1,290 @@
>> +/*
>> + * Emulator TPM driver which connects over the mssim protocol
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * Copyright (c) 2022
>> + * Author: James Bottomley <jejb@linux.ibm.com>
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/sockets.h"
>> +
>> +#include "qapi/clone-visitor.h"
>> +#include "qapi/qapi-visit-tpm.h"
>> +
>> +#include "io/channel-socket.h"
>> +
>> +#include "sysemu/runstate.h"
>> +#include "sysemu/tpm_backend.h"
>> +#include "sysemu/tpm_util.h"
>> +
>> +#include "qom/object.h"
>> +
>> +#include "tpm_int.h"
>> +#include "tpm_mssim.h"
>> +
>> +#define ERROR_PREFIX "TPM mssim Emulator: "
>> +
>> +#define TYPE_TPM_MSSIM "tpm-mssim"
>> +OBJECT_DECLARE_SIMPLE_TYPE(TPMMssim, TPM_MSSIM)
>> +
>> +struct TPMMssim {
>> + TPMBackend parent;
>> +
>> + TPMMssimOptions opts;
>> +
>> + QIOChannelSocket *cmd_qc, *ctrl_qc;
>> +};
>> +
>> +static int tpm_send_ctrl(TPMMssim *t, uint32_t cmd, Error **errp)
>> +{
>> + int ret;
>> +
>> + qio_channel_socket_connect_sync(t->ctrl_qc, t->opts.control, errp);
>
> Need to assign to 'ret' and check for failure here, otherwise the
> next call to write_all will overwrite the useful message in 'errp'
> with a less helpful one.
No, it'll crash :)
An @errp argument must point to a null pointer. If it doesn't, setting
an error will trip error_setv()'s assertion.
> + cmd = htonl(cmd);
> + ret = qio_channel_write_all(QIO_CHANNEL(t->ctrl_qc),
> + (char *)&cmd, sizeof(cmd), errp);
> + if (ret != 0) {
> + goto out;
> + }
qapi/error.h's big comment advises:
* Receive and accumulate multiple errors (first one wins):
* Error *err = NULL, *local_err = NULL;
* foo(arg, &err);
* bar(arg, &local_err);
* error_propagate(&err, local_err);
* if (err) {
* handle the error...
* }
*
* Do *not* "optimize" this to
* Error *err = NULL;
* foo(arg, &err);
* bar(arg, &err); // WRONG!
* if (err) {
* handle the error...
* }
* because this may pass a non-null err to bar().
*
* Likewise, do *not*
* Error *err = NULL;
* if (cond1) {
* error_setg(&err, ...);
* }
* if (cond2) {
* error_setg(&err, ...); // WRONG!
* }
* because this may pass a non-null err to error_setg().
The quoted code is like the last example, except the error_setg() lurk
within the functions called.
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem
2023-09-27 13:30 [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem Thomas Huth
2023-09-27 19:13 ` Markus Armbruster
@ 2023-10-04 14:04 ` Juan Quintela
2023-10-05 5:03 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 5+ messages in thread
From: Juan Quintela @ 2023-10-04 14:04 UTC (permalink / raw)
To: Thomas Huth
Cc: Yuval Shaia, Marcel Apfelbaum, qemu-devel, Markus Armbruster,
Zhijian Li, Peter Xu, Daniel P . Berrangé, libvir-list,
Eric Auger, Fabiano Rosas, Philippe Mathieu-Daudé,
Michael Tokarev
Thomas Huth <thuth@redhat.com> wrote:
> This subsystem is said to be in a bad shape (see e.g. [1], [2]
> and [3]), and nobody seems to feel responsible to pick up patches
> for this and send them via a pull request. For example there is
> a patch for a CVE-worthy bug posted more than half a year ago [4]
> which has never been merged.
>
> Quoting Markus: "Given the shape it is in, I wouldn't let friends
> use it in production" - we shouldn't expose this to our users in
> the current state. Thus let's mark it as deprecated and finally
> remove it unless somebody steps up and improves the code quality
> and adds proper regression tests.
>
> [1] https://lore.kernel.org/qemu-devel/20230918144206.560120-1-armbru@redhat.com/
> [2] https://lore.kernel.org/qemu-devel/ZQnojJOqoFu73995@redhat.com/
> [3] https://lore.kernel.org/qemu-devel/1054981c-e8ae-c676-3b04-eeb030e11f65@tls.msk.ru/
> [4] https://lore.kernel.org/qemu-devel/20230301142926.18686-1-yuval.shaia.ml@gmail.com/
> [5] https://lore.kernel.org/qemu-devel/8734z9f086.fsf@pond.sub.org/
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Acked-by: Juan Quintela <quintela@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem
2023-09-27 13:30 [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem Thomas Huth
2023-09-27 19:13 ` Markus Armbruster
2023-10-04 14:04 ` Juan Quintela
@ 2023-10-05 5:03 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-05 5:03 UTC (permalink / raw)
To: Thomas Huth, Yuval Shaia, Marcel Apfelbaum, qemu-devel
Cc: Markus Armbruster, Zhijian Li, Peter Xu, Daniel P . Berrangé,
libvir-list, Juan Quintela, Eric Auger, Fabiano Rosas,
Michael Tokarev
On 27/9/23 15:30, Thomas Huth wrote:
> This subsystem is said to be in a bad shape (see e.g. [1], [2]
> and [3]), and nobody seems to feel responsible to pick up patches
> for this and send them via a pull request. For example there is
> a patch for a CVE-worthy bug posted more than half a year ago [4]
> which has never been merged.
>
> Quoting Markus: "Given the shape it is in, I wouldn't let friends
> use it in production" - we shouldn't expose this to our users in
> the current state. Thus let's mark it as deprecated and finally
> remove it unless somebody steps up and improves the code quality
> and adds proper regression tests.
>
> [1] https://lore.kernel.org/qemu-devel/20230918144206.560120-1-armbru@redhat.com/
> [2] https://lore.kernel.org/qemu-devel/ZQnojJOqoFu73995@redhat.com/
> [3] https://lore.kernel.org/qemu-devel/1054981c-e8ae-c676-3b04-eeb030e11f65@tls.msk.ru/
> [4] https://lore.kernel.org/qemu-devel/20230301142926.18686-1-yuval.shaia.ml@gmail.com/
> [5] https://lore.kernel.org/qemu-devel/8734z9f086.fsf@pond.sub.org/
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> MAINTAINERS | 2 +-
> docs/about/deprecated.rst | 8 ++++++++
> hw/rdma/vmw/pvrdma_main.c | 2 ++
> 3 files changed, 11 insertions(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 5+ messages in thread