[PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem

[PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem

[Thomas Huth]

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(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 355b1960ce..ca42b89ef8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3815,7 +3815,7 @@ F: docs/block-replication.txt
 PVRDMA
 M: Yuval Shaia <yuval.shaia.ml@gmail.com>
 M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
-S: Maintained
+S: Odd Fixes
 F: hw/rdma/*
 F: hw/rdma/vmw/*
 F: docs/pvrdma.txt
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index dc4da95329..f0c7addb1f 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -365,6 +365,14 @@ QEMU's ``vhost`` feature, which would eliminate the high latency costs under
 which the 9p ``proxy`` backend currently suffers. However as of to date nobody
 has indicated plans for such kind of reimplementation unfortunately.
 
+``-device pvrdma`` and the rdma subsystem (since 8.2)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The pvrdma device and the whole rdma subsystem are in a bad shape and
+without active maintenance. The QEMU project intends to remove this
+device and subsystem from the code base in a future release without
+replacement unless somebody steps up and improves the situation.
+
 
 Block device options
 ''''''''''''''''''''
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 4fc6712025..ed49ce1e72 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -601,6 +601,8 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
     bool ram_shared = false;
     PCIDevice *func0;
 
+    warn_report_once("pvrdma is deprecated and will be removed in a future release");
+
     rdma_info_report("Initializing device %s %x.%x", pdev->name,
                      PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
 
-- 
2.41.0

Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem

[Markus Armbruster]

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.

[...]

Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem

[Markus Armbruster]

Wrong thread, please ignore.

Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem

[Juan Quintela]

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>

Re: [PATCH] hw/rdma: Deprecate the pvrdma device and the rdma subsystem

[Philippe Mathieu-Daudé]

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>