[PATCH v3 0/5] Add a vhost RPMsg API

[PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

v3:
- address several checkpatch warnings
- address comments from Mathieu Poirier

v2:
- update patch #5 with a correct vhost_dev_init() prototype
- drop patch #6 - it depends on a different patch, that is currently
  an RFC
- address comments from Pierre-Louis Bossart:
  * remove "default n" from Kconfig

Linux supports RPMsg over VirtIO for "remote processor" /AMP use
cases. It can however also be used for virtualisation scenarios,
e.g. when using KVM to run Linux on both the host and the guests.
This patch set adds a wrapper API to facilitate writing vhost
drivers for such RPMsg-based solutions. The first use case is an
audio DSP virtualisation project, currently under development, ready
for review and submission, available at
https://github.com/thesofproject/linux/pull/1501/commits
A further patch for the ADSP vhost RPMsg driver will be sent
separately for review only since it cannot be merged without audio
patches being upstreamed first.

Thanks
Guennadi

Guennadi Liakhovetski (5):
  vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
  vhost: (cosmetic) remove a superfluous variable initialisation
  rpmsg: move common structures and defines to headers
  rpmsg: update documentation
  vhost: add an RPMsg API

 Documentation/rpmsg.txt          |   6 +-
 drivers/rpmsg/virtio_rpmsg_bus.c |  78 +-------
 drivers/vhost/Kconfig            |   7 +
 drivers/vhost/Makefile           |   3 +
 drivers/vhost/rpmsg.c            | 382 +++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.c            |   2 +-
 drivers/vhost/vhost_rpmsg.h      |  74 ++++++++
 include/linux/virtio_rpmsg.h     |  81 +++++++++
 include/uapi/linux/rpmsg.h       |   3 +
 include/uapi/linux/vhost.h       |   4 +-
 10 files changed, 559 insertions(+), 81 deletions(-)
 create mode 100644 drivers/vhost/rpmsg.c
 create mode 100644 drivers/vhost/vhost_rpmsg.h
 create mode 100644 include/linux/virtio_rpmsg.h

-- 
1.9.3

[PATCH v3 1/5] vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl

[Guennadi Liakhovetski]

VHOST_VSOCK_SET_RUNNING is used by the vhost vsock driver to perform
crucial VirtQueue initialisation, like assigning .private fields and
calling vhost_vq_init_access(), and clean up. However, this ioctl is
actually extremely useful for any vhost driver, that doesn't have a
side channel to inform it of a status change, e.g. upon a guest
reboot. This patch makes that ioctl generic, while preserving its
numeric value and also keeping the original alias.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 include/uapi/linux/vhost.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index 9fe72e4..b54af9d 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -93,6 +93,8 @@
 #define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64)
 #define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64)
 
+#define VHOST_SET_RUNNING _IOW(VHOST_VIRTIO, 0x61, int)
+
 /* VHOST_NET specific defines */
 
 /* Attach virtio net ring to a raw socket, or tap device.
@@ -114,7 +116,7 @@
 /* VHOST_VSOCK specific defines */
 
 #define VHOST_VSOCK_SET_GUEST_CID	_IOW(VHOST_VIRTIO, 0x60, __u64)
-#define VHOST_VSOCK_SET_RUNNING		_IOW(VHOST_VIRTIO, 0x61, int)
+#define VHOST_VSOCK_SET_RUNNING		VHOST_SET_RUNNING
 
 /* VHOST_VDPA specific defines */
 
-- 
1.9.3

[PATCH v3 2/5] vhost: (cosmetic) remove a superfluous variable initialisation

[Guennadi Liakhovetski]

Even the compiler is able to figure out that in this case the
initialisation is superfluous.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 drivers/vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2f4383bb..2b9ad8a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -895,7 +895,7 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
 
 #define vhost_put_user(vq, x, ptr)		\
 ({ \
-	int ret = -EFAULT; \
+	int ret; \
 	if (!vq->iotlb) { \
 		ret = __put_user(x, ptr); \
 	} else { \
-- 
1.9.3

[PATCH v3 3/5] rpmsg: move common structures and defines to headers

[Guennadi Liakhovetski]

virtio_rpmsg_bus.c keeps RPMsg protocol structure declarations and
common defines like the ones, needed for name-space announcements,
internal. Move them to common headers instead.

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-------------------------------------
 include/linux/virtio_rpmsg.h     | 81 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/rpmsg.h       |  3 ++
 3 files changed, 86 insertions(+), 76 deletions(-)
 create mode 100644 include/linux/virtio_rpmsg.h

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 07d4f33..f3bd050 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -25,7 +25,9 @@
 #include <linux/virtio.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
+#include <linux/virtio_rpmsg.h>
 #include <linux/wait.h>
+#include <uapi/linux/rpmsg.h>
 
 #include "rpmsg_internal.h"
 
@@ -69,58 +71,6 @@ struct virtproc_info {
 	struct rpmsg_endpoint *ns_ept;
 };
 
-/* The feature bitmap for virtio rpmsg */
-#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
-
-/**
- * struct rpmsg_hdr - common header for all rpmsg messages
- * @src: source address
- * @dst: destination address
- * @reserved: reserved for future use
- * @len: length of payload (in bytes)
- * @flags: message flags
- * @data: @len bytes of message payload data
- *
- * Every message sent(/received) on the rpmsg bus begins with this header.
- */
-struct rpmsg_hdr {
-	u32 src;
-	u32 dst;
-	u32 reserved;
-	u16 len;
-	u16 flags;
-	u8 data[];
-} __packed;
-
-/**
- * struct rpmsg_ns_msg - dynamic name service announcement message
- * @name: name of remote service that is published
- * @addr: address of remote service that is published
- * @flags: indicates whether service is created or destroyed
- *
- * This message is sent across to publish a new service, or announce
- * about its removal. When we receive these messages, an appropriate
- * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
- * or ->remove() handler of the appropriate rpmsg driver will be invoked
- * (if/as-soon-as one is registered).
- */
-struct rpmsg_ns_msg {
-	char name[RPMSG_NAME_SIZE];
-	u32 addr;
-	u32 flags;
-} __packed;
-
-/**
- * enum rpmsg_ns_flags - dynamic name service announcement flags
- *
- * @RPMSG_NS_CREATE: a new remote service was just created
- * @RPMSG_NS_DESTROY: a known remote service was just destroyed
- */
-enum rpmsg_ns_flags {
-	RPMSG_NS_CREATE		= 0,
-	RPMSG_NS_DESTROY	= 1,
-};
-
 /**
  * @vrp: the remote processor this channel belongs to
  */
@@ -134,36 +84,12 @@ struct virtio_rpmsg_channel {
 	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
 
 /*
- * We're allocating buffers of 512 bytes each for communications. The
- * number of buffers will be computed from the number of buffers supported
- * by the vring, upto a maximum of 512 buffers (256 in each direction).
- *
- * Each buffer will have 16 bytes for the msg header and 496 bytes for
- * the payload.
- *
- * This will utilize a maximum total space of 256KB for the buffers.
- *
- * We might also want to add support for user-provided buffers in time.
- * This will allow bigger buffer size flexibility, and can also be used
- * to achieve zero-copy messaging.
- *
- * Note that these numbers are purely a decision of this driver - we
- * can change this without changing anything in the firmware of the remote
- * processor.
- */
-#define MAX_RPMSG_NUM_BUFS	(512)
-#define MAX_RPMSG_BUF_SIZE	(512)
-
-/*
  * Local addresses are dynamically allocated on-demand.
  * We do not dynamically assign addresses from the low 1024 range,
  * in order to reserve that address range for predefined services.
  */
 #define RPMSG_RESERVED_ADDRESSES	(1024)
 
-/* Address 53 is reserved for advertising remote services */
-#define RPMSG_NS_ADDR			(53)
-
 static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept);
 static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
 static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
diff --git a/include/linux/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
new file mode 100644
index 00000000..679be8b
--- /dev/null
+++ b/include/linux/virtio_rpmsg.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_VIRTIO_RPMSG_H
+#define _LINUX_VIRTIO_RPMSG_H
+
+#include <linux/mod_devicetable.h>
+
+/**
+ * struct rpmsg_hdr - common header for all rpmsg messages
+ * @src: source address
+ * @dst: destination address
+ * @reserved: reserved for future use
+ * @len: length of payload (in bytes)
+ * @flags: message flags
+ * @data: @len bytes of message payload data
+ *
+ * Every message sent(/received) on the rpmsg bus begins with this header.
+ */
+struct rpmsg_hdr {
+	u32 src;
+	u32 dst;
+	u32 reserved;
+	u16 len;
+	u16 flags;
+	u8 data[];
+} __packed;
+
+/**
+ * struct rpmsg_ns_msg - dynamic name service announcement message
+ * @name: name of remote service that is published
+ * @addr: address of remote service that is published
+ * @flags: indicates whether service is created or destroyed
+ *
+ * This message is sent across to publish a new service, or announce
+ * about its removal. When we receive these messages, an appropriate
+ * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
+ * or ->remove() handler of the appropriate rpmsg driver will be invoked
+ * (if/as-soon-as one is registered).
+ */
+struct rpmsg_ns_msg {
+	char name[RPMSG_NAME_SIZE];
+	u32 addr;
+	u32 flags;
+} __packed;
+
+/**
+ * enum rpmsg_ns_flags - dynamic name service announcement flags
+ *
+ * @RPMSG_NS_CREATE: a new remote service was just created
+ * @RPMSG_NS_DESTROY: a known remote service was just destroyed
+ */
+enum rpmsg_ns_flags {
+	RPMSG_NS_CREATE		= 0,
+	RPMSG_NS_DESTROY	= 1,
+};
+
+/*
+ * We're allocating buffers of 512 bytes each for communications. The
+ * number of buffers will be computed from the number of buffers supported
+ * by the vring, upto a maximum of 512 buffers (256 in each direction).
+ *
+ * Each buffer will have 16 bytes for the msg header and 496 bytes for
+ * the payload.
+ *
+ * This will utilize a maximum total space of 256KB for the buffers.
+ *
+ * We might also want to add support for user-provided buffers in time.
+ * This will allow bigger buffer size flexibility, and can also be used
+ * to achieve zero-copy messaging.
+ *
+ * Note that these numbers are purely a decision of this driver - we
+ * can change this without changing anything in the firmware of the remote
+ * processor.
+ */
+#define MAX_RPMSG_NUM_BUFS	512
+#define MAX_RPMSG_BUF_SIZE	512
+
+/* Address 53 is reserved for advertising remote services */
+#define RPMSG_NS_ADDR		53
+
+#endif
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6da..d669c04 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -24,4 +24,7 @@ struct rpmsg_endpoint_info {
 #define RPMSG_CREATE_EPT_IOCTL	_IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
+/* The feature bitmap for virtio rpmsg */
+#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
+
 #endif
-- 
1.9.3

[PATCH v3 4/5] rpmsg: update documentation

[Guennadi Liakhovetski]

rpmsg_create_ept() takes struct rpmsg_channel_info chinfo as its last
argument, not a u32 value. The first two arguments are also updated.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 Documentation/rpmsg.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/rpmsg.txt b/Documentation/rpmsg.txt
index 24b7a9e..1ce353c 100644
--- a/Documentation/rpmsg.txt
+++ b/Documentation/rpmsg.txt
@@ -192,9 +192,9 @@ Returns 0 on success and an appropriate error value on failure.
 
 ::
 
-  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_channel *rpdev,
-		void (*cb)(struct rpmsg_channel *, void *, int, void *, u32),
-		void *priv, u32 addr);
+  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
+					  rpmsg_rx_cb_t cb, void *priv,
+					  struct rpmsg_channel_info chinfo);
 
 every rpmsg address in the system is bound to an rx callback (so when
 inbound messages arrive, they are dispatched by the rpmsg bus using the
-- 
1.9.3

Re: [PATCH v3 4/5] rpmsg: update documentation

[Mathieu Poirier]

On Wed, 27 May 2020 at 12:05, Guennadi Liakhovetski
<guennadi.liakhovetski@linux.intel.com> wrote:
>
> rpmsg_create_ept() takes struct rpmsg_channel_info chinfo as its last
> argument, not a u32 value. The first two arguments are also updated.
>
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> ---
>  Documentation/rpmsg.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rpmsg.txt b/Documentation/rpmsg.txt
> index 24b7a9e..1ce353c 100644
> --- a/Documentation/rpmsg.txt
> +++ b/Documentation/rpmsg.txt
> @@ -192,9 +192,9 @@ Returns 0 on success and an appropriate error value on failure.
>
>  ::
>
> -  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_channel *rpdev,
> -               void (*cb)(struct rpmsg_channel *, void *, int, void *, u32),
> -               void *priv, u32 addr);
> +  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
> +                                         rpmsg_rx_cb_t cb, void *priv,
> +                                         struct rpmsg_channel_info chinfo);
>

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>  every rpmsg address in the system is bound to an rx callback (so when
>  inbound messages arrive, they are dispatched by the rpmsg bus using the
> --
> 1.9.3
>

[PATCH v3 5/5] vhost: add an RPMsg API

[Guennadi Liakhovetski]

Linux supports running the RPMsg protocol over the VirtIO transport
protocol, but currently there is only support for VirtIO clients and
no support for a VirtIO server. This patch adds a vhost-based RPMsg
server implementation.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 drivers/vhost/Kconfig       |   7 +
 drivers/vhost/Makefile      |   3 +
 drivers/vhost/rpmsg.c       | 382 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost_rpmsg.h |  74 +++++++++
 4 files changed, 466 insertions(+)
 create mode 100644 drivers/vhost/rpmsg.c
 create mode 100644 drivers/vhost/vhost_rpmsg.h

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 2c75d16..8b91f3e 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -38,6 +38,13 @@ config VHOST_NET
 	  To compile this driver as a module, choose M here: the module will
 	  be called vhost_net.
 
+config VHOST_RPMSG
+	tristate
+	depends on VHOST
+	help
+	  Vhost RPMsg API allows vhost drivers to communicate with VirtIO
+	  drivers, using the RPMsg over VirtIO protocol.
+
 config VHOST_SCSI
 	tristate "VHOST_SCSI TCM fabric driver"
 	depends on TARGET_CORE && EVENTFD
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
index f3e1897..9cf459d 100644
--- a/drivers/vhost/Makefile
+++ b/drivers/vhost/Makefile
@@ -2,6 +2,9 @@
 obj-$(CONFIG_VHOST_NET) += vhost_net.o
 vhost_net-y := net.o
 
+obj-$(CONFIG_VHOST_RPMSG) += vhost_rpmsg.o
+vhost_rpmsg-y := rpmsg.o
+
 obj-$(CONFIG_VHOST_SCSI) += vhost_scsi.o
 vhost_scsi-y := scsi.o
 
diff --git a/drivers/vhost/rpmsg.c b/drivers/vhost/rpmsg.c
new file mode 100644
index 00000000..ea77e1f
--- /dev/null
+++ b/drivers/vhost/rpmsg.c
@@ -0,0 +1,382 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright(c) 2020 Intel Corporation. All rights reserved.
+ *
+ * Author: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
+ *
+ * Vhost RPMsg VirtIO interface. It provides a set of functions to match the
+ * guest side RPMsg VirtIO API, provided by drivers/rpmsg/virtio_rpmsg_bus.c
+ * These functions handle creation of 2 virtual queues, handling of endpoint
+ * addresses, sending a name-space announcement to the guest as well as any
+ * user messages. This API can be used by any vhost driver to handle RPMsg
+ * specific processing.
+ * Specific vhost drivers, using this API will use their own VirtIO device
+ * IDs, that should then also be added to the ID table in virtio_rpmsg_bus.c
+ */
+
+#include <linux/compat.h>
+#include <linux/file.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/vhost.h>
+#include <linux/virtio_rpmsg.h>
+#include <uapi/linux/rpmsg.h>
+
+#include "vhost.h"
+#include "vhost_rpmsg.h"
+
+/*
+ * All virtio-rpmsg virtual queue kicks always come with just one buffer -
+ * either input or output
+ */
+static int vhost_rpmsg_get_single(struct vhost_virtqueue *vq)
+{
+	struct vhost_rpmsg *vr = container_of(vq->dev, struct vhost_rpmsg, dev);
+	unsigned int out, in;
+	int head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+				     &out, &in, NULL, NULL);
+	if (head < 0) {
+		vq_err(vq, "%s(): error %d getting buffer\n",
+		       __func__, head);
+		return head;
+	}
+
+	/* Nothing new? */
+	if (head == vq->num)
+		return head;
+
+	if (vq == &vr->vq[VIRTIO_RPMSG_RESPONSE] && (out || in != 1)) {
+		vq_err(vq,
+		       "%s(): invalid %d input and %d output in response queue\n",
+		       __func__, in, out);
+		goto return_buf;
+	}
+
+	if (vq == &vr->vq[VIRTIO_RPMSG_REQUEST] && (in || out != 1)) {
+		vq_err(vq,
+		       "%s(): invalid %d input and %d output in request queue\n",
+		       __func__, in, out);
+		goto return_buf;
+	}
+
+	return head;
+
+return_buf:
+	/*
+	 * FIXME: might need to return the buffer using vhost_add_used()
+	 * or vhost_discard_vq_desc(). vhost_discard_vq_desc() is
+	 * described as "being useful for error handling," but it makes
+	 * the thus discarded buffers "unseen," so next time we look we
+	 * retrieve them again?
+	 */
+	return -EINVAL;
+}
+
+static const struct vhost_rpmsg_ept *vhost_rpmsg_ept_find(
+					struct vhost_rpmsg *vr, int addr)
+{
+	unsigned int i;
+
+	for (i = 0; i < vr->n_epts; i++)
+		if (vr->ept[i].addr == addr)
+			return vr->ept + i;
+
+	return NULL;
+}
+
+/*
+ * if len < 0, then for reading a request, the complete virtual queue buffer
+ * size is prepared, for sending a response, the length in the iterator is used
+ */
+int vhost_rpmsg_start_lock(struct vhost_rpmsg *vr,
+			   struct vhost_rpmsg_iter *iter,
+			   unsigned int qid, ssize_t len)
+	__acquires(vq->mutex)
+{
+	struct vhost_virtqueue *vq = vr->vq + qid;
+	size_t tmp;
+
+	if (qid >= VIRTIO_RPMSG_NUM_OF_VQS)
+		return -EINVAL;
+
+	iter->vq = vq;
+
+	mutex_lock(&vq->mutex);
+	vhost_disable_notify(&vr->dev, vq);
+
+	iter->head = vhost_rpmsg_get_single(vq);
+	if (iter->head == vq->num)
+		iter->head = -EAGAIN;
+
+	if (iter->head < 0)
+		goto unlock;
+
+	tmp = vq->iov[0].iov_len;
+	if (tmp < sizeof(iter->rhdr)) {
+		vq_err(vq, "%s(): size %zu too small\n", __func__, tmp);
+		iter->head = -ENOBUFS;
+		goto return_buf;
+	}
+
+	switch (qid) {
+	case VIRTIO_RPMSG_REQUEST:
+		if (len < 0) {
+			len = tmp - sizeof(iter->rhdr);
+		} else if (tmp < sizeof(iter->rhdr) + len) {
+			iter->head = -ENOBUFS;
+			goto return_buf;
+		}
+
+		/* len is now the size of the payload */
+		iov_iter_init(&iter->iov_iter, WRITE,
+			      vq->iov, 1, sizeof(iter->rhdr) + len);
+
+		/* Read the RPMSG header with endpoint addresses */
+		tmp = copy_from_iter(&iter->rhdr, sizeof(iter->rhdr),
+				     &iter->iov_iter);
+		if (tmp != sizeof(iter->rhdr)) {
+			vq_err(vq, "%s(): got %zu instead of %zu\n", __func__,
+			       tmp, sizeof(iter->rhdr));
+			iter->head = -EIO;
+			goto return_buf;
+		}
+
+		iter->ept = vhost_rpmsg_ept_find(vr, iter->rhdr.dst);
+		if (!iter->ept) {
+			vq_err(vq, "%s(): no endpoint with address %d\n",
+			       __func__, iter->rhdr.dst);
+			iter->head = -ENOENT;
+			goto return_buf;
+		}
+
+		/* Let the endpoint read the payload */
+		if (iter->ept->read) {
+			ssize_t ret = iter->ept->read(vr, iter);
+
+			if (ret < 0) {
+				iter->head = ret;
+				goto return_buf;
+			}
+
+			iter->rhdr.len = ret;
+		} else {
+			iter->rhdr.len = 0;
+		}
+
+		/* Prepare for the response phase */
+		iter->rhdr.dst = iter->rhdr.src;
+		iter->rhdr.src = iter->ept->addr;
+
+		break;
+	case VIRTIO_RPMSG_RESPONSE:
+		if (!iter->ept && iter->rhdr.dst != RPMSG_NS_ADDR) {
+			/*
+			 * Usually the iterator is configured when processing a
+			 * message on the request queue, but it's also possible
+			 * to send a message on the response queue without a
+			 * preceding request, in that case the iterator must
+			 * contain source and destination addresses.
+			 */
+			iter->ept = vhost_rpmsg_ept_find(vr, iter->rhdr.src);
+			if (!iter->ept) {
+				iter->head = -ENOENT;
+				goto return_buf;
+			}
+		}
+
+		if (len < 0) {
+			len = tmp - sizeof(iter->rhdr);
+		} else if (tmp < sizeof(iter->rhdr) + len) {
+			iter->head = -ENOBUFS;
+			goto return_buf;
+		} else {
+			iter->rhdr.len = len;
+		}
+
+		/* len is now the size of the payload */
+		iov_iter_init(&iter->iov_iter, READ,
+			      vq->iov, 1, sizeof(iter->rhdr) + len);
+
+		/* Write the RPMSG header with endpoint addresses */
+		tmp = copy_to_iter(&iter->rhdr, sizeof(iter->rhdr),
+				   &iter->iov_iter);
+		if (tmp != sizeof(iter->rhdr)) {
+			iter->head = -EIO;
+			goto return_buf;
+		}
+
+		/* Let the endpoint write the payload */
+		if (iter->ept && iter->ept->write) {
+			ssize_t ret = iter->ept->write(vr, iter);
+
+			if (ret < 0) {
+				iter->head = ret;
+				goto return_buf;
+			}
+		}
+
+		break;
+	}
+
+	return 0;
+
+return_buf:
+	/*
+	 * FIXME: vhost_discard_vq_desc() or vhost_add_used(), see comment in
+	 * vhost_rpmsg_get_single()
+	 */
+unlock:
+	vhost_enable_notify(&vr->dev, vq);
+	mutex_unlock(&vq->mutex);
+
+	return iter->head;
+}
+EXPORT_SYMBOL_GPL(vhost_rpmsg_start_lock);
+
+size_t vhost_rpmsg_copy(struct vhost_rpmsg *vr, struct vhost_rpmsg_iter *iter,
+			void *data, size_t size)
+{
+	/*
+	 * We could check for excess data, but copy_{to,from}_iter() don't do
+	 * that either
+	 */
+	if (iter->vq == vr->vq + VIRTIO_RPMSG_RESPONSE)
+		return copy_to_iter(data, size, &iter->iov_iter);
+
+	return copy_from_iter(data, size, &iter->iov_iter);
+}
+EXPORT_SYMBOL_GPL(vhost_rpmsg_copy);
+
+int vhost_rpmsg_finish_unlock(struct vhost_rpmsg *vr,
+			      struct vhost_rpmsg_iter *iter)
+	__releases(vq->mutex)
+{
+	if (iter->head >= 0)
+		vhost_add_used_and_signal(iter->vq->dev, iter->vq, iter->head,
+					  iter->rhdr.len + sizeof(iter->rhdr));
+
+	vhost_enable_notify(&vr->dev, iter->vq);
+	mutex_unlock(&iter->vq->mutex);
+
+	return iter->head;
+}
+EXPORT_SYMBOL_GPL(vhost_rpmsg_finish_unlock);
+
+/*
+ * Return false to terminate the external loop only if we fail to obtain either
+ * a request or a response buffer
+ */
+static bool handle_rpmsg_req_single(struct vhost_rpmsg *vr,
+				    struct vhost_virtqueue *vq)
+{
+	struct vhost_rpmsg_iter iter;
+	int ret = vhost_rpmsg_start_lock(vr, &iter, VIRTIO_RPMSG_REQUEST,
+					 -EINVAL);
+	if (!ret)
+		ret = vhost_rpmsg_finish_unlock(vr, &iter);
+	if (ret < 0) {
+		if (ret != -EAGAIN)
+			vq_err(vq, "%s(): RPMSG processing failed %d\n",
+			       __func__, ret);
+		return false;
+	}
+
+	if (!iter.ept->write)
+		return true;
+
+	ret = vhost_rpmsg_start_lock(vr, &iter, VIRTIO_RPMSG_RESPONSE,
+				     -EINVAL);
+	if (!ret)
+		ret = vhost_rpmsg_finish_unlock(vr, &iter);
+	if (ret < 0) {
+		vq_err(vq, "%s(): RPMSG finalising failed %d\n", __func__, ret);
+		return false;
+	}
+
+	return true;
+}
+
+static void handle_rpmsg_req_kick(struct vhost_work *work)
+{
+	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
+						  poll.work);
+	struct vhost_rpmsg *vr = container_of(vq->dev, struct vhost_rpmsg, dev);
+
+	while (handle_rpmsg_req_single(vr, vq))
+		;
+}
+
+/*
+ * initialise two virtqueues with an array of endpoints,
+ * request and response callbacks
+ */
+void vhost_rpmsg_init(struct vhost_rpmsg *vr, const struct vhost_rpmsg_ept *ept,
+		      unsigned int n_epts)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(vr->vq); i++)
+		vr->vq_p[i] = &vr->vq[i];
+
+	/* vq[0]: host -> guest, vq[1]: host <- guest */
+	vr->vq[VIRTIO_RPMSG_REQUEST].handle_kick = handle_rpmsg_req_kick;
+	vr->vq[VIRTIO_RPMSG_RESPONSE].handle_kick = NULL;
+
+	vr->ept = ept;
+	vr->n_epts = n_epts;
+
+	vhost_dev_init(&vr->dev, vr->vq_p, VIRTIO_RPMSG_NUM_OF_VQS,
+		       UIO_MAXIOV, 0, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(vhost_rpmsg_init);
+
+void vhost_rpmsg_destroy(struct vhost_rpmsg *vr)
+{
+	if (vhost_dev_has_owner(&vr->dev))
+		vhost_poll_flush(&vr->vq[VIRTIO_RPMSG_REQUEST].poll);
+
+	vhost_dev_cleanup(&vr->dev);
+}
+EXPORT_SYMBOL_GPL(vhost_rpmsg_destroy);
+
+/* send namespace */
+int vhost_rpmsg_ns_announce(struct vhost_rpmsg *vr, const char *name,
+			    unsigned int src)
+{
+	struct vhost_rpmsg_iter iter = {
+		.rhdr = {
+			.src = 0,
+			.dst = RPMSG_NS_ADDR,
+			.flags = RPMSG_NS_CREATE, /* rpmsg_recv_single() */
+		},
+	};
+	struct rpmsg_ns_msg ns = {
+		.addr = src,
+		.flags = RPMSG_NS_CREATE, /* for rpmsg_ns_cb() */
+	};
+	int ret = vhost_rpmsg_start_lock(vr, &iter, VIRTIO_RPMSG_RESPONSE,
+					 sizeof(ns));
+
+	if (ret < 0)
+		return ret;
+
+	strlcpy(ns.name, name, sizeof(ns.name));
+
+	ret = vhost_rpmsg_copy(vr, &iter, &ns, sizeof(ns));
+	if (ret != sizeof(ns))
+		vq_err(iter.vq, "%s(): added %d instead of %zu bytes\n",
+		       __func__, ret, sizeof(ns));
+
+	ret = vhost_rpmsg_finish_unlock(vr, &iter);
+	if (ret < 0)
+		vq_err(iter.vq, "%s(): namespace announcement failed: %d\n",
+		       __func__, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vhost_rpmsg_ns_announce);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Intel, Inc.");
+MODULE_DESCRIPTION("Vhost RPMsg API");
diff --git a/drivers/vhost/vhost_rpmsg.h b/drivers/vhost/vhost_rpmsg.h
new file mode 100644
index 00000000..a3d0dda
--- /dev/null
+++ b/drivers/vhost/vhost_rpmsg.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright(c) 2020 Intel Corporation. All rights reserved.
+ *
+ * Author: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
+ */
+
+#ifndef VHOST_RPMSG_H
+#define VHOST_RPMSG_H
+
+#include <linux/uio.h>
+#include <linux/virtio_rpmsg.h>
+
+#include "vhost.h"
+
+/* RPMsg uses two VirtQueues: one for each direction */
+enum {
+	VIRTIO_RPMSG_RESPONSE,	/* RPMsg response (host->guest) buffers */
+	VIRTIO_RPMSG_REQUEST,	/* RPMsg request (guest->host) buffers */
+	/* Keep last */
+	VIRTIO_RPMSG_NUM_OF_VQS,
+};
+
+struct vhost_rpmsg_ept;
+
+struct vhost_rpmsg_iter {
+	struct iov_iter iov_iter;
+	struct rpmsg_hdr rhdr;
+	struct vhost_virtqueue *vq;
+	const struct vhost_rpmsg_ept *ept;
+	int head;
+	void *priv;
+};
+
+struct vhost_rpmsg {
+	struct vhost_dev dev;
+	struct vhost_virtqueue vq[VIRTIO_RPMSG_NUM_OF_VQS];
+	struct vhost_virtqueue *vq_p[VIRTIO_RPMSG_NUM_OF_VQS];
+	const struct vhost_rpmsg_ept *ept;
+	unsigned int n_epts;
+};
+
+struct vhost_rpmsg_ept {
+	ssize_t (*read)(struct vhost_rpmsg *, struct vhost_rpmsg_iter *);
+	ssize_t (*write)(struct vhost_rpmsg *, struct vhost_rpmsg_iter *);
+	int addr;
+};
+
+static inline size_t vhost_rpmsg_iter_len(const struct vhost_rpmsg_iter *iter)
+{
+	return iter->rhdr.len;
+}
+
+#define VHOST_RPMSG_ITER(_src, _dst) {	\
+	.rhdr = {			\
+			.src = _src,	\
+			.dst = _dst,	\
+		},			\
+	}
+
+void vhost_rpmsg_init(struct vhost_rpmsg *vr, const struct vhost_rpmsg_ept *ept,
+		      unsigned int n_epts);
+void vhost_rpmsg_destroy(struct vhost_rpmsg *vr);
+int vhost_rpmsg_ns_announce(struct vhost_rpmsg *vr, const char *name,
+			    unsigned int src);
+int vhost_rpmsg_start_lock(struct vhost_rpmsg *vr,
+			   struct vhost_rpmsg_iter *iter,
+			   unsigned int qid, ssize_t len);
+size_t vhost_rpmsg_copy(struct vhost_rpmsg *vr, struct vhost_rpmsg_iter *iter,
+			void *data, size_t size);
+int vhost_rpmsg_finish_unlock(struct vhost_rpmsg *vr,
+			      struct vhost_rpmsg_iter *iter);
+
+#endif
-- 
1.9.3

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Mathieu Poirier]

On Wed, 27 May 2020 at 12:05, Guennadi Liakhovetski
<guennadi.liakhovetski@linux.intel.com> wrote:
>
> Linux supports running the RPMsg protocol over the VirtIO transport
> protocol, but currently there is only support for VirtIO clients and
> no support for a VirtIO server. This patch adds a vhost-based RPMsg
> server implementation.
>
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> ---
>  drivers/vhost/Kconfig       |   7 +
>  drivers/vhost/Makefile      |   3 +
>  drivers/vhost/rpmsg.c       | 382 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost_rpmsg.h |  74 +++++++++
>  4 files changed, 466 insertions(+)
>  create mode 100644 drivers/vhost/rpmsg.c
>  create mode 100644 drivers/vhost/vhost_rpmsg.h
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 2c75d16..8b91f3e 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -38,6 +38,13 @@ config VHOST_NET
>           To compile this driver as a module, choose M here: the module will
>           be called vhost_net.
>
> +config VHOST_RPMSG
> +       tristate
> +       depends on VHOST
> +       help
> +         Vhost RPMsg API allows vhost drivers to communicate with VirtIO
> +         drivers, using the RPMsg over VirtIO protocol.
> +
>  config VHOST_SCSI
>         tristate "VHOST_SCSI TCM fabric driver"
>         depends on TARGET_CORE && EVENTFD
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index f3e1897..9cf459d 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -2,6 +2,9 @@
>  obj-$(CONFIG_VHOST_NET) += vhost_net.o
>  vhost_net-y := net.o
>
> +obj-$(CONFIG_VHOST_RPMSG) += vhost_rpmsg.o
> +vhost_rpmsg-y := rpmsg.o
> +
>  obj-$(CONFIG_VHOST_SCSI) += vhost_scsi.o
>  vhost_scsi-y := scsi.o
>
> diff --git a/drivers/vhost/rpmsg.c b/drivers/vhost/rpmsg.c
> new file mode 100644
> index 00000000..ea77e1f
> --- /dev/null
> +++ b/drivers/vhost/rpmsg.c
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright(c) 2020 Intel Corporation. All rights reserved.
> + *
> + * Author: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> + *
> + * Vhost RPMsg VirtIO interface. It provides a set of functions to match the
> + * guest side RPMsg VirtIO API, provided by drivers/rpmsg/virtio_rpmsg_bus.c
> + * These functions handle creation of 2 virtual queues, handling of endpoint
> + * addresses, sending a name-space announcement to the guest as well as any
> + * user messages. This API can be used by any vhost driver to handle RPMsg
> + * specific processing.
> + * Specific vhost drivers, using this API will use their own VirtIO device
> + * IDs, that should then also be added to the ID table in virtio_rpmsg_bus.c
> + */
> +

Thank you for adding that.

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> +#include <linux/compat.h>
> +#include <linux/file.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/vhost.h>
> +#include <linux/virtio_rpmsg.h>
> +#include <uapi/linux/rpmsg.h>
> +
> +#include "vhost.h"
> +#include "vhost_rpmsg.h"
> +
> +/*
> + * All virtio-rpmsg virtual queue kicks always come with just one buffer -
> + * either input or output
> + */
> +static int vhost_rpmsg_get_single(struct vhost_virtqueue *vq)
> +{
> +       struct vhost_rpmsg *vr = container_of(vq->dev, struct vhost_rpmsg, dev);
> +       unsigned int out, in;
> +       int head = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> +                                    &out, &in, NULL, NULL);
> +       if (head < 0) {
> +               vq_err(vq, "%s(): error %d getting buffer\n",
> +                      __func__, head);
> +               return head;
> +       }
> +
> +       /* Nothing new? */
> +       if (head == vq->num)
> +               return head;
> +
> +       if (vq == &vr->vq[VIRTIO_RPMSG_RESPONSE] && (out || in != 1)) {
> +               vq_err(vq,
> +                      "%s(): invalid %d input and %d output in response queue\n",
> +                      __func__, in, out);
> +               goto return_buf;
> +       }
> +
> +       if (vq == &vr->vq[VIRTIO_RPMSG_REQUEST] && (in || out != 1)) {
> +               vq_err(vq,
> +                      "%s(): invalid %d input and %d output in request queue\n",
> +                      __func__, in, out);
> +               goto return_buf;
> +       }
> +
> +       return head;
> +
> +return_buf:
> +       /*
> +        * FIXME: might need to return the buffer using vhost_add_used()
> +        * or vhost_discard_vq_desc(). vhost_discard_vq_desc() is
> +        * described as "being useful for error handling," but it makes
> +        * the thus discarded buffers "unseen," so next time we look we
> +        * retrieve them again?
> +        */
> +       return -EINVAL;
> +}
> +
> +static const struct vhost_rpmsg_ept *vhost_rpmsg_ept_find(
> +                                       struct vhost_rpmsg *vr, int addr)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < vr->n_epts; i++)
> +               if (vr->ept[i].addr == addr)
> +                       return vr->ept + i;
> +
> +       return NULL;
> +}
> +
> +/*
> + * if len < 0, then for reading a request, the complete virtual queue buffer
> + * size is prepared, for sending a response, the length in the iterator is used
> + */
> +int vhost_rpmsg_start_lock(struct vhost_rpmsg *vr,
> +                          struct vhost_rpmsg_iter *iter,
> +                          unsigned int qid, ssize_t len)
> +       __acquires(vq->mutex)
> +{
> +       struct vhost_virtqueue *vq = vr->vq + qid;
> +       size_t tmp;
> +
> +       if (qid >= VIRTIO_RPMSG_NUM_OF_VQS)
> +               return -EINVAL;
> +
> +       iter->vq = vq;
> +
> +       mutex_lock(&vq->mutex);
> +       vhost_disable_notify(&vr->dev, vq);
> +
> +       iter->head = vhost_rpmsg_get_single(vq);
> +       if (iter->head == vq->num)
> +               iter->head = -EAGAIN;
> +
> +       if (iter->head < 0)
> +               goto unlock;
> +
> +       tmp = vq->iov[0].iov_len;
> +       if (tmp < sizeof(iter->rhdr)) {
> +               vq_err(vq, "%s(): size %zu too small\n", __func__, tmp);
> +               iter->head = -ENOBUFS;
> +               goto return_buf;
> +       }
> +
> +       switch (qid) {
> +       case VIRTIO_RPMSG_REQUEST:
> +               if (len < 0) {
> +                       len = tmp - sizeof(iter->rhdr);
> +               } else if (tmp < sizeof(iter->rhdr) + len) {
> +                       iter->head = -ENOBUFS;
> +                       goto return_buf;
> +               }
> +
> +               /* len is now the size of the payload */
> +               iov_iter_init(&iter->iov_iter, WRITE,
> +                             vq->iov, 1, sizeof(iter->rhdr) + len);
> +
> +               /* Read the RPMSG header with endpoint addresses */
> +               tmp = copy_from_iter(&iter->rhdr, sizeof(iter->rhdr),
> +                                    &iter->iov_iter);
> +               if (tmp != sizeof(iter->rhdr)) {
> +                       vq_err(vq, "%s(): got %zu instead of %zu\n", __func__,
> +                              tmp, sizeof(iter->rhdr));
> +                       iter->head = -EIO;
> +                       goto return_buf;
> +               }
> +
> +               iter->ept = vhost_rpmsg_ept_find(vr, iter->rhdr.dst);
> +               if (!iter->ept) {
> +                       vq_err(vq, "%s(): no endpoint with address %d\n",
> +                              __func__, iter->rhdr.dst);
> +                       iter->head = -ENOENT;
> +                       goto return_buf;
> +               }
> +
> +               /* Let the endpoint read the payload */
> +               if (iter->ept->read) {
> +                       ssize_t ret = iter->ept->read(vr, iter);
> +
> +                       if (ret < 0) {
> +                               iter->head = ret;
> +                               goto return_buf;
> +                       }
> +
> +                       iter->rhdr.len = ret;
> +               } else {
> +                       iter->rhdr.len = 0;
> +               }
> +
> +               /* Prepare for the response phase */
> +               iter->rhdr.dst = iter->rhdr.src;
> +               iter->rhdr.src = iter->ept->addr;
> +
> +               break;
> +       case VIRTIO_RPMSG_RESPONSE:
> +               if (!iter->ept && iter->rhdr.dst != RPMSG_NS_ADDR) {
> +                       /*
> +                        * Usually the iterator is configured when processing a
> +                        * message on the request queue, but it's also possible
> +                        * to send a message on the response queue without a
> +                        * preceding request, in that case the iterator must
> +                        * contain source and destination addresses.
> +                        */
> +                       iter->ept = vhost_rpmsg_ept_find(vr, iter->rhdr.src);
> +                       if (!iter->ept) {
> +                               iter->head = -ENOENT;
> +                               goto return_buf;
> +                       }
> +               }
> +
> +               if (len < 0) {
> +                       len = tmp - sizeof(iter->rhdr);
> +               } else if (tmp < sizeof(iter->rhdr) + len) {
> +                       iter->head = -ENOBUFS;
> +                       goto return_buf;
> +               } else {
> +                       iter->rhdr.len = len;
> +               }
> +
> +               /* len is now the size of the payload */
> +               iov_iter_init(&iter->iov_iter, READ,
> +                             vq->iov, 1, sizeof(iter->rhdr) + len);
> +
> +               /* Write the RPMSG header with endpoint addresses */
> +               tmp = copy_to_iter(&iter->rhdr, sizeof(iter->rhdr),
> +                                  &iter->iov_iter);
> +               if (tmp != sizeof(iter->rhdr)) {
> +                       iter->head = -EIO;
> +                       goto return_buf;
> +               }
> +
> +               /* Let the endpoint write the payload */
> +               if (iter->ept && iter->ept->write) {
> +                       ssize_t ret = iter->ept->write(vr, iter);
> +
> +                       if (ret < 0) {
> +                               iter->head = ret;
> +                               goto return_buf;
> +                       }
> +               }
> +
> +               break;
> +       }
> +
> +       return 0;
> +
> +return_buf:
> +       /*
> +        * FIXME: vhost_discard_vq_desc() or vhost_add_used(), see comment in
> +        * vhost_rpmsg_get_single()
> +        */
> +unlock:
> +       vhost_enable_notify(&vr->dev, vq);
> +       mutex_unlock(&vq->mutex);
> +
> +       return iter->head;
> +}
> +EXPORT_SYMBOL_GPL(vhost_rpmsg_start_lock);
> +
> +size_t vhost_rpmsg_copy(struct vhost_rpmsg *vr, struct vhost_rpmsg_iter *iter,
> +                       void *data, size_t size)
> +{
> +       /*
> +        * We could check for excess data, but copy_{to,from}_iter() don't do
> +        * that either
> +        */
> +       if (iter->vq == vr->vq + VIRTIO_RPMSG_RESPONSE)
> +               return copy_to_iter(data, size, &iter->iov_iter);
> +
> +       return copy_from_iter(data, size, &iter->iov_iter);
> +}
> +EXPORT_SYMBOL_GPL(vhost_rpmsg_copy);
> +
> +int vhost_rpmsg_finish_unlock(struct vhost_rpmsg *vr,
> +                             struct vhost_rpmsg_iter *iter)
> +       __releases(vq->mutex)
> +{
> +       if (iter->head >= 0)
> +               vhost_add_used_and_signal(iter->vq->dev, iter->vq, iter->head,
> +                                         iter->rhdr.len + sizeof(iter->rhdr));
> +
> +       vhost_enable_notify(&vr->dev, iter->vq);
> +       mutex_unlock(&iter->vq->mutex);
> +
> +       return iter->head;
> +}
> +EXPORT_SYMBOL_GPL(vhost_rpmsg_finish_unlock);
> +
> +/*
> + * Return false to terminate the external loop only if we fail to obtain either
> + * a request or a response buffer
> + */
> +static bool handle_rpmsg_req_single(struct vhost_rpmsg *vr,
> +                                   struct vhost_virtqueue *vq)
> +{
> +       struct vhost_rpmsg_iter iter;
> +       int ret = vhost_rpmsg_start_lock(vr, &iter, VIRTIO_RPMSG_REQUEST,
> +                                        -EINVAL);
> +       if (!ret)
> +               ret = vhost_rpmsg_finish_unlock(vr, &iter);
> +       if (ret < 0) {
> +               if (ret != -EAGAIN)
> +                       vq_err(vq, "%s(): RPMSG processing failed %d\n",
> +                              __func__, ret);
> +               return false;
> +       }
> +
> +       if (!iter.ept->write)
> +               return true;
> +
> +       ret = vhost_rpmsg_start_lock(vr, &iter, VIRTIO_RPMSG_RESPONSE,
> +                                    -EINVAL);
> +       if (!ret)
> +               ret = vhost_rpmsg_finish_unlock(vr, &iter);
> +       if (ret < 0) {
> +               vq_err(vq, "%s(): RPMSG finalising failed %d\n", __func__, ret);
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +static void handle_rpmsg_req_kick(struct vhost_work *work)
> +{
> +       struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> +                                                 poll.work);
> +       struct vhost_rpmsg *vr = container_of(vq->dev, struct vhost_rpmsg, dev);
> +
> +       while (handle_rpmsg_req_single(vr, vq))
> +               ;
> +}
> +
> +/*
> + * initialise two virtqueues with an array of endpoints,
> + * request and response callbacks
> + */
> +void vhost_rpmsg_init(struct vhost_rpmsg *vr, const struct vhost_rpmsg_ept *ept,
> +                     unsigned int n_epts)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(vr->vq); i++)
> +               vr->vq_p[i] = &vr->vq[i];
> +
> +       /* vq[0]: host -> guest, vq[1]: host <- guest */
> +       vr->vq[VIRTIO_RPMSG_REQUEST].handle_kick = handle_rpmsg_req_kick;
> +       vr->vq[VIRTIO_RPMSG_RESPONSE].handle_kick = NULL;
> +
> +       vr->ept = ept;
> +       vr->n_epts = n_epts;
> +
> +       vhost_dev_init(&vr->dev, vr->vq_p, VIRTIO_RPMSG_NUM_OF_VQS,
> +                      UIO_MAXIOV, 0, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(vhost_rpmsg_init);
> +
> +void vhost_rpmsg_destroy(struct vhost_rpmsg *vr)
> +{
> +       if (vhost_dev_has_owner(&vr->dev))
> +               vhost_poll_flush(&vr->vq[VIRTIO_RPMSG_REQUEST].poll);
> +
> +       vhost_dev_cleanup(&vr->dev);
> +}
> +EXPORT_SYMBOL_GPL(vhost_rpmsg_destroy);
> +
> +/* send namespace */
> +int vhost_rpmsg_ns_announce(struct vhost_rpmsg *vr, const char *name,
> +                           unsigned int src)
> +{
> +       struct vhost_rpmsg_iter iter = {
> +               .rhdr = {
> +                       .src = 0,
> +                       .dst = RPMSG_NS_ADDR,
> +                       .flags = RPMSG_NS_CREATE, /* rpmsg_recv_single() */
> +               },
> +       };
> +       struct rpmsg_ns_msg ns = {
> +               .addr = src,
> +               .flags = RPMSG_NS_CREATE, /* for rpmsg_ns_cb() */
> +       };
> +       int ret = vhost_rpmsg_start_lock(vr, &iter, VIRTIO_RPMSG_RESPONSE,
> +                                        sizeof(ns));
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       strlcpy(ns.name, name, sizeof(ns.name));
> +
> +       ret = vhost_rpmsg_copy(vr, &iter, &ns, sizeof(ns));
> +       if (ret != sizeof(ns))
> +               vq_err(iter.vq, "%s(): added %d instead of %zu bytes\n",
> +                      __func__, ret, sizeof(ns));
> +
> +       ret = vhost_rpmsg_finish_unlock(vr, &iter);
> +       if (ret < 0)
> +               vq_err(iter.vq, "%s(): namespace announcement failed: %d\n",
> +                      __func__, ret);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(vhost_rpmsg_ns_announce);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Intel, Inc.");
> +MODULE_DESCRIPTION("Vhost RPMsg API");
> diff --git a/drivers/vhost/vhost_rpmsg.h b/drivers/vhost/vhost_rpmsg.h
> new file mode 100644
> index 00000000..a3d0dda
> --- /dev/null
> +++ b/drivers/vhost/vhost_rpmsg.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright(c) 2020 Intel Corporation. All rights reserved.
> + *
> + * Author: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> + */
> +
> +#ifndef VHOST_RPMSG_H
> +#define VHOST_RPMSG_H
> +
> +#include <linux/uio.h>
> +#include <linux/virtio_rpmsg.h>
> +
> +#include "vhost.h"
> +
> +/* RPMsg uses two VirtQueues: one for each direction */
> +enum {
> +       VIRTIO_RPMSG_RESPONSE,  /* RPMsg response (host->guest) buffers */
> +       VIRTIO_RPMSG_REQUEST,   /* RPMsg request (guest->host) buffers */
> +       /* Keep last */
> +       VIRTIO_RPMSG_NUM_OF_VQS,
> +};
> +
> +struct vhost_rpmsg_ept;
> +
> +struct vhost_rpmsg_iter {
> +       struct iov_iter iov_iter;
> +       struct rpmsg_hdr rhdr;
> +       struct vhost_virtqueue *vq;
> +       const struct vhost_rpmsg_ept *ept;
> +       int head;
> +       void *priv;
> +};
> +
> +struct vhost_rpmsg {
> +       struct vhost_dev dev;
> +       struct vhost_virtqueue vq[VIRTIO_RPMSG_NUM_OF_VQS];
> +       struct vhost_virtqueue *vq_p[VIRTIO_RPMSG_NUM_OF_VQS];
> +       const struct vhost_rpmsg_ept *ept;
> +       unsigned int n_epts;
> +};
> +
> +struct vhost_rpmsg_ept {
> +       ssize_t (*read)(struct vhost_rpmsg *, struct vhost_rpmsg_iter *);
> +       ssize_t (*write)(struct vhost_rpmsg *, struct vhost_rpmsg_iter *);
> +       int addr;
> +};
> +
> +static inline size_t vhost_rpmsg_iter_len(const struct vhost_rpmsg_iter *iter)
> +{
> +       return iter->rhdr.len;
> +}
> +
> +#define VHOST_RPMSG_ITER(_src, _dst) { \
> +       .rhdr = {                       \
> +                       .src = _src,    \
> +                       .dst = _dst,    \
> +               },                      \
> +       }
> +
> +void vhost_rpmsg_init(struct vhost_rpmsg *vr, const struct vhost_rpmsg_ept *ept,
> +                     unsigned int n_epts);
> +void vhost_rpmsg_destroy(struct vhost_rpmsg *vr);
> +int vhost_rpmsg_ns_announce(struct vhost_rpmsg *vr, const char *name,
> +                           unsigned int src);
> +int vhost_rpmsg_start_lock(struct vhost_rpmsg *vr,
> +                          struct vhost_rpmsg_iter *iter,
> +                          unsigned int qid, ssize_t len);
> +size_t vhost_rpmsg_copy(struct vhost_rpmsg *vr, struct vhost_rpmsg_iter *iter,
> +                       void *data, size_t size);
> +int vhost_rpmsg_finish_unlock(struct vhost_rpmsg *vr,
> +                             struct vhost_rpmsg_iter *iter);
> +
> +#endif
> --
> 1.9.3
>

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Vincent Whitchurch]

On Wed, May 27, 2020 at 08:05:41PM +0200, Guennadi Liakhovetski wrote:
> Linux supports running the RPMsg protocol over the VirtIO transport
> protocol, but currently there is only support for VirtIO clients and
> no support for a VirtIO server. This patch adds a vhost-based RPMsg
> server implementation.

This looks really useful, but why is it implemented as an API and not as
a real vhost driver which implements an rpmsg bus?  If you implement it
as a vhost driver which implements rpmsg_device_ops and
rpmsg_endpoint_ops, then wouldn't you be able to implement your
vhost-sof driver using the normal rpmsg APIs?

I tried quickly hooking up this code to such a vhost driver and I was
able to communicate between host and guest systems with both
rpmsg-client-sample and rpmsg-char which almost no modifications to
those drivers.

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Guennadi Liakhovetski]

Hi Vincent,

On Wed, Jun 17, 2020 at 09:17:42PM +0200, Vincent Whitchurch wrote:
> On Wed, May 27, 2020 at 08:05:41PM +0200, Guennadi Liakhovetski wrote:
> > Linux supports running the RPMsg protocol over the VirtIO transport
> > protocol, but currently there is only support for VirtIO clients and
> > no support for a VirtIO server. This patch adds a vhost-based RPMsg
> > server implementation.
> 
> This looks really useful, but why is it implemented as an API and not as
> a real vhost driver which implements an rpmsg bus?  If you implement it
> as a vhost driver which implements rpmsg_device_ops and
> rpmsg_endpoint_ops, then wouldn't you be able to implement your
> vhost-sof driver using the normal rpmsg APIs?

Sorry, not sure what you mean by the "normal rpmsg API?" Do you mean the 
VirtIO RPMsg API? But that's the opposite side of the link - that's the 
guest side in the VM case and the Linux side in the remoteproc case. What 
this API is adding is a vhost RPMsg API. The kernel vhost framework 
itself is essentially a library of functions. Kernel vhost drivers simply 
create a misc device and use the vhost functions for some common 
functionality. This RPMsg vhost API stays in the same concept and provides 
further functions for RPMsg specific vhost operation.

> I tried quickly hooking up this code to such a vhost driver and I was
> able to communicate between host and guest systems with both
> rpmsg-client-sample and rpmsg-char which almost no modifications to
> those drivers.

You mean you used this patch to create RPMsg vhost drivers? Without 
creating a vhost RPMsg bus? Nice, glad to hear that!

Thanks
Guennadi

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Vincent Whitchurch]

On Thu, Jun 18, 2020 at 11:03:42AM +0200, Guennadi Liakhovetski wrote:
> On Wed, Jun 17, 2020 at 09:17:42PM +0200, Vincent Whitchurch wrote:
> > On Wed, May 27, 2020 at 08:05:41PM +0200, Guennadi Liakhovetski wrote:
> > > Linux supports running the RPMsg protocol over the VirtIO transport
> > > protocol, but currently there is only support for VirtIO clients and
> > > no support for a VirtIO server. This patch adds a vhost-based RPMsg
> > > server implementation.
> > 
> > This looks really useful, but why is it implemented as an API and not as
> > a real vhost driver which implements an rpmsg bus?  If you implement it
> > as a vhost driver which implements rpmsg_device_ops and
> > rpmsg_endpoint_ops, then wouldn't you be able to implement your
> > vhost-sof driver using the normal rpmsg APIs?
> 
> Sorry, not sure what you mean by the "normal rpmsg API?" Do you mean the 
> VirtIO RPMsg API? But that's the opposite side of the link - that's the 
> guest side in the VM case and the Linux side in the remoteproc case. What 
> this API is adding is a vhost RPMsg API. The kernel vhost framework 
> itself is essentially a library of functions. Kernel vhost drivers simply 
> create a misc device and use the vhost functions for some common 
> functionality. This RPMsg vhost API stays in the same concept and provides 
> further functions for RPMsg specific vhost operation.

By the "normal rpmsg API" I mean register_rpmsg_driver(), rpmsg_send(),
etc.  That API is not tied to virtio in any way and there are other
non-virtio backends for this API in the tree.  So it seems quite natural
to implement a vhost backend for this API so that both sides of the link
can use the same API but different backends, instead of forcing them to
use of different APIs.

> > I tried quickly hooking up this code to such a vhost driver and I was
> > able to communicate between host and guest systems with both
> > rpmsg-client-sample and rpmsg-char which almost no modifications to
> > those drivers.
> 
> You mean you used this patch to create RPMsg vhost drivers? Without 
> creating a vhost RPMsg bus? Nice, glad to hear that!

Not quite, I hacked togther a single generic vhost-rpmsg-bus driver
which just wraps the API in this patch and implements a basic
rpmsg_device_ops and rpmsg_endpoint_ops.  Then with the following
patches and no other vhost-specific API use, I was able to load and use
the same rpmsg-char and rpmsg-client-sample drivers on both host and
guest kernels.

Userspace sets up the vhost using vhost-rpmsg-bus' misc device and
triggers creation of an rpdev which leads to a probe of the (for
example) rpmsg-client-sample driver on the host (server), which, in
turn, via NS announcement, triggers a creation of an rpdev and a probe
of the rpmsg-client-sample driver on the guest (client).

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index a76b963a7e5..7a03978d002 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -104,6 +104,11 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
 	struct rpmsg_eptdev *eptdev = priv;
 	struct sk_buff *skb;
 
+	if (rpdev->dst == RPMSG_ADDR_ANY) {
+		printk("%s: got client address %#x from first rx!\n", __func__, addr);
+		rpdev->dst = addr;
+	}
+
 	skb = alloc_skb(len, GFP_ATOMIC);
 	if (!skb)
 		return -ENOMEM;
@@ -235,6 +240,12 @@ static ssize_t rpmsg_eptdev_write(struct file *filp, const char __user *buf,
 		goto unlock_eptdev;
 	}
 
+	if (eptdev->rpdev->dst == RPMSG_ADDR_ANY) {
+		ret = -EPIPE;
+		WARN(1, "Cannot write first on server, must wait for client!\n");
+		goto unlock_eptdev;
+	}
+
 	if (filp->f_flags & O_NONBLOCK)
 		ret = rpmsg_trysend(eptdev->ept, kbuf, len);
 	else
diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/rpmsg_client_sample.c
index f161dfd3e70..5d8ca84dce0 100644
--- a/samples/rpmsg/rpmsg_client_sample.c
+++ b/samples/rpmsg/rpmsg_client_sample.c
@@ -46,6 +46,9 @@ static int rpmsg_sample_cb(struct rpmsg_device *rpdev, void *data, int len,
 		return 0;
 	}
 
+	if (rpdev->dst == RPMSG_ADDR_ANY)
+		rpdev->dst = src;
+
 	/* send a new message now */
 	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
 	if (ret)
@@ -68,11 +71,13 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(&rpdev->dev, idata);
 
-	/* send a message to our remote processor */
-	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
-	if (ret) {
-		dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
-		return ret;
+	if (rpdev->dst != RPMSG_ADDR_ANY) {
+		/* send a message to our remote processor */
+		ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
+		if (ret) {
+			dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
+			return ret;
+		}
 	}
 
 	return 0;

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Guennadi Liakhovetski]

On Thu, Jun 18, 2020 at 11:33:24AM +0200, Vincent Whitchurch wrote:
> On Thu, Jun 18, 2020 at 11:03:42AM +0200, Guennadi Liakhovetski wrote:
> > On Wed, Jun 17, 2020 at 09:17:42PM +0200, Vincent Whitchurch wrote:
> > > On Wed, May 27, 2020 at 08:05:41PM +0200, Guennadi Liakhovetski wrote:
> > > > Linux supports running the RPMsg protocol over the VirtIO transport
> > > > protocol, but currently there is only support for VirtIO clients and
> > > > no support for a VirtIO server. This patch adds a vhost-based RPMsg
> > > > server implementation.
> > > 
> > > This looks really useful, but why is it implemented as an API and not as
> > > a real vhost driver which implements an rpmsg bus?  If you implement it
> > > as a vhost driver which implements rpmsg_device_ops and
> > > rpmsg_endpoint_ops, then wouldn't you be able to implement your
> > > vhost-sof driver using the normal rpmsg APIs?
> > 
> > Sorry, not sure what you mean by the "normal rpmsg API?" Do you mean the 
> > VirtIO RPMsg API? But that's the opposite side of the link - that's the 
> > guest side in the VM case and the Linux side in the remoteproc case. What 
> > this API is adding is a vhost RPMsg API. The kernel vhost framework 
> > itself is essentially a library of functions. Kernel vhost drivers simply 
> > create a misc device and use the vhost functions for some common 
> > functionality. This RPMsg vhost API stays in the same concept and provides 
> > further functions for RPMsg specific vhost operation.
> 
> By the "normal rpmsg API" I mean register_rpmsg_driver(), rpmsg_send(),
> etc.  That API is not tied to virtio in any way and there are other
> non-virtio backends for this API in the tree.  So it seems quite natural
> to implement a vhost backend for this API so that both sides of the link
> can use the same API but different backends, instead of forcing them to
> use of different APIs.

Ok, I see what you mean now. But I'm not sure this is useful or desired. I'm 
not an expert in KVM / VirtIO, I've only been working in the area for less 
than a year, so, I might well be wrong.

You're proposing to use the rpmsg API in vhost drivers. As far as I 
understand so far that API was only designated for the Linux side (in case of 
AMPs) which corresponds to VM guests in virtualisation case. So, I'm not sure 
we want to use the same API for the hosts? This can be done as you have 
illustrated, but is it desirable? The vhost API is far enough from the VirtIO 
driver API, so I'm not sure why we want the same API for rpmsg?

Thanks
Guennadi

> > > I tried quickly hooking up this code to such a vhost driver and I was
> > > able to communicate between host and guest systems with both
> > > rpmsg-client-sample and rpmsg-char which almost no modifications to
> > > those drivers.
> > 
> > You mean you used this patch to create RPMsg vhost drivers? Without 
> > creating a vhost RPMsg bus? Nice, glad to hear that!
> 
> Not quite, I hacked togther a single generic vhost-rpmsg-bus driver
> which just wraps the API in this patch and implements a basic
> rpmsg_device_ops and rpmsg_endpoint_ops.  Then with the following
> patches and no other vhost-specific API use, I was able to load and use
> the same rpmsg-char and rpmsg-client-sample drivers on both host and
> guest kernels.
> 
> Userspace sets up the vhost using vhost-rpmsg-bus' misc device and
> triggers creation of an rpdev which leads to a probe of the (for
> example) rpmsg-client-sample driver on the host (server), which, in
> turn, via NS announcement, triggers a creation of an rpdev and a probe
> of the rpmsg-client-sample driver on the guest (client).
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index a76b963a7e5..7a03978d002 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -104,6 +104,11 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>  	struct rpmsg_eptdev *eptdev = priv;
>  	struct sk_buff *skb;
>  
> +	if (rpdev->dst == RPMSG_ADDR_ANY) {
> +		printk("%s: got client address %#x from first rx!\n", __func__, addr);
> +		rpdev->dst = addr;
> +	}
> +
>  	skb = alloc_skb(len, GFP_ATOMIC);
>  	if (!skb)
>  		return -ENOMEM;
> @@ -235,6 +240,12 @@ static ssize_t rpmsg_eptdev_write(struct file *filp, const char __user *buf,
>  		goto unlock_eptdev;
>  	}
>  
> +	if (eptdev->rpdev->dst == RPMSG_ADDR_ANY) {
> +		ret = -EPIPE;
> +		WARN(1, "Cannot write first on server, must wait for client!\n");
> +		goto unlock_eptdev;
> +	}
> +
>  	if (filp->f_flags & O_NONBLOCK)
>  		ret = rpmsg_trysend(eptdev->ept, kbuf, len);
>  	else
> diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/rpmsg_client_sample.c
> index f161dfd3e70..5d8ca84dce0 100644
> --- a/samples/rpmsg/rpmsg_client_sample.c
> +++ b/samples/rpmsg/rpmsg_client_sample.c
> @@ -46,6 +46,9 @@ static int rpmsg_sample_cb(struct rpmsg_device *rpdev, void *data, int len,
>  		return 0;
>  	}
>  
> +	if (rpdev->dst == RPMSG_ADDR_ANY)
> +		rpdev->dst = src;
> +
>  	/* send a new message now */
>  	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
>  	if (ret)
> @@ -68,11 +71,13 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
>  
>  	dev_set_drvdata(&rpdev->dev, idata);
>  
> -	/* send a message to our remote processor */
> -	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
> -	if (ret) {
> -		dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> -		return ret;
> +	if (rpdev->dst != RPMSG_ADDR_ANY) {
> +		/* send a message to our remote processor */
> +		ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
> +		if (ret) {
> +			dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Vincent Whitchurch]

On Thu, Jun 18, 2020 at 12:39:40PM +0200, Guennadi Liakhovetski wrote:
> On Thu, Jun 18, 2020 at 11:33:24AM +0200, Vincent Whitchurch wrote:
> > By the "normal rpmsg API" I mean register_rpmsg_driver(), rpmsg_send(),
> > etc.  That API is not tied to virtio in any way and there are other
> > non-virtio backends for this API in the tree.  So it seems quite natural
> > to implement a vhost backend for this API so that both sides of the link
> > can use the same API but different backends, instead of forcing them to
> > use of different APIs.
> 
> Ok, I see what you mean now. But I'm not sure this is useful or desired. I'm 
> not an expert in KVM / VirtIO, I've only been working in the area for less 
> than a year, so, I might well be wrong.
> 
> You're proposing to use the rpmsg API in vhost drivers. As far as I 
> understand so far that API was only designated for the Linux side (in case of 
> AMPs) which corresponds to VM guests in virtualisation case. So, I'm not sure 
> we want to use the same API for the hosts? This can be done as you have 
> illustrated, but is it desirable? The vhost API is far enough from the VirtIO 
> driver API, so I'm not sure why we want the same API for rpmsg?

Note that "the Linux side" is ambiguous for AMP since both sides can be
Linux, as they happen to be in my case.  I'm running virtio/rpmsg
between two physical processors (of different architectures), both
running Linux.

virtio has distinct driver and device roles so the completely different
APIs on each side are understandable.  But I don't see that distinction
in the rpmsg API which is why it seems like a good idea to me to make it
work from both sides of the link and allow the reuse of drivers like
rpmsg-char, instead of imposing virtio's distinction on rpmsg.

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Guennadi Liakhovetski]

On Thu, Jun 18, 2020 at 03:52:42PM +0200, Vincent Whitchurch wrote:
> On Thu, Jun 18, 2020 at 12:39:40PM +0200, Guennadi Liakhovetski wrote:
> > On Thu, Jun 18, 2020 at 11:33:24AM +0200, Vincent Whitchurch wrote:
> > > By the "normal rpmsg API" I mean register_rpmsg_driver(), rpmsg_send(),
> > > etc.  That API is not tied to virtio in any way and there are other
> > > non-virtio backends for this API in the tree.  So it seems quite natural
> > > to implement a vhost backend for this API so that both sides of the link
> > > can use the same API but different backends, instead of forcing them to
> > > use of different APIs.
> > 
> > Ok, I see what you mean now. But I'm not sure this is useful or desired. I'm 
> > not an expert in KVM / VirtIO, I've only been working in the area for less 
> > than a year, so, I might well be wrong.
> > 
> > You're proposing to use the rpmsg API in vhost drivers. As far as I 
> > understand so far that API was only designated for the Linux side (in case of 
> > AMPs) which corresponds to VM guests in virtualisation case. So, I'm not sure 
> > we want to use the same API for the hosts? This can be done as you have 
> > illustrated, but is it desirable? The vhost API is far enough from the VirtIO 
> > driver API, so I'm not sure why we want the same API for rpmsg?
> 
> Note that "the Linux side" is ambiguous for AMP since both sides can be
> Linux, as they happen to be in my case.  I'm running virtio/rpmsg
> between two physical processors (of different architectures), both
> running Linux.

Ok, interesting, I didn't know such configurations were used too. I understood 
the Linux rpmsg implementation in the way, that it's assumed, that the "host" 
has to boot the "device" by sending an ELF formatted executable image to it, is 
that optional? You aren't sending a complete Linux image to the device side, 
are you?

> virtio has distinct driver and device roles so the completely different
> APIs on each side are understandable.  But I don't see that distinction
> in the rpmsg API which is why it seems like a good idea to me to make it
> work from both sides of the link and allow the reuse of drivers like
> rpmsg-char, instead of imposing virtio's distinction on rpmsg.

Understand. In principle I'm open to this idea, but before I implement it it 
would be good to know what maintainers think?

Thanks
Guennadi

Re: [PATCH v3 5/5] vhost: add an RPMsg API

[Vincent Whitchurch]

On Thu, Jun 18, 2020 at 04:14:12PM +0200, Guennadi Liakhovetski wrote:
> On Thu, Jun 18, 2020 at 03:52:42PM +0200, Vincent Whitchurch wrote:
> > Note that "the Linux side" is ambiguous for AMP since both sides can be
> > Linux, as they happen to be in my case.  I'm running virtio/rpmsg
> > between two physical processors (of different architectures), both
> > running Linux.
> 
> Ok, interesting, I didn't know such configurations were used too. I understood 
> the Linux rpmsg implementation in the way, that it's assumed, that the "host" 
> has to boot the "device" by sending an ELF formatted executable image to it, is 
> that optional? You aren't sending a complete Linux image to the device side, 
> are you?

I do pack the zImage, the dtb, and the initramfs into an ELF (along with
a tiny "bootloader" with just a handful of instructions), but the
remoteproc framework is not tied to the ELF format since ->parse_fw()
and friends are overridable by the remoteproc driver.

> > virtio has distinct driver and device roles so the completely different
> > APIs on each side are understandable.  But I don't see that distinction
> > in the rpmsg API which is why it seems like a good idea to me to make it
> > work from both sides of the link and allow the reuse of drivers like
> > rpmsg-char, instead of imposing virtio's distinction on rpmsg.
> 
> Understand. In principle I'm open to this idea, but before I implement it it 
> would be good to know what maintainers think?

Certainly.

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Jason Wang]

On 2020/5/28 上午2:05, Guennadi Liakhovetski wrote:
> v3:
> - address several checkpatch warnings
> - address comments from Mathieu Poirier
>
> v2:
> - update patch #5 with a correct vhost_dev_init() prototype
> - drop patch #6 - it depends on a different patch, that is currently
>    an RFC
> - address comments from Pierre-Louis Bossart:
>    * remove "default n" from Kconfig
>
> Linux supports RPMsg over VirtIO for "remote processor" /AMP use
> cases. It can however also be used for virtualisation scenarios,
> e.g. when using KVM to run Linux on both the host and the guests.
> This patch set adds a wrapper API to facilitate writing vhost
> drivers for such RPMsg-based solutions. The first use case is an
> audio DSP virtualisation project, currently under development, ready
> for review and submission, available at
> https://github.com/thesofproject/linux/pull/1501/commits
> A further patch for the ADSP vhost RPMsg driver will be sent
> separately for review only since it cannot be merged without audio
> patches being upstreamed first.


Hi:

It would be hard to evaluate this series without a real user. So if 
possible, I suggest to post the actual user for vhost rpmsg API.

Thanks


>
> Thanks
> Guennadi

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

Hi Jason,

On Fri, May 29, 2020 at 02:01:53PM +0800, Jason Wang wrote:
> 
> On 2020/5/28 上午2:05, Guennadi Liakhovetski wrote:
> > v3:
> > - address several checkpatch warnings
> > - address comments from Mathieu Poirier
> > 
> > v2:
> > - update patch #5 with a correct vhost_dev_init() prototype
> > - drop patch #6 - it depends on a different patch, that is currently
> >    an RFC
> > - address comments from Pierre-Louis Bossart:
> >    * remove "default n" from Kconfig
> > 
> > Linux supports RPMsg over VirtIO for "remote processor" /AMP use
> > cases. It can however also be used for virtualisation scenarios,
> > e.g. when using KVM to run Linux on both the host and the guests.
> > This patch set adds a wrapper API to facilitate writing vhost
> > drivers for such RPMsg-based solutions. The first use case is an
> > audio DSP virtualisation project, currently under development, ready
> > for review and submission, available at
> > https://github.com/thesofproject/linux/pull/1501/commits
> > A further patch for the ADSP vhost RPMsg driver will be sent
> > separately for review only since it cannot be merged without audio
> > patches being upstreamed first.
> 
> 
> Hi:
> 
> It would be hard to evaluate this series without a real user. So if
> possible, I suggest to post the actual user for vhost rpmsg API.

Sure, the whole series is available at 
https://github.com/thesofproject/linux/pull/1501/commits or would you 
prefer the missing patches posted to the lists too?

Thanks
Guennadi

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Jason Wang]

On 2020/5/29 下午2:50, Guennadi Liakhovetski wrote:
> Hi Jason,
>
> On Fri, May 29, 2020 at 02:01:53PM +0800, Jason Wang wrote:
>> On 2020/5/28 上午2:05, Guennadi Liakhovetski wrote:
>>> v3:
>>> - address several checkpatch warnings
>>> - address comments from Mathieu Poirier
>>>
>>> v2:
>>> - update patch #5 with a correct vhost_dev_init() prototype
>>> - drop patch #6 - it depends on a different patch, that is currently
>>>     an RFC
>>> - address comments from Pierre-Louis Bossart:
>>>     * remove "default n" from Kconfig
>>>
>>> Linux supports RPMsg over VirtIO for "remote processor" /AMP use
>>> cases. It can however also be used for virtualisation scenarios,
>>> e.g. when using KVM to run Linux on both the host and the guests.
>>> This patch set adds a wrapper API to facilitate writing vhost
>>> drivers for such RPMsg-based solutions. The first use case is an
>>> audio DSP virtualisation project, currently under development, ready
>>> for review and submission, available at
>>> https://github.com/thesofproject/linux/pull/1501/commits
>>> A further patch for the ADSP vhost RPMsg driver will be sent
>>> separately for review only since it cannot be merged without audio
>>> patches being upstreamed first.
>>
>> Hi:
>>
>> It would be hard to evaluate this series without a real user. So if
>> possible, I suggest to post the actual user for vhost rpmsg API.
> Sure, the whole series is available at
> https://github.com/thesofproject/linux/pull/1501/commits or would you
> prefer the missing patches posted to the lists too?


Yes, since I see new virtio and vhost drivers there.

Thanks


>
> Thanks
> Guennadi
>

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Michael S. Tsirkin]

On Wed, May 27, 2020 at 08:05:36PM +0200, Guennadi Liakhovetski wrote:
> v3:
> - address several checkpatch warnings
> - address comments from Mathieu Poirier
> 
> v2:
> - update patch #5 with a correct vhost_dev_init() prototype
> - drop patch #6 - it depends on a different patch, that is currently
>   an RFC
> - address comments from Pierre-Louis Bossart:
>   * remove "default n" from Kconfig
> 
> Linux supports RPMsg over VirtIO for "remote processor" /AMP use
> cases. It can however also be used for virtualisation scenarios,
> e.g. when using KVM to run Linux on both the host and the guests.
> This patch set adds a wrapper API to facilitate writing vhost
> drivers for such RPMsg-based solutions. The first use case is an
> audio DSP virtualisation project, currently under development, ready
> for review and submission, available at
> https://github.com/thesofproject/linux/pull/1501/commits
> A further patch for the ADSP vhost RPMsg driver will be sent
> separately for review only since it cannot be merged without audio
> patches being upstreamed first.


RPMsg over virtio has several problems. One is that it's
not specced at all. Before we add more stuff, I'd like so
see at least an attempt at describing what it's supposed to do.

Another it's out of line with 1.0 spec passing guest
endian data around. Won't work if host and guest
endian-ness do not match. Should pass eveything in LE and
convert.

It's great to see it's seeing active development finally.
Do you think you will have time to address these?



> Thanks
> Guennadi
> 
> Guennadi Liakhovetski (5):
>   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
>   vhost: (cosmetic) remove a superfluous variable initialisation
>   rpmsg: move common structures and defines to headers
>   rpmsg: update documentation
>   vhost: add an RPMsg API
> 
>  Documentation/rpmsg.txt          |   6 +-
>  drivers/rpmsg/virtio_rpmsg_bus.c |  78 +-------
>  drivers/vhost/Kconfig            |   7 +
>  drivers/vhost/Makefile           |   3 +
>  drivers/vhost/rpmsg.c            | 382 +++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/vhost.c            |   2 +-
>  drivers/vhost/vhost_rpmsg.h      |  74 ++++++++
>  include/linux/virtio_rpmsg.h     |  81 +++++++++
>  include/uapi/linux/rpmsg.h       |   3 +
>  include/uapi/linux/vhost.h       |   4 +-
>  10 files changed, 559 insertions(+), 81 deletions(-)
>  create mode 100644 drivers/vhost/rpmsg.c
>  create mode 100644 drivers/vhost/vhost_rpmsg.h
>  create mode 100644 include/linux/virtio_rpmsg.h
> 
> -- 
> 1.9.3

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

Hi Michael,

Thanks for your review.

On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:
> On Wed, May 27, 2020 at 08:05:36PM +0200, Guennadi Liakhovetski wrote:
> > v3:
> > - address several checkpatch warnings
> > - address comments from Mathieu Poirier
> > 
> > v2:
> > - update patch #5 with a correct vhost_dev_init() prototype
> > - drop patch #6 - it depends on a different patch, that is currently
> >   an RFC
> > - address comments from Pierre-Louis Bossart:
> >   * remove "default n" from Kconfig
> > 
> > Linux supports RPMsg over VirtIO for "remote processor" /AMP use
> > cases. It can however also be used for virtualisation scenarios,
> > e.g. when using KVM to run Linux on both the host and the guests.
> > This patch set adds a wrapper API to facilitate writing vhost
> > drivers for such RPMsg-based solutions. The first use case is an
> > audio DSP virtualisation project, currently under development, ready
> > for review and submission, available at
> > https://github.com/thesofproject/linux/pull/1501/commits
> > A further patch for the ADSP vhost RPMsg driver will be sent
> > separately for review only since it cannot be merged without audio
> > patches being upstreamed first.
> 
> 
> RPMsg over virtio has several problems. One is that it's
> not specced at all. Before we add more stuff, I'd like so
> see at least an attempt at describing what it's supposed to do.

Sure, I can work on this with the original authors of the virtio-rpmsg 
implementation.

> Another it's out of line with 1.0 spec passing guest
> endian data around. Won't work if host and guest
> endian-ness do not match. Should pass eveything in LE and
> convert.

Yes, I have to fix this, thanks.

> It's great to see it's seeing active development finally.
> Do you think you will have time to address these?

Sure, I'll try to take care of them.

Thanks
Guennadi

> > Guennadi Liakhovetski (5):
> >   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
> >   vhost: (cosmetic) remove a superfluous variable initialisation
> >   rpmsg: move common structures and defines to headers
> >   rpmsg: update documentation
> >   vhost: add an RPMsg API
> > 
> >  Documentation/rpmsg.txt          |   6 +-
> >  drivers/rpmsg/virtio_rpmsg_bus.c |  78 +-------
> >  drivers/vhost/Kconfig            |   7 +
> >  drivers/vhost/Makefile           |   3 +
> >  drivers/vhost/rpmsg.c            | 382 +++++++++++++++++++++++++++++++++++++++
> >  drivers/vhost/vhost.c            |   2 +-
> >  drivers/vhost/vhost_rpmsg.h      |  74 ++++++++
> >  include/linux/virtio_rpmsg.h     |  81 +++++++++
> >  include/uapi/linux/rpmsg.h       |   3 +
> >  include/uapi/linux/vhost.h       |   4 +-
> >  10 files changed, 559 insertions(+), 81 deletions(-)
> >  create mode 100644 drivers/vhost/rpmsg.c
> >  create mode 100644 drivers/vhost/vhost_rpmsg.h
> >  create mode 100644 include/linux/virtio_rpmsg.h
> > 
> > -- 
> > 1.9.3
> 

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

Hi Michael,

On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote:
> 
> On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:

[snip]

> > Another it's out of line with 1.0 spec passing guest
> > endian data around. Won't work if host and guest
> > endian-ness do not match. Should pass eveything in LE and
> > convert.
> 
> Yes, I have to fix this, thanks.

Just to make sure my understanding is correct: this would involve also 
modifying the current virtio_rpmsg_bus.c implementation to add 
endianness conversions. That's what you meant, right?

Thanks
Guennadi

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 09:37:15AM +0200, Guennadi Liakhovetski wrote:
> Hi Michael,
> 
> On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote:
> > 
> > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:
> 
> [snip]
> 
> > > Another it's out of line with 1.0 spec passing guest
> > > endian data around. Won't work if host and guest
> > > endian-ness do not match. Should pass eveything in LE and
> > > convert.
> > 
> > Yes, I have to fix this, thanks.
> 
> Just to make sure my understanding is correct: this would involve also 
> modifying the current virtio_rpmsg_bus.c implementation to add 
> endianness conversions. That's what you meant, right?
> 
> Thanks
> Guennadi

right and if there are legacy compat considerations, using _virtio16 and
friends types, as well as virtio16_to_cpu and friends functions.

-- 
MST

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, 
including byte order, is defined on a per-device type basis. RPMsg is 
indeed included in the spec as device type 7, but that's the only 
mention of it in both versions. It seems RPMsg over VirtIO isn't 
standardised yet. Also it looks like newer interface definitions 
specify using "guest native endianness" for Virtual Queue data. So 
I think the same should be done for RPMsg instead of enforcing LE?

Thanks
Guennadi

On Mon, Jun 08, 2020 at 09:37:15AM +0200, Guennadi Liakhovetski wrote:
> Hi Michael,
> 
> On Fri, Jun 05, 2020 at 08:34:35AM +0200, Guennadi Liakhovetski wrote:
> > 
> > On Thu, Jun 04, 2020 at 03:23:37PM -0400, Michael S. Tsirkin wrote:
> 
> [snip]
> 
> > > Another it's out of line with 1.0 spec passing guest
> > > endian data around. Won't work if host and guest
> > > endian-ness do not match. Should pass eveything in LE and
> > > convert.
> > 
> > Yes, I have to fix this, thanks.
> 
> Just to make sure my understanding is correct: this would involve also 
> modifying the current virtio_rpmsg_bus.c implementation to add 
> endianness conversions. That's what you meant, right?

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, 
> including byte order, is defined on a per-device type basis. RPMsg is 
> indeed included in the spec as device type 7, but that's the only 
> mention of it in both versions. It seems RPMsg over VirtIO isn't 
> standardised yet.

Yes. And it would be very good to have some standartization before we
keep adding things. For example without any spec if host code breaks
with some guests, how do we know which side should be fixed?

> Also it looks like newer interface definitions 
> specify using "guest native endianness" for Virtual Queue data.

They really don't or shouldn't. That's limited to legacy chapters.
Some definitions could have slipped through but it's not
the norm. I just quickly looked through the 1.1 spec and could
not find any instances that specify "guest native endianness"
but feel free to point them out to me.

> So 
> I think the same should be done for RPMsg instead of enforcing LE?
> 
> Thanks
> Guennadi

That makes hardware implementations as well as any cross-endian
hypervisors tricky.

-- 
MST

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, 
> > including byte order, is defined on a per-device type basis. RPMsg is 
> > indeed included in the spec as device type 7, but that's the only 
> > mention of it in both versions. It seems RPMsg over VirtIO isn't 
> > standardised yet.
> 
> Yes. And it would be very good to have some standartization before we
> keep adding things. For example without any spec if host code breaks
> with some guests, how do we know which side should be fixed?
> 
> > Also it looks like newer interface definitions 
> > specify using "guest native endianness" for Virtual Queue data.
> 
> They really don't or shouldn't. That's limited to legacy chapters.
> Some definitions could have slipped through but it's not
> the norm. I just quickly looked through the 1.1 spec and could
> not find any instances that specify "guest native endianness"
> but feel free to point them out to me.

Oh, there you go. No, sorry, my fault, it's the other way round: "guest 
native" is for legacy and LE is for current / v1.0 and up.

> > So 
> > I think the same should be done for RPMsg instead of enforcing LE?
> 
> That makes hardware implementations as well as any cross-endian
> hypervisors tricky.

Yes, LE it is then. And we need to add some text to the spec.

In theory there could be a backward compatibility issue - in case someone 
was already using virtio_rpmsg_bus.c in BE mode, but I very much doubt 
that...

Thanks
Guennadi

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Guennadi Liakhovetski]

On Mon, Jun 08, 2020 at 12:15:26PM +0200, Guennadi Liakhovetski wrote:
> On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, 
> > > including byte order, is defined on a per-device type basis. RPMsg is 
> > > indeed included in the spec as device type 7, but that's the only 
> > > mention of it in both versions. It seems RPMsg over VirtIO isn't 
> > > standardised yet.
> > 
> > Yes. And it would be very good to have some standartization before we
> > keep adding things. For example without any spec if host code breaks
> > with some guests, how do we know which side should be fixed?
> > 
> > > Also it looks like newer interface definitions 
> > > specify using "guest native endianness" for Virtual Queue data.
> > 
> > They really don't or shouldn't. That's limited to legacy chapters.
> > Some definitions could have slipped through but it's not
> > the norm. I just quickly looked through the 1.1 spec and could
> > not find any instances that specify "guest native endianness"
> > but feel free to point them out to me.
> 
> Oh, there you go. No, sorry, my fault, it's the other way round: "guest 
> native" is for legacy and LE is for current / v1.0 and up.
> 
> > > So 
> > > I think the same should be done for RPMsg instead of enforcing LE?
> > 
> > That makes hardware implementations as well as any cross-endian
> > hypervisors tricky.
> 
> Yes, LE it is then. And we need to add some text to the spec.

I found the protocol and the message format definition: 
https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol#transport-layer---rpmsg 
Don't know what the best way for referencing it in the VirtIO standard 
would be: just a link to the source or a quote.

Thanks
Guennadi

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 01:16:38PM +0200, Guennadi Liakhovetski wrote:
> On Mon, Jun 08, 2020 at 12:15:26PM +0200, Guennadi Liakhovetski wrote:
> > On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote:
> > > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> > > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, 
> > > > including byte order, is defined on a per-device type basis. RPMsg is 
> > > > indeed included in the spec as device type 7, but that's the only 
> > > > mention of it in both versions. It seems RPMsg over VirtIO isn't 
> > > > standardised yet.
> > > 
> > > Yes. And it would be very good to have some standartization before we
> > > keep adding things. For example without any spec if host code breaks
> > > with some guests, how do we know which side should be fixed?
> > > 
> > > > Also it looks like newer interface definitions 
> > > > specify using "guest native endianness" for Virtual Queue data.
> > > 
> > > They really don't or shouldn't. That's limited to legacy chapters.
> > > Some definitions could have slipped through but it's not
> > > the norm. I just quickly looked through the 1.1 spec and could
> > > not find any instances that specify "guest native endianness"
> > > but feel free to point them out to me.
> > 
> > Oh, there you go. No, sorry, my fault, it's the other way round: "guest 
> > native" is for legacy and LE is for current / v1.0 and up.
> > 
> > > > So 
> > > > I think the same should be done for RPMsg instead of enforcing LE?
> > > 
> > > That makes hardware implementations as well as any cross-endian
> > > hypervisors tricky.
> > 
> > Yes, LE it is then. And we need to add some text to the spec.
> 
> I found the protocol and the message format definition: 
> https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol#transport-layer---rpmsg 
> Don't know what the best way for referencing it in the VirtIO standard 
> would be: just a link to the source or a quote.
> 
> Thanks
> Guennadi

I wasn't aware of that one, thanks!
OK so that's good.

Ideally we'd have RPMsg Header Definition, RPMsg Channel and RPMsg
Endppint in the spec proper.

This link is informal so can't be copied into spec as is but can be used as a basis.

We'd also need approval from authors for inclusion in the spec,
sent to the TC mailing list.

-- 
MST

Re: [PATCH v3 0/5] Add a vhost RPMsg API

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 12:15:27PM +0200, Guennadi Liakhovetski wrote:
> On Mon, Jun 08, 2020 at 05:19:06AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2020 at 11:11:00AM +0200, Guennadi Liakhovetski wrote:
> > > Update: I looked through VirtIO 1.0 and 1.1 specs, data format their, 
> > > including byte order, is defined on a per-device type basis. RPMsg is 
> > > indeed included in the spec as device type 7, but that's the only 
> > > mention of it in both versions. It seems RPMsg over VirtIO isn't 
> > > standardised yet.
> > 
> > Yes. And it would be very good to have some standartization before we
> > keep adding things. For example without any spec if host code breaks
> > with some guests, how do we know which side should be fixed?
> > 
> > > Also it looks like newer interface definitions 
> > > specify using "guest native endianness" for Virtual Queue data.
> > 
> > They really don't or shouldn't. That's limited to legacy chapters.
> > Some definitions could have slipped through but it's not
> > the norm. I just quickly looked through the 1.1 spec and could
> > not find any instances that specify "guest native endianness"
> > but feel free to point them out to me.
> 
> Oh, there you go. No, sorry, my fault, it's the other way round: "guest 
> native" is for legacy and LE is for current / v1.0 and up.
> 
> > > So 
> > > I think the same should be done for RPMsg instead of enforcing LE?
> > 
> > That makes hardware implementations as well as any cross-endian
> > hypervisors tricky.
> 
> Yes, LE it is then. And we need to add some text to the spec.
> 
> In theory there could be a backward compatibility issue - in case someone 
> was already using virtio_rpmsg_bus.c in BE mode, but I very much doubt 
> that...
> 
> Thanks
> Guennadi

It's probably easiest to use virtio wrappers and then we don't need to
worry about it.

-- 
MST

Re: [Sound-open-firmware] [PATCH v3 0/5] Add a vhost RPMsg API

[Liam Girdwood]

On Thu, 2020-06-04 at 15:23 -0400, Michael S. Tsirkin wrote:
> On Wed, May 27, 2020 at 08:05:36PM +0200, Guennadi Liakhovetski
> wrote:
> > v3:
> > - address several checkpatch warnings
> > - address comments from Mathieu Poirier
> > 
> > v2:
> > - update patch #5 with a correct vhost_dev_init() prototype
> > - drop patch #6 - it depends on a different patch, that is
> > currently
> >   an RFC
> > - address comments from Pierre-Louis Bossart:
> >   * remove "default n" from Kconfig
> > 
> > Linux supports RPMsg over VirtIO for "remote processor" /AMP use
> > cases. It can however also be used for virtualisation scenarios,
> > e.g. when using KVM to run Linux on both the host and the guests.
> > This patch set adds a wrapper API to facilitate writing vhost
> > drivers for such RPMsg-based solutions. The first use case is an
> > audio DSP virtualisation project, currently under development,
> > ready
> > for review and submission, available at
> > https://github.com/thesofproject/linux/pull/1501/commits
> > A further patch for the ADSP vhost RPMsg driver will be sent
> > separately for review only since it cannot be merged without audio
> > patches being upstreamed first.
> 
> RPMsg over virtio has several problems. One is that it's
> not specced at all. Before we add more stuff, I'd like so
> see at least an attempt at describing what it's supposed to do.
> 

Sure, I'll add some more context here. The remote processor in this use
case is any DSP (from any vendor) running SOF. The work from Guennadi
virtualises the SOF mailbox and SOF doorbell mechanisms (which the
platform driver abstracts) via rpmsg/virtio so the guest SOF drivers
can send and receive SOF IPCs (just as the host SOF driver would do).
It's 95% the same Linux driver on host or guest (for each feature).

I would also add here (and it's maybe confusing in the SOF naming) that
SOF is multi a feature FW, it's not just an audio FW, so we would also
expect to see other guest drivers (e.g. sensing) that would use the
same mechanism for IPC on guests. I would expect the feature driver
count to increase as the FW features grow.

The IPC ABI between the FW and host drivers continually evolves as
features and new HW is added (not just from Intel, but from other SOF
partners and external partners that supply proprietary audio
processing). The only part of the interface that is specced is the
rpmsg header, as the SOF message content will keep evolving (it's up to
driver and FW to align on ABI version used - it does this already
today). 

I guess it boils down to two goals here

1) virtualising the SOF features on any platform/guest/OS so that
guests would be able to access any FW feature (provided guest was
permitted) just as they would on host.

2) Supporting FW features and use cases from multiple parties without
having to change driver core or driver virtualisation core. i.e. all
the changes (for new features) would be in the edge drivers e.g. new
audio features would impact audio driver only.

> Another it's out of line with 1.0 spec passing guest
> endian data around. Won't work if host and guest
> endian-ness do not match. Should pass eveything in LE and
> convert.
> 

I think Guennadi is working on this now.

> It's great to see it's seeing active development finally.
> Do you think you will have time to address these?
> 

Yes, of course. Let me know if you need any more background or context.

Thanks

Liam

> 
> 
> > Thanks
> > Guennadi
> > 
> > Guennadi Liakhovetski (5):
> >   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
> >   vhost: (cosmetic) remove a superfluous variable initialisation
> >   rpmsg: move common structures and defines to headers
> >   rpmsg: update documentation
> >   vhost: add an RPMsg API
> > 
> >  Documentation/rpmsg.txt          |   6 +-
> >  drivers/rpmsg/virtio_rpmsg_bus.c |  78 +-------
> >  drivers/vhost/Kconfig            |   7 +
> >  drivers/vhost/Makefile           |   3 +
> >  drivers/vhost/rpmsg.c            | 382
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/vhost/vhost.c            |   2 +-
> >  drivers/vhost/vhost_rpmsg.h      |  74 ++++++++
> >  include/linux/virtio_rpmsg.h     |  81 +++++++++
> >  include/uapi/linux/rpmsg.h       |   3 +
> >  include/uapi/linux/vhost.h       |   4 +-
> >  10 files changed, 559 insertions(+), 81 deletions(-)
> >  create mode 100644 drivers/vhost/rpmsg.c
> >  create mode 100644 drivers/vhost/vhost_rpmsg.h
> >  create mode 100644 include/linux/virtio_rpmsg.h
> > 
> > -- 
> > 1.9.3
> 
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware