[PATCH v6 0/4] Add a vhost RPMsg API

[PATCH v6 0/4] Add a vhost RPMsg API

[Guennadi Liakhovetski]

Hi,

Next update:

v6:
- rename include/linux/virtio_rpmsg.h -> include/linux/rpmsg/virtio.h

v5:
- don't hard-code message layout

v4:
- add endianness conversions to comply with the VirtIO standard

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

Thanks
Guennadi

Guennadi Liakhovetski (4):
  vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
  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            | 373 +++++++++++++++++++++++++++++++
 drivers/vhost/vhost_rpmsg.h      |  74 ++++++
 include/linux/rpmsg/virtio.h     |  83 +++++++
 include/uapi/linux/rpmsg.h       |   3 +
 include/uapi/linux/vhost.h       |   4 +-
 9 files changed, 551 insertions(+), 80 deletions(-)
 create mode 100644 drivers/vhost/rpmsg.c
 create mode 100644 drivers/vhost/vhost_rpmsg.h
 create mode 100644 include/linux/rpmsg/virtio.h

-- 
2.28.0

[PATCH v6 1/4] 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 75232185324a..11a4948b6216 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -97,6 +97,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.
@@ -118,7 +120,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 */
 
-- 
2.28.0

[PATCH v6 2/4] 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.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
 include/linux/rpmsg/virtio.h     | 83 ++++++++++++++++++++++++++++++++
 include/uapi/linux/rpmsg.h       |  3 ++
 3 files changed, 88 insertions(+), 76 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio.h

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 9006fc7f73d0..f39c426f9c5e 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -19,6 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/rpmsg.h>
+#include <linux/rpmsg/virtio.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
@@ -27,6 +28,7 @@
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 #include <linux/wait.h>
+#include <uapi/linux/rpmsg.h>
 
 #include "rpmsg_internal.h"
 
@@ -70,58 +72,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 {
-	__virtio32 src;
-	__virtio32 dst;
-	__virtio32 reserved;
-	__virtio16 len;
-	__virtio16 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];
-	__virtio32 addr;
-	__virtio32 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,27 +84,6 @@ struct virtio_rpmsg_channel {
 #define to_virtio_rpmsg_channel(_rpdev) \
 	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,
@@ -162,9 +91,6 @@ struct virtio_rpmsg_channel {
  */
 #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/rpmsg/virtio.h b/include/linux/rpmsg/virtio.h
new file mode 100644
index 000000000000..3ede1a4a68a3
--- /dev/null
+++ b/include/linux/rpmsg/virtio.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_RPMSG_VIRTIO_H
+#define _LINUX_RPMSG_VIRTIO_H
+
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+#include <linux/virtio_types.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 {
+	__virtio32 src;
+	__virtio32 dst;
+	__virtio32 reserved;
+	__virtio16 len;
+	__virtio16 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];
+	__virtio32 addr;
+	__virtio32 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 e14c6dab4223..d669c04ef289 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
-- 
2.28.0

[PATCH v6 3/4] 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 24b7a9e1a5f9..1ce353cb232a 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
-- 
2.28.0

[PATCH v6 4/4] 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       | 373 ++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost_rpmsg.h |  74 +++++++
 4 files changed, 457 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 587fbae06182..046b948fc411 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
+	select 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 f3e1897cce85..9cf459d59f97 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 000000000000..2fa527121a8f
--- /dev/null
+++ b/drivers/vhost/rpmsg.c
@@ -0,0 +1,373 @@
+// 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/rpmsg/virtio.h>
+#include <linux/vhost.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, but we can also handle split messages
+ */
+static int vhost_rpmsg_get_msg(struct vhost_virtqueue *vq, unsigned int *cnt)
+{
+	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]) {
+		if (out) {
+			vq_err(vq, "%s(): invalid %d output in response queue\n",
+			       __func__, out);
+			goto return_buf;
+		}
+
+		*cnt = in;
+	}
+
+	if (vq == &vr->vq[VIRTIO_RPMSG_REQUEST]) {
+		if (in) {
+			vq_err(vq, "%s(): invalid %d input in request queue\n",
+		       __func__, in);
+			goto return_buf;
+		}
+
+		*cnt = out;
+	}
+
+	return head;
+
+return_buf:
+	vhost_add_used(vq, head, 0);
+
+	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;
+	unsigned int cnt;
+	ssize_t ret;
+	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_msg(vq, &cnt);
+	if (iter->head == vq->num)
+		iter->head = -EAGAIN;
+
+	if (iter->head < 0) {
+		ret = iter->head;
+		goto unlock;
+	}
+
+	tmp = iov_length(vq->iov, cnt);
+	if (tmp < sizeof(iter->rhdr)) {
+		vq_err(vq, "%s(): size %zu too small\n", __func__, tmp);
+		ret = -ENOBUFS;
+		goto return_buf;
+	}
+
+	switch (qid) {
+	case VIRTIO_RPMSG_REQUEST:
+		if (len >= 0) {
+			if (tmp < sizeof(iter->rhdr) + len) {
+				ret = -ENOBUFS;
+				goto return_buf;
+			}
+
+			tmp = len + sizeof(iter->rhdr);
+		}
+
+		/* len is now the size of the payload */
+		iov_iter_init(&iter->iov_iter, WRITE, vq->iov, cnt, tmp);
+
+		/* 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));
+			ret = -EIO;
+			goto return_buf;
+		}
+
+		iter->ept = vhost_rpmsg_ept_find(vr, vhost32_to_cpu(vq, iter->rhdr.dst));
+		if (!iter->ept) {
+			vq_err(vq, "%s(): no endpoint with address %d\n",
+			       __func__, vhost32_to_cpu(vq, iter->rhdr.dst));
+			ret = -ENOENT;
+			goto return_buf;
+		}
+
+		/* Let the endpoint read the payload */
+		if (iter->ept->read) {
+			ret = iter->ept->read(vr, iter);
+			if (ret < 0)
+				goto return_buf;
+
+			iter->rhdr.len = cpu_to_vhost16(vq, ret);
+		} else {
+			iter->rhdr.len = 0;
+		}
+
+		/* Prepare for the response phase */
+		iter->rhdr.dst = iter->rhdr.src;
+		iter->rhdr.src = cpu_to_vhost32(vq, iter->ept->addr);
+
+		break;
+	case VIRTIO_RPMSG_RESPONSE:
+		if (!iter->ept && iter->rhdr.dst != cpu_to_vhost32(vq, 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, vhost32_to_cpu(vq, iter->rhdr.src));
+			if (!iter->ept) {
+				ret = -ENOENT;
+				goto return_buf;
+			}
+		}
+
+		if (len >= 0) {
+			if (tmp < sizeof(iter->rhdr) + len) {
+				ret = -ENOBUFS;
+				goto return_buf;
+			}
+
+			iter->rhdr.len = cpu_to_vhost16(vq, len);
+			tmp = len + sizeof(iter->rhdr);
+		}
+
+		/* len is now the size of the payload */
+		iov_iter_init(&iter->iov_iter, READ, vq->iov, cnt, tmp);
+
+		/* Write the RPMSG header with endpoint addresses */
+		tmp = copy_to_iter(&iter->rhdr, sizeof(iter->rhdr), &iter->iov_iter);
+		if (tmp != sizeof(iter->rhdr)) {
+			ret = -EIO;
+			goto return_buf;
+		}
+
+		/* Let the endpoint write the payload */
+		if (iter->ept && iter->ept->write) {
+			ret = iter->ept->write(vr, iter);
+			if (ret < 0)
+				goto return_buf;
+		}
+
+		break;
+	}
+
+	return 0;
+
+return_buf:
+	vhost_add_used(vq, iter->head, 0);
+unlock:
+	vhost_enable_notify(&vr->dev, vq);
+	mutex_unlock(&vq->mutex);
+
+	return ret;
+}
+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,
+					  vhost16_to_cpu(iter->vq, 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, true, 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_virtqueue *vq = &vr->vq[VIRTIO_RPMSG_RESPONSE];
+	struct vhost_rpmsg_iter iter = {
+		.rhdr = {
+			.src = 0,
+			.dst = cpu_to_vhost32(vq, RPMSG_NS_ADDR),
+			.flags = cpu_to_vhost16(vq, RPMSG_NS_CREATE), /* rpmsg_recv_single() */
+		},
+	};
+	struct rpmsg_ns_msg ns = {
+		.addr = cpu_to_vhost32(vq, src),
+		.flags = cpu_to_vhost32(vq, 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 000000000000..c020ea14cd16
--- /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/rpmsg/virtio.h>
+#include <linux/uio.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(_vq, _src, _dst) {			\
+	.rhdr = {						\
+			.src = cpu_to_vhost32(_vq, _src),	\
+			.dst = cpu_to_vhost32(_vq, _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
-- 
2.28.0

Re: [PATCH v6 2/4] rpmsg: move common structures and defines to headers

[Mathieu Poirier]

On Tue, Sep 01, 2020 at 05:11:51PM +0200, Guennadi Liakhovetski wrote:
> 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.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

I already reviewed this patch and added my RB to it.  Please carry it in your
next revision.

Thanks,
Mathieu

> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
>  include/linux/rpmsg/virtio.h     | 83 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/rpmsg.h       |  3 ++
>  3 files changed, 88 insertions(+), 76 deletions(-)
>  create mode 100644 include/linux/rpmsg/virtio.h
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 9006fc7f73d0..f39c426f9c5e 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -19,6 +19,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/rpmsg.h>
> +#include <linux/rpmsg/virtio.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> @@ -27,6 +28,7 @@
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_config.h>
>  #include <linux/wait.h>
> +#include <uapi/linux/rpmsg.h>
>  
>  #include "rpmsg_internal.h"
>  
> @@ -70,58 +72,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 {
> -	__virtio32 src;
> -	__virtio32 dst;
> -	__virtio32 reserved;
> -	__virtio16 len;
> -	__virtio16 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];
> -	__virtio32 addr;
> -	__virtio32 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,27 +84,6 @@ struct virtio_rpmsg_channel {
>  #define to_virtio_rpmsg_channel(_rpdev) \
>  	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,
> @@ -162,9 +91,6 @@ struct virtio_rpmsg_channel {
>   */
>  #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/rpmsg/virtio.h b/include/linux/rpmsg/virtio.h
> new file mode 100644
> index 000000000000..3ede1a4a68a3
> --- /dev/null
> +++ b/include/linux/rpmsg/virtio.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_RPMSG_VIRTIO_H
> +#define _LINUX_RPMSG_VIRTIO_H
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +#include <linux/virtio_types.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 {
> +	__virtio32 src;
> +	__virtio32 dst;
> +	__virtio32 reserved;
> +	__virtio16 len;
> +	__virtio16 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];
> +	__virtio32 addr;
> +	__virtio32 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 e14c6dab4223..d669c04ef289 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
> -- 
> 2.28.0
> 

Re: [PATCH v6 2/4] rpmsg: move common structures and defines to headers

[Guennadi Liakhovetski]

Hi Mathew,

On Tue, Sep 01, 2020 at 11:23:21AM -0600, Mathieu Poirier wrote:
> On Tue, Sep 01, 2020 at 05:11:51PM +0200, Guennadi Liakhovetski wrote:
> > 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.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> 
> I already reviewed this patch and added my RB to it.  Please carry it in your
> next revision.

But only as long as the patch doesn't change, right? And in fact it did change 
this time - I renamed the header, moving it under include/linux/rpmsg, actually 
also following your suggestion. Still, formally the patch has changed, so, I 
didn't include your "Reviewed-by" in this version. And you anyway would be 
reviewing the other patches in this series to, right?

Thanks
Guennadi

> Thanks,
> Mathieu
> 
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
> >  include/linux/rpmsg/virtio.h     | 83 ++++++++++++++++++++++++++++++++
> >  include/uapi/linux/rpmsg.h       |  3 ++
> >  3 files changed, 88 insertions(+), 76 deletions(-)
> >  create mode 100644 include/linux/rpmsg/virtio.h
> > 
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 9006fc7f73d0..f39c426f9c5e 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> >  #include <linux/rpmsg.h>
> > +#include <linux/rpmsg/virtio.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/slab.h>
> >  #include <linux/sched.h>
> > @@ -27,6 +28,7 @@
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> >  #include <linux/wait.h>
> > +#include <uapi/linux/rpmsg.h>
> >  
> >  #include "rpmsg_internal.h"
> >  
> > @@ -70,58 +72,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 {
> > -	__virtio32 src;
> > -	__virtio32 dst;
> > -	__virtio32 reserved;
> > -	__virtio16 len;
> > -	__virtio16 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];
> > -	__virtio32 addr;
> > -	__virtio32 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,27 +84,6 @@ struct virtio_rpmsg_channel {
> >  #define to_virtio_rpmsg_channel(_rpdev) \
> >  	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,
> > @@ -162,9 +91,6 @@ struct virtio_rpmsg_channel {
> >   */
> >  #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/rpmsg/virtio.h b/include/linux/rpmsg/virtio.h
> > new file mode 100644
> > index 000000000000..3ede1a4a68a3
> > --- /dev/null
> > +++ b/include/linux/rpmsg/virtio.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_RPMSG_VIRTIO_H
> > +#define _LINUX_RPMSG_VIRTIO_H
> > +
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/types.h>
> > +#include <linux/virtio_types.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 {
> > +	__virtio32 src;
> > +	__virtio32 dst;
> > +	__virtio32 reserved;
> > +	__virtio16 len;
> > +	__virtio16 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];
> > +	__virtio32 addr;
> > +	__virtio32 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 e14c6dab4223..d669c04ef289 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
> > -- 
> > 2.28.0
> > 

Re: [PATCH v6 2/4] rpmsg: move common structures and defines to headers

[Mathieu Poirier]

On Wed, Sep 02, 2020 at 07:35:27AM +0200, Guennadi Liakhovetski wrote:
> Hi Mathew,
> 
> On Tue, Sep 01, 2020 at 11:23:21AM -0600, Mathieu Poirier wrote:
> > On Tue, Sep 01, 2020 at 05:11:51PM +0200, Guennadi Liakhovetski wrote:
> > > 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.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > 
> > I already reviewed this patch and added my RB to it.  Please carry it in your
> > next revision.
> 
> But only as long as the patch doesn't change, right? And in fact it did change 
> this time - I renamed the header, moving it under include/linux/rpmsg, actually 

Patch 2/4 in V5 was identical to what was submitted in V4 and my RB tag was not
added, nor was the entry for virtio_rpmsg.h added to the MAINTAINERS file.

> also following your suggestion. Still, formally the patch has changed, so, I 
> didn't include your "Reviewed-by" in this version. And you anyway would be 

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

> reviewing the other patches in this series to, right?

As much as I'd like to reviewing the other patches in this series won't be
possible at this time. 

> 
> Thanks
> Guennadi
> 
> > Thanks,
> > Mathieu
> > 
> > > ---
> > >  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
> > >  include/linux/rpmsg/virtio.h     | 83 ++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/rpmsg.h       |  3 ++
> > >  3 files changed, 88 insertions(+), 76 deletions(-)
> > >  create mode 100644 include/linux/rpmsg/virtio.h
> > > 
> > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > index 9006fc7f73d0..f39c426f9c5e 100644
> > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/mutex.h>
> > >  #include <linux/of_device.h>
> > >  #include <linux/rpmsg.h>
> > > +#include <linux/rpmsg/virtio.h>
> > >  #include <linux/scatterlist.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/sched.h>
> > > @@ -27,6 +28,7 @@
> > >  #include <linux/virtio_ids.h>
> > >  #include <linux/virtio_config.h>
> > >  #include <linux/wait.h>
> > > +#include <uapi/linux/rpmsg.h>
> > >  
> > >  #include "rpmsg_internal.h"
> > >  
> > > @@ -70,58 +72,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 {
> > > -	__virtio32 src;
> > > -	__virtio32 dst;
> > > -	__virtio32 reserved;
> > > -	__virtio16 len;
> > > -	__virtio16 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];
> > > -	__virtio32 addr;
> > > -	__virtio32 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,27 +84,6 @@ struct virtio_rpmsg_channel {
> > >  #define to_virtio_rpmsg_channel(_rpdev) \
> > >  	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,
> > > @@ -162,9 +91,6 @@ struct virtio_rpmsg_channel {
> > >   */
> > >  #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/rpmsg/virtio.h b/include/linux/rpmsg/virtio.h
> > > new file mode 100644
> > > index 000000000000..3ede1a4a68a3
> > > --- /dev/null
> > > +++ b/include/linux/rpmsg/virtio.h
> > > @@ -0,0 +1,83 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef _LINUX_RPMSG_VIRTIO_H
> > > +#define _LINUX_RPMSG_VIRTIO_H
> > > +
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/types.h>
> > > +#include <linux/virtio_types.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 {
> > > +	__virtio32 src;
> > > +	__virtio32 dst;
> > > +	__virtio32 reserved;
> > > +	__virtio16 len;
> > > +	__virtio16 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];
> > > +	__virtio32 addr;
> > > +	__virtio32 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 e14c6dab4223..d669c04ef289 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
> > > -- 
> > > 2.28.0
> > > 

Re: [PATCH v6 2/4] rpmsg: move common structures and defines to headers

[Guennadi Liakhovetski]

Hi Mathieu,

On Wed, Sep 02, 2020 at 11:24:37AM -0600, Mathieu Poirier wrote:
> On Wed, Sep 02, 2020 at 07:35:27AM +0200, Guennadi Liakhovetski wrote:
> > Hi Mathew,
> > 
> > On Tue, Sep 01, 2020 at 11:23:21AM -0600, Mathieu Poirier wrote:
> > > On Tue, Sep 01, 2020 at 05:11:51PM +0200, Guennadi Liakhovetski wrote:
> > > > 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.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > > 
> > > I already reviewed this patch and added my RB to it.  Please carry it in your
> > > next revision.
> > 
> > But only as long as the patch doesn't change, right? And in fact it did change 
> > this time - I renamed the header, moving it under include/linux/rpmsg, actually 
> 
> Patch 2/4 in V5 was identical to what was submitted in V4 and my RB tag was not
> added, nor was the entry for virtio_rpmsg.h added to the MAINTAINERS file.

Right, yes, I forgot about that your request when creating v5, sorry about that, 
that's why I made a v6 with the header moved to include/linux/rpmsg/.

> > also following your suggestion. Still, formally the patch has changed, so, I 
> > didn't include your "Reviewed-by" in this version. And you anyway would be 
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> 
> > reviewing the other patches in this series to, right?
> 
> As much as I'd like to reviewing the other patches in this series won't be
> possible at this time. 

Ok, understand, I'm wondering then what the path to upstreaming would be then?

Thanks
Guennadi

> > > Thanks,
> > > Mathieu
> > > 
> > > > ---
> > > >  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
> > > >  include/linux/rpmsg/virtio.h     | 83 ++++++++++++++++++++++++++++++++
> > > >  include/uapi/linux/rpmsg.h       |  3 ++
> > > >  3 files changed, 88 insertions(+), 76 deletions(-)
> > > >  create mode 100644 include/linux/rpmsg/virtio.h
> > > > 
> > > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > index 9006fc7f73d0..f39c426f9c5e 100644
> > > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/mutex.h>
> > > >  #include <linux/of_device.h>
> > > >  #include <linux/rpmsg.h>
> > > > +#include <linux/rpmsg/virtio.h>
> > > >  #include <linux/scatterlist.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/sched.h>
> > > > @@ -27,6 +28,7 @@
> > > >  #include <linux/virtio_ids.h>
> > > >  #include <linux/virtio_config.h>
> > > >  #include <linux/wait.h>
> > > > +#include <uapi/linux/rpmsg.h>
> > > >  
> > > >  #include "rpmsg_internal.h"
> > > >  
> > > > @@ -70,58 +72,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 {
> > > > -	__virtio32 src;
> > > > -	__virtio32 dst;
> > > > -	__virtio32 reserved;
> > > > -	__virtio16 len;
> > > > -	__virtio16 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];
> > > > -	__virtio32 addr;
> > > > -	__virtio32 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,27 +84,6 @@ struct virtio_rpmsg_channel {
> > > >  #define to_virtio_rpmsg_channel(_rpdev) \
> > > >  	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,
> > > > @@ -162,9 +91,6 @@ struct virtio_rpmsg_channel {
> > > >   */
> > > >  #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/rpmsg/virtio.h b/include/linux/rpmsg/virtio.h
> > > > new file mode 100644
> > > > index 000000000000..3ede1a4a68a3
> > > > --- /dev/null
> > > > +++ b/include/linux/rpmsg/virtio.h
> > > > @@ -0,0 +1,83 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +
> > > > +#ifndef _LINUX_RPMSG_VIRTIO_H
> > > > +#define _LINUX_RPMSG_VIRTIO_H
> > > > +
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/types.h>
> > > > +#include <linux/virtio_types.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 {
> > > > +	__virtio32 src;
> > > > +	__virtio32 dst;
> > > > +	__virtio32 reserved;
> > > > +	__virtio16 len;
> > > > +	__virtio16 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];
> > > > +	__virtio32 addr;
> > > > +	__virtio32 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 e14c6dab4223..d669c04ef289 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
> > > > -- 
> > > > 2.28.0
> > > > 

Re: [PATCH v6 3/4] rpmsg: update documentation

[Mathieu Poirier]

On Tue, Sep 01, 2020 at 05:11:52PM +0200, Guennadi Liakhovetski 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 24b7a9e1a5f9..1ce353cb232a 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);
>

I added my RB tag to this patch in V3.
 
>  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
> -- 
> 2.28.0
> 

Re: [PATCH v6 2/4] rpmsg: move common structures and defines to headers

[Mathieu Poirier]

On Thu, Sep 03, 2020 at 07:51:48AM +0200, Guennadi Liakhovetski wrote:
> Hi Mathieu,
> 
> On Wed, Sep 02, 2020 at 11:24:37AM -0600, Mathieu Poirier wrote:
> > On Wed, Sep 02, 2020 at 07:35:27AM +0200, Guennadi Liakhovetski wrote:
> > > Hi Mathew,
> > > 
> > > On Tue, Sep 01, 2020 at 11:23:21AM -0600, Mathieu Poirier wrote:
> > > > On Tue, Sep 01, 2020 at 05:11:51PM +0200, Guennadi Liakhovetski wrote:
> > > > > 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.
> > > > > 
> > > > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > > > 
> > > > I already reviewed this patch and added my RB to it.  Please carry it in your
> > > > next revision.
> > > 
> > > But only as long as the patch doesn't change, right? And in fact it did change 
> > > this time - I renamed the header, moving it under include/linux/rpmsg, actually 
> > 
> > Patch 2/4 in V5 was identical to what was submitted in V4 and my RB tag was not
> > added, nor was the entry for virtio_rpmsg.h added to the MAINTAINERS file.
> 
> Right, yes, I forgot about that your request when creating v5, sorry about that, 
> that's why I made a v6 with the header moved to include/linux/rpmsg/.
> 
> > > also following your suggestion. Still, formally the patch has changed, so, I 
> > > didn't include your "Reviewed-by" in this version. And you anyway would be 
> > 
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > 
> > > reviewing the other patches in this series to, right?
> > 
> > As much as I'd like to reviewing the other patches in this series won't be
> > possible at this time. 
> 
> Ok, understand, I'm wondering then what the path to upstreaming would be then?

I'm really interested by the work done in the virtualisation world but right now
I don't have the bandwidth to investigate further, which is a real conumdrum
for me.

Because the bulk of the new work introduced is related to vhost, I would
normally expect Michael to pick up the set once Bjorn had the chance to review
it.

> 
> Thanks
> Guennadi
> 
> > > > Thanks,
> > > > Mathieu
> > > > 
> > > > > ---
> > > > >  drivers/rpmsg/virtio_rpmsg_bus.c | 78 +-----------------------------
> > > > >  include/linux/rpmsg/virtio.h     | 83 ++++++++++++++++++++++++++++++++
> > > > >  include/uapi/linux/rpmsg.h       |  3 ++
> > > > >  3 files changed, 88 insertions(+), 76 deletions(-)
> > > > >  create mode 100644 include/linux/rpmsg/virtio.h
> > > > > 
> > > > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > > index 9006fc7f73d0..f39c426f9c5e 100644
> > > > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > > @@ -19,6 +19,7 @@
> > > > >  #include <linux/mutex.h>
> > > > >  #include <linux/of_device.h>
> > > > >  #include <linux/rpmsg.h>
> > > > > +#include <linux/rpmsg/virtio.h>
> > > > >  #include <linux/scatterlist.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/sched.h>
> > > > > @@ -27,6 +28,7 @@
> > > > >  #include <linux/virtio_ids.h>
> > > > >  #include <linux/virtio_config.h>
> > > > >  #include <linux/wait.h>
> > > > > +#include <uapi/linux/rpmsg.h>
> > > > >  
> > > > >  #include "rpmsg_internal.h"
> > > > >  
> > > > > @@ -70,58 +72,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 {
> > > > > -	__virtio32 src;
> > > > > -	__virtio32 dst;
> > > > > -	__virtio32 reserved;
> > > > > -	__virtio16 len;
> > > > > -	__virtio16 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];
> > > > > -	__virtio32 addr;
> > > > > -	__virtio32 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,27 +84,6 @@ struct virtio_rpmsg_channel {
> > > > >  #define to_virtio_rpmsg_channel(_rpdev) \
> > > > >  	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,
> > > > > @@ -162,9 +91,6 @@ struct virtio_rpmsg_channel {
> > > > >   */
> > > > >  #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/rpmsg/virtio.h b/include/linux/rpmsg/virtio.h
> > > > > new file mode 100644
> > > > > index 000000000000..3ede1a4a68a3
> > > > > --- /dev/null
> > > > > +++ b/include/linux/rpmsg/virtio.h
> > > > > @@ -0,0 +1,83 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +
> > > > > +#ifndef _LINUX_RPMSG_VIRTIO_H
> > > > > +#define _LINUX_RPMSG_VIRTIO_H
> > > > > +
> > > > > +#include <linux/mod_devicetable.h>
> > > > > +#include <linux/types.h>
> > > > > +#include <linux/virtio_types.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 {
> > > > > +	__virtio32 src;
> > > > > +	__virtio32 dst;
> > > > > +	__virtio32 reserved;
> > > > > +	__virtio16 len;
> > > > > +	__virtio16 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];
> > > > > +	__virtio32 addr;
> > > > > +	__virtio32 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 e14c6dab4223..d669c04ef289 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
> > > > > -- 
> > > > > 2.28.0
> > > > > 

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Arnaud POULIQUEN]

Hi  Guennadi,

On 9/1/20 5:11 PM, Guennadi Liakhovetski wrote:
> Hi,
> 
> Next update:
> 
> v6:
> - rename include/linux/virtio_rpmsg.h -> include/linux/rpmsg/virtio.h
> 
> v5:
> - don't hard-code message layout
> 
> v4:
> - add endianness conversions to comply with the VirtIO standard
> 
> 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

Mathieu pointed me your series. On my side i proposed the rpmsg_ns_msg
service[1] that does not match with your implementation.
As i come late, i hope that i did not miss something in the history...
Don't hesitate to point me the discussions, if it is the case.

Regarding your patchset, it is quite confusing for me. It seems that you
implement your own protocol on top of vhost forked from the RPMsg one.
But look to me that it is not the RPMsg protocol.

So i would be agree with Vincent[2] which proposed to switch on a RPMsg API
and creating a vhost rpmsg device. This is also proposed in the 
"Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
Do you think that this alternative could match with your need?

[1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335 
[2]. https://www.spinics.net/lists/linux-virtualization/msg44195.html
[3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html  

Thanks,
Arnaud

> 
> Thanks
> Guennadi
> 
> Guennadi Liakhovetski (4):
>   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
>   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            | 373 +++++++++++++++++++++++++++++++
>  drivers/vhost/vhost_rpmsg.h      |  74 ++++++
>  include/linux/rpmsg/virtio.h     |  83 +++++++
>  include/uapi/linux/rpmsg.h       |   3 +
>  include/uapi/linux/vhost.h       |   4 +-
>  9 files changed, 551 insertions(+), 80 deletions(-)
>  create mode 100644 drivers/vhost/rpmsg.c
>  create mode 100644 drivers/vhost/vhost_rpmsg.h
>  create mode 100644 include/linux/rpmsg/virtio.h
> 

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Vincent Whitchurch]

On Thu, Sep 17, 2020 at 07:47:06AM +0200, Guennadi Liakhovetski wrote:
> On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
> > So i would be agree with Vincent[2] which proposed to switch on a RPMsg API
> > and creating a vhost rpmsg device. This is also proposed in the 
> > "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
> > Do you think that this alternative could match with your need?
> 
> As I replied to Vincent, I understand his proposal and the approach taken 
> in the series [3], but I'm not sure I agree, that adding yet another 
> virtual device / driver layer on the vhost side is a good idea. As far as 
> I understand adding new completely virtual devices isn't considered to be 
> a good practice in the kernel. Currently vhost is just a passive "library" 
> and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of 
> converting vhost to a virtual device infrastructure.

I know it wasn't what you meant, but I noticed that the above paragraph
could be read as if my suggestion was to convert vhost to a virtual
device infrastructure, so I just want to clarify that that those are not
related.  The only similarity between what I suggested in the thread in
[2] and Kishon's RFC in [3] is that both involve creating a generic
vhost-rpmsg driver which would allow the RPMsg API to be used for both
sides of the link, instead of introducing a new API just for the server
side.  That can be done without rewriting drivers/vhost/.

> > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335 
> > [2]. https://www.spinics.net/lists/linux-virtualization/msg44195.html
> > [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html  

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Guennadi Liakhovetski]

Hi Vincent,

On Thu, Sep 17, 2020 at 10:36:44AM +0200, Vincent Whitchurch wrote:
> On Thu, Sep 17, 2020 at 07:47:06AM +0200, Guennadi Liakhovetski wrote:
> > On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
> > > So i would be agree with Vincent[2] which proposed to switch on a RPMsg API
> > > and creating a vhost rpmsg device. This is also proposed in the 
> > > "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
> > > Do you think that this alternative could match with your need?
> > 
> > As I replied to Vincent, I understand his proposal and the approach taken 
> > in the series [3], but I'm not sure I agree, that adding yet another 
> > virtual device / driver layer on the vhost side is a good idea. As far as 
> > I understand adding new completely virtual devices isn't considered to be 
> > a good practice in the kernel. Currently vhost is just a passive "library" 
> > and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of 
> > converting vhost to a virtual device infrastructure.
> 
> I know it wasn't what you meant, but I noticed that the above paragraph
> could be read as if my suggestion was to convert vhost to a virtual
> device infrastructure, so I just want to clarify that that those are not
> related.  The only similarity between what I suggested in the thread in
> [2] and Kishon's RFC in [3] is that both involve creating a generic
> vhost-rpmsg driver which would allow the RPMsg API to be used for both
> sides of the link, instead of introducing a new API just for the server
> side.  That can be done without rewriting drivers/vhost/.

Thanks for the clarification. Another flexibility, that I'm trying to preserve 
with my approach is keeping direct access to iovec style data buffers for 
cases where that's the structure, that's already used by the respective 
driver on the host side. Since we already do packing and unpacking on the 
guest / client side, we don't need the same on the host / server side again.

Thanks
Guennadi

> > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335 
> > > [2]. https://www.spinics.net/lists/linux-virtualization/msg44195.html
> > > [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html  

RE: [PATCH v6 0/4] Add a vhost RPMsg API

[Arnaud POULIQUEN]

Hi Guennadi,

> -----Original Message-----
> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Sent: jeudi 17 septembre 2020 07:47
> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
> Cc: kvm@vger.kernel.org; linux-remoteproc@vger.kernel.org;
> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa-
> project.org; Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Liam
> Girdwood <liam.r.girdwood@linux.intel.com>; Michael S. Tsirkin
> <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Ohad Ben-Cohen
> <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Mathieu
> Poirier <mathieu.poirier@linaro.org>; Vincent Whitchurch
> <vincent.whitchurch@axis.com>
> Subject: Re: [PATCH v6 0/4] Add a vhost RPMsg API
> 
> Hi Arnaud,
> 
> On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
> > Hi  Guennadi,
> >
> > On 9/1/20 5:11 PM, Guennadi Liakhovetski wrote:
> > > Hi,
> > >
> > > Next update:
> > >
> > > v6:
> > > - rename include/linux/virtio_rpmsg.h ->
> > > include/linux/rpmsg/virtio.h
> > >
> > > v5:
> > > - don't hard-code message layout
> > >
> > > v4:
> > > - add endianness conversions to comply with the VirtIO standard
> > >
> > > 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
> >
> > Mathieu pointed me your series. On my side i proposed the rpmsg_ns_msg
> > service[1] that does not match with your implementation.
> > As i come late, i hope that i did not miss something in the history...
> > Don't hesitate to point me the discussions, if it is the case.
> 
> Well, as you see, this is a v6 only of this patch set, and apart from it there have
> been several side discussions and patch sets.
> 
> > Regarding your patchset, it is quite confusing for me. It seems that
> > you implement your own protocol on top of vhost forked from the RPMsg
> one.
> > But look to me that it is not the RPMsg protocol.
> 
> I'm implementing a counterpart to the rpmsg protocol over VirtIO as initially
> implemented by drivers/rpmsg/virtio_rpmsg_bus.c for the "main CPU" (in case
> of remoteproc over VirtIO) or the guest side in case of Linux virtualisation.
> Since my implementation can talk to that driver, I don't think, that I'm inventing
> a new protocol. I'm adding support for the same protocol for the opposite side
> of the VirtIO divide.

The main point I would like to highlight here is related to the use of the name "RPMsg"
more than how you implement your IPC protocol.
If It is a counterpart, it probably does not respect interface for RPMsg clients.
A good way to answer this, might be to respond to this question:
Is the rpmsg sample client[4] can be used on top of your vhost RPMsg implementation?
If the response is no, describe it as a RPMsg implementation could lead to confusion...

[4] https://elixir.bootlin.com/linux/v5.9-rc5/source/samples/rpmsg/rpmsg_client_sample.c

Regards,
Arnaud

> 
> > So i would be agree with Vincent[2] which proposed to switch on a
> > RPMsg API and creating a vhost rpmsg device. This is also proposed in
> > the "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
> > Do you think that this alternative could match with your need?
> 
> As I replied to Vincent, I understand his proposal and the approach taken in the
> series [3], but I'm not sure I agree, that adding yet another virtual device /
> driver layer on the vhost side is a good idea. As far as I understand adding new
> completely virtual devices isn't considered to be a good practice in the kernel.
> Currently vhost is just a passive "library"
> and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of
> converting vhost to a virtual device infrastructure.
> 
> Thanks for pointing me out at [3], I should have a better look at it.
> 
> Thanks
> Guennadi
> 
> > [1].
> > https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338
> > 335 [2].
> > https://www.spinics.net/lists/linux-virtualization/msg44195.html
> > [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html
> >
> > Thanks,
> > Arnaud
> >
> > >
> > > Thanks
> > > Guennadi
> > >
> > > Guennadi Liakhovetski (4):
> > >   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
> > >   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            | 373 +++++++++++++++++++++++++++++++
> > >  drivers/vhost/vhost_rpmsg.h      |  74 ++++++
> > >  include/linux/rpmsg/virtio.h     |  83 +++++++
> > >  include/uapi/linux/rpmsg.h       |   3 +
> > >  include/uapi/linux/vhost.h       |   4 +-
> > >  9 files changed, 551 insertions(+), 80 deletions(-)  create mode
> > > 100644 drivers/vhost/rpmsg.c  create mode 100644
> > > drivers/vhost/vhost_rpmsg.h  create mode 100644
> > > include/linux/rpmsg/virtio.h
> > >

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Arnaud POULIQUEN]

Hi Guennadi,

On 9/18/20 7:44 AM, Guennadi Liakhovetski wrote:
> Hi Arnaud,
> 
> On Thu, Sep 17, 2020 at 05:21:02PM +0200, Arnaud POULIQUEN wrote:
>> Hi Guennadi,
>>
>>> -----Original Message-----
>>> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
>>> Sent: jeudi 17 septembre 2020 07:47
>>> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
>>> Cc: kvm@vger.kernel.org; linux-remoteproc@vger.kernel.org;
>>> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa-
>>> project.org; Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Liam
>>> Girdwood <liam.r.girdwood@linux.intel.com>; Michael S. Tsirkin
>>> <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Ohad Ben-Cohen
>>> <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Mathieu
>>> Poirier <mathieu.poirier@linaro.org>; Vincent Whitchurch
>>> <vincent.whitchurch@axis.com>
>>> Subject: Re: [PATCH v6 0/4] Add a vhost RPMsg API
>>>
>>> Hi Arnaud,
>>>
>>> On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
>>>> Hi  Guennadi,
>>>>
>>>> On 9/1/20 5:11 PM, Guennadi Liakhovetski wrote:
>>>>> Hi,
>>>>>
>>>>> Next update:
>>>>>
>>>>> v6:
>>>>> - rename include/linux/virtio_rpmsg.h ->
>>>>> include/linux/rpmsg/virtio.h
>>>>>
>>>>> v5:
>>>>> - don't hard-code message layout
>>>>>
>>>>> v4:
>>>>> - add endianness conversions to comply with the VirtIO standard
>>>>>
>>>>> 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
>>>>
>>>> Mathieu pointed me your series. On my side i proposed the rpmsg_ns_msg
>>>> service[1] that does not match with your implementation.
>>>> As i come late, i hope that i did not miss something in the history...
>>>> Don't hesitate to point me the discussions, if it is the case.
>>>
>>> Well, as you see, this is a v6 only of this patch set, and apart from it there have
>>> been several side discussions and patch sets.
>>>
>>>> Regarding your patchset, it is quite confusing for me. It seems that
>>>> you implement your own protocol on top of vhost forked from the RPMsg
>>> one.
>>>> But look to me that it is not the RPMsg protocol.
>>>
>>> I'm implementing a counterpart to the rpmsg protocol over VirtIO as initially
>>> implemented by drivers/rpmsg/virtio_rpmsg_bus.c for the "main CPU" (in case
>>> of remoteproc over VirtIO) or the guest side in case of Linux virtualisation.
>>> Since my implementation can talk to that driver, I don't think, that I'm inventing
>>> a new protocol. I'm adding support for the same protocol for the opposite side
>>> of the VirtIO divide.
>>
>> The main point I would like to highlight here is related to the use of the name "RPMsg"
>> more than how you implement your IPC protocol.
>> If It is a counterpart, it probably does not respect interface for RPMsg clients.
>> A good way to answer this, might be to respond to this question:
>> Is the rpmsg sample client[4] can be used on top of your vhost RPMsg implementation?
>> If the response is no, describe it as a RPMsg implementation could lead to confusion...
> 
> Sorry, I don't quite understand your logic. RPMsg is a communication protocol, not an 
> API. An RPMsg implementation has to be able to communicate with other compliant RPMsg 
> implementations, it doesn't have to provide any specific API. Am I missing anything?

You are right nothing is written in stone that compliance with the user RPMsg API defined
in the Linux Documentation [5] is mandatory.
IMO, as this API is defined in the Linux documentation [5] we should respect it, to ensure
one generic implementation. The RPMsg sample client[4] uses this user API, so seems to me
a good candidate to verify this. 

That's said, shall we multiple the RPMsg implementations in Linux with several APIs,
With the risk to make the RPMsg clients devices dependent on these implementations?
That could lead to complex code or duplications...

I'm not the right person to answer, Bjorn and Mathieu are.

[5] https://elixir.bootlin.com/linux/v5.8.10/source/Documentation/rpmsg.txt#L66

Thanks,
Arnaud

  
> 
> Thanks
> Guennadi
> 
>> [4] https://elixir.bootlin.com/linux/v5.9-rc5/source/samples/rpmsg/rpmsg_client_sample.c
>>
>> Regards,
>> Arnaud
>>
>>>
>>>> So i would be agree with Vincent[2] which proposed to switch on a
>>>> RPMsg API and creating a vhost rpmsg device. This is also proposed in
>>>> the "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
>>>> Do you think that this alternative could match with your need?
>>>
>>> As I replied to Vincent, I understand his proposal and the approach taken in the
>>> series [3], but I'm not sure I agree, that adding yet another virtual device /
>>> driver layer on the vhost side is a good idea. As far as I understand adding new
>>> completely virtual devices isn't considered to be a good practice in the kernel.
>>> Currently vhost is just a passive "library"
>>> and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of
>>> converting vhost to a virtual device infrastructure.
>>>
>>> Thanks for pointing me out at [3], I should have a better look at it.
>>>
>>> Thanks
>>> Guennadi
>>>
>>>> [1].
>>>> https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338
>>>> 335 [2].
>>>> https://www.spinics.net/lists/linux-virtualization/msg44195.html
>>>> [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>
>>>>>
>>>>> Thanks
>>>>> Guennadi
>>>>>
>>>>> Guennadi Liakhovetski (4):
>>>>>   vhost: convert VHOST_VSOCK_SET_RUNNING to a generic ioctl
>>>>>   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            | 373 +++++++++++++++++++++++++++++++
>>>>>  drivers/vhost/vhost_rpmsg.h      |  74 ++++++
>>>>>  include/linux/rpmsg/virtio.h     |  83 +++++++
>>>>>  include/uapi/linux/rpmsg.h       |   3 +
>>>>>  include/uapi/linux/vhost.h       |   4 +-
>>>>>  9 files changed, 551 insertions(+), 80 deletions(-)  create mode
>>>>> 100644 drivers/vhost/rpmsg.c  create mode 100644
>>>>> drivers/vhost/vhost_rpmsg.h  create mode 100644
>>>>> include/linux/rpmsg/virtio.h
>>>>>

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Vincent Whitchurch]

On Fri, Sep 18, 2020 at 11:47:20AM +0200, Guennadi Liakhovetski wrote:
> On Fri, Sep 18, 2020 at 09:47:45AM +0200, Arnaud POULIQUEN wrote:
> > IMO, as this API is defined in the Linux documentation [5] we should respect it, to ensure
> > one generic implementation. The RPMsg sample client[4] uses this user API, so seems to me
> > a good candidate to verify this. 
> > 
> > That's said, shall we multiple the RPMsg implementations in Linux with several APIs,
> > With the risk to make the RPMsg clients devices dependent on these implementations?
> > That could lead to complex code or duplications...
> 
> So, no, in my understanding there aren't two competing alternative APIs, you'd never have 
> to choose between them. If you're writing a driver for Linux to communicate with remote 
> processors or to run on VMs, you use the existing API. If you're writing a driver for 
> Linux to communicate with those VMs, you use the vhost API and whatever help is available 
> for RPMsg processing.
> 
> However, I can in principle imagine a single driver, written to work on both sides. 
> Something like the rpmsg_char.c or maybe some networking driver. Is that what you're 
> referring to? I can see that as a fun exercise, but are there any real uses for that? 

I hinted at a real use case for this in the previous mail thread[0].
I'm exploring using rpmsg-char to allow communication between two chips,
both running Linux.  rpmsg-char can be used pretty much as-is for both
sides of the userspace-to-userspace communication and (the userspace
side of the) userspace-to-kernel communication between the two chips.

> You could do the same with VirtIO, however, it has been decided to go with two 
> distinct APIs: virtio for guests and vhost for the host, noone bothered to create a 
> single API for both and nobody seems to miss one. Why would we want one with RPMsg?

I think I answered this question in the previous mail thread as well[1]:
| 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.

[0] https://www.spinics.net/lists/linux-virtualization/msg43799.html
[1] https://www.spinics.net/lists/linux-virtualization/msg43802.html

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Guennadi Liakhovetski]

On Fri, Sep 18, 2020 at 12:39:07PM +0200, Vincent Whitchurch wrote:
> On Fri, Sep 18, 2020 at 11:47:20AM +0200, Guennadi Liakhovetski wrote:
> > On Fri, Sep 18, 2020 at 09:47:45AM +0200, Arnaud POULIQUEN wrote:
> > > IMO, as this API is defined in the Linux documentation [5] we should respect it, to ensure
> > > one generic implementation. The RPMsg sample client[4] uses this user API, so seems to me
> > > a good candidate to verify this. 
> > > 
> > > That's said, shall we multiple the RPMsg implementations in Linux with several APIs,
> > > With the risk to make the RPMsg clients devices dependent on these implementations?
> > > That could lead to complex code or duplications...
> > 
> > So, no, in my understanding there aren't two competing alternative APIs, you'd never have 
> > to choose between them. If you're writing a driver for Linux to communicate with remote 
> > processors or to run on VMs, you use the existing API. If you're writing a driver for 
> > Linux to communicate with those VMs, you use the vhost API and whatever help is available 
> > for RPMsg processing.
> > 
> > However, I can in principle imagine a single driver, written to work on both sides. 
> > Something like the rpmsg_char.c or maybe some networking driver. Is that what you're 
> > referring to? I can see that as a fun exercise, but are there any real uses for that? 
> 
> I hinted at a real use case for this in the previous mail thread[0].
> I'm exploring using rpmsg-char to allow communication between two chips,
> both running Linux.  rpmsg-char can be used pretty much as-is for both
> sides of the userspace-to-userspace communication and (the userspace
> side of the) userspace-to-kernel communication between the two chips.
> 
> > You could do the same with VirtIO, however, it has been decided to go with two 
> > distinct APIs: virtio for guests and vhost for the host, noone bothered to create a 
> > single API for both and nobody seems to miss one. Why would we want one with RPMsg?
> 
> I think I answered this question in the previous mail thread as well[1]:
> | 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.

I think RPMsg is lacking real established documentation... Quating from [2]:

<quote>
In the current protocol, at startup, the master sends notification to remote to let it 
know that it can receive name service announcement.
</quote>

Isn't that a sufficient asymnetry?

Thanks
Guennadi

[2] https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol

> 
> [0] https://www.spinics.net/lists/linux-virtualization/msg43799.html
> [1] https://www.spinics.net/lists/linux-virtualization/msg43802.html

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Arnaud POULIQUEN]

Hi Guennadi,


On 9/18/20 11:47 AM, Guennadi Liakhovetski wrote:
> Hi Arnaud,
> 
> On Fri, Sep 18, 2020 at 09:47:45AM +0200, Arnaud POULIQUEN wrote:
>> Hi Guennadi,
>>
>> On 9/18/20 7:44 AM, Guennadi Liakhovetski wrote:
>>> Hi Arnaud,
>>>
>>> On Thu, Sep 17, 2020 at 05:21:02PM +0200, Arnaud POULIQUEN wrote:
>>>> Hi Guennadi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
>>>>> Sent: jeudi 17 septembre 2020 07:47
>>>>> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
>>>>> Cc: kvm@vger.kernel.org; linux-remoteproc@vger.kernel.org;
>>>>> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa-
>>>>> project.org; Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Liam
>>>>> Girdwood <liam.r.girdwood@linux.intel.com>; Michael S. Tsirkin
>>>>> <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Ohad Ben-Cohen
>>>>> <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Mathieu
>>>>> Poirier <mathieu.poirier@linaro.org>; Vincent Whitchurch
>>>>> <vincent.whitchurch@axis.com>
>>>>> Subject: Re: [PATCH v6 0/4] Add a vhost RPMsg API
>>>>>
>>>>> Hi Arnaud,
>>>>>
>>>>> On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
>>>>>> Hi  Guennadi,
>>>>>>
>>>>>> On 9/1/20 5:11 PM, Guennadi Liakhovetski wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Next update:
>>>>>>>
>>>>>>> v6:
>>>>>>> - rename include/linux/virtio_rpmsg.h ->
>>>>>>> include/linux/rpmsg/virtio.h
>>>>>>>
>>>>>>> v5:
>>>>>>> - don't hard-code message layout
>>>>>>>
>>>>>>> v4:
>>>>>>> - add endianness conversions to comply with the VirtIO standard
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>> Mathieu pointed me your series. On my side i proposed the rpmsg_ns_msg
>>>>>> service[1] that does not match with your implementation.
>>>>>> As i come late, i hope that i did not miss something in the history...
>>>>>> Don't hesitate to point me the discussions, if it is the case.
>>>>>
>>>>> Well, as you see, this is a v6 only of this patch set, and apart from it there have
>>>>> been several side discussions and patch sets.
>>>>>
>>>>>> Regarding your patchset, it is quite confusing for me. It seems that
>>>>>> you implement your own protocol on top of vhost forked from the RPMsg
>>>>> one.
>>>>>> But look to me that it is not the RPMsg protocol.
>>>>>
>>>>> I'm implementing a counterpart to the rpmsg protocol over VirtIO as initially
>>>>> implemented by drivers/rpmsg/virtio_rpmsg_bus.c for the "main CPU" (in case
>>>>> of remoteproc over VirtIO) or the guest side in case of Linux virtualisation.
>>>>> Since my implementation can talk to that driver, I don't think, that I'm inventing
>>>>> a new protocol. I'm adding support for the same protocol for the opposite side
>>>>> of the VirtIO divide.
>>>>
>>>> The main point I would like to highlight here is related to the use of the name "RPMsg"
>>>> more than how you implement your IPC protocol.
>>>> If It is a counterpart, it probably does not respect interface for RPMsg clients.
>>>> A good way to answer this, might be to respond to this question:
>>>> Is the rpmsg sample client[4] can be used on top of your vhost RPMsg implementation?
>>>> If the response is no, describe it as a RPMsg implementation could lead to confusion...
>>>
>>> Sorry, I don't quite understand your logic. RPMsg is a communication protocol, not an 
>>> API. An RPMsg implementation has to be able to communicate with other compliant RPMsg 
>>> implementations, it doesn't have to provide any specific API. Am I missing anything?
>>
>> You are right nothing is written in stone that compliance with the user RPMsg API defined
>> in the Linux Documentation [5] is mandatory.
> 
> A quote from [5]:
> 
> <quote>
> Rpmsg is a virtio-based messaging bus that allows kernel drivers to communicate
> with remote processors available on the system.
> </quote>
> 
> So, that document describes the API used by Linux drivers to talk to remote processors. 
> It says nothing about VMs. What my patches do, they add a capability to the Linux RPMsg 
> implementation to also be used with VMs. Moreover, this is a particularly good fit, 
> because both cases can use VirtIO, so, the "VirtIO side" of the communication doesn't 
> have to change, and indeed it remains unchanged and uses the API in [5]. But what I do, 
> is I also add RPMsg support to the host side.

The feature you propose is very interesting and using RPMsg for this is clearly,
for me, a good approach.

But I'm not sure that we are speaking about the same things...
  
Perhaps, I need to clarify my view with a new approach describing RPMsg layers. 

in next part I'm focusing only on Linux local side (I'm ignoring the remote side for now).
We can divide RPMsg implementation in layers.

1) Rpmsg service layer:
  This layer implements a service on top of the RPMsg protocol.
  It uses the RPMSG user API to:
    - register/unregister a device
    - create destroy endpoints
    - send/receive messages
  This layer is independent from the ways the message is sent (virtio, vhost,...)	 
  In Linux kernel as example we can find the RPMsg sample device and rpmsg_char device 

2) The RPMsg core layer:
  This is the transport layer. It implements the RPMsg API
  It a kind of message mixer/router layer based on local and distant addresses.
  This layer is independent from the ways the message is sent ( virtio, vhost,...)	 

3) The RPMsg bus layer:
  This backend layer implements the RPMsg protocol over an IPC layer.
  This layer depends on the platform.
  Some exemples are :
    - drivers/rpmsg/mtk_rpmsg.c
    - drivers/rpmsg/qcom_glink_native.c
    - drivers/rpmsg/virtio_rpmsg_bus.c

Regarding your implementation your drivers/vhost/rpmsg.c replaces the layers 2)
and 3) and define a new "Vhost RPMsg" API, right?
As consequence the layer 1) has to by modified or duplicated to support the
"Vhost RPMsg" API.

What Vincent an I proposed (please tell me Vincent if i'm wrong) is that only the
layer 3) is implemented for portability on vhost. This as been proposed in the
"RFC patch 14/22" [6] from Kishon.

But I'm not a vhost expert, So perhaps it is not adapted...?

[6] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2219863.html 

> 
>> IMO, as this API is defined in the Linux documentation [5] we should respect it, to ensure
>> one generic implementation. The RPMsg sample client[4] uses this user API, so seems to me
>> a good candidate to verify this. 
>>
>> That's said, shall we multiple the RPMsg implementations in Linux with several APIs,
>> With the risk to make the RPMsg clients devices dependent on these implementations?
>> That could lead to complex code or duplications...
> 
> So, no, in my understanding there aren't two competing alternative APIs, you'd never have 
> to choose between them. If you're writing a driver for Linux to communicate with remote 
> processors or to run on VMs, you use the existing API. If you're writing a driver for 
> Linux to communicate with those VMs, you use the vhost API and whatever help is available 
> for RPMsg processing.

This is what I would have expect here. To have only one driver per service, not
to instantiate it for each type of type of communication.

> 
> However, I can in principle imagine a single driver, written to work on both sides. 
> Something like the rpmsg_char.c or maybe some networking driver. Is that what you're 
> referring to? I can see that as a fun exercise, but are there any real uses for that? 
> You could do the same with VirtIO, however, it has been decided to go with two 
> distinct APIs: virtio for guests and vhost for the host, noone bothered to create a 
> single API for both and nobody seems to miss one. Why would we want one with RPMsg?

Regarding the RFC [3] mentioned in a previous mail, perhaps this requirement
exists. I added Kishon in copy. 

In ST, we have such requirement but not concerning vhost.Our need is to
facilitate the services porting between an internal coprocessor (virtio) and an
external coprocessor(serial link) using RPMsg.

The Sound open firmware project could also takes benefit of an uniformization of
the communication with the audio DSP, using the RPMsg API to address in a same
way an internal coprocessor, an external coprocessor or a virtual machine for
the control part...
   
And of course to simplify the maintenance and evolution of the RPMsg protocol in
Linux.

That's said our approach seems to me also valid as it respects the RPMsg protocol.

Now there are 2 different patch series with 2 different approaches sent to
the mailing list. So i guess that maintainers will have to decide whether
they will get the both or only one.

Thanks,
Arnaud

> 
> Thanks
> Guennadi
>> [5] https://elixir.bootlin.com/linux/v5.8.10/source/Documentation/rpmsg.txt#L66
>>
>> Thanks,
>> Arnaud
>>
>>   
>>>
>>> Thanks
>>> Guennadi
>>>
>>>> [4] https://elixir.bootlin.com/linux/v5.9-rc5/source/samples/rpmsg/rpmsg_client_sample.c
>>>>
>>>> Regards,
>>>> Arnaud
>>>>
>>>>>
>>>>>> So i would be agree with Vincent[2] which proposed to switch on a
>>>>>> RPMsg API and creating a vhost rpmsg device. This is also proposed in
>>>>>> the "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
>>>>>> Do you think that this alternative could match with your need?
>>>>>
>>>>> As I replied to Vincent, I understand his proposal and the approach taken in the
>>>>> series [3], but I'm not sure I agree, that adding yet another virtual device /
>>>>> driver layer on the vhost side is a good idea. As far as I understand adding new
>>>>> completely virtual devices isn't considered to be a good practice in the kernel.
>>>>> Currently vhost is just a passive "library"
>>>>> and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of
>>>>> converting vhost to a virtual device infrastructure.
>>>>>
>>>>> Thanks for pointing me out at [3], I should have a better look at it.
>>>>>
>>>>> Thanks
>>>>> Guennadi
>>>>>
>>>>>> [1].
>>>>>> https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338
>>>>>> 335 [2].
>>>>>> https://www.spinics.net/lists/linux-virtualization/msg44195.html
>>>>>> [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html
>>>>>>
>>>>>> Thanks,
>>>>>> Arnaud
>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>> Guennadi

Re: [PATCH v6 0/4] Add a vhost RPMsg API

[Mathieu Poirier]

Hi all,

On Fri, Sep 18, 2020 at 07:26:50PM +0200, Arnaud POULIQUEN wrote:
> Hi Guennadi,
> 
> 
> On 9/18/20 11:47 AM, Guennadi Liakhovetski wrote:
> > Hi Arnaud,
> > 
> > On Fri, Sep 18, 2020 at 09:47:45AM +0200, Arnaud POULIQUEN wrote:
> >> Hi Guennadi,
> >>
> >> On 9/18/20 7:44 AM, Guennadi Liakhovetski wrote:
> >>> Hi Arnaud,
> >>>
> >>> On Thu, Sep 17, 2020 at 05:21:02PM +0200, Arnaud POULIQUEN wrote:
> >>>> Hi Guennadi,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> >>>>> Sent: jeudi 17 septembre 2020 07:47
> >>>>> To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
> >>>>> Cc: kvm@vger.kernel.org; linux-remoteproc@vger.kernel.org;
> >>>>> virtualization@lists.linux-foundation.org; sound-open-firmware@alsa-
> >>>>> project.org; Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Liam
> >>>>> Girdwood <liam.r.girdwood@linux.intel.com>; Michael S. Tsirkin
> >>>>> <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Ohad Ben-Cohen
> >>>>> <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Mathieu
> >>>>> Poirier <mathieu.poirier@linaro.org>; Vincent Whitchurch
> >>>>> <vincent.whitchurch@axis.com>
> >>>>> Subject: Re: [PATCH v6 0/4] Add a vhost RPMsg API
> >>>>>
> >>>>> Hi Arnaud,
> >>>>>
> >>>>> On Tue, Sep 15, 2020 at 02:13:23PM +0200, Arnaud POULIQUEN wrote:
> >>>>>> Hi  Guennadi,
> >>>>>>
> >>>>>> On 9/1/20 5:11 PM, Guennadi Liakhovetski wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Next update:
> >>>>>>>
> >>>>>>> v6:
> >>>>>>> - rename include/linux/virtio_rpmsg.h ->
> >>>>>>> include/linux/rpmsg/virtio.h
> >>>>>>>
> >>>>>>> v5:
> >>>>>>> - don't hard-code message layout
> >>>>>>>
> >>>>>>> v4:
> >>>>>>> - add endianness conversions to comply with the VirtIO standard
> >>>>>>>
> >>>>>>> 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
> >>>>>>
> >>>>>> Mathieu pointed me your series. On my side i proposed the rpmsg_ns_msg
> >>>>>> service[1] that does not match with your implementation.
> >>>>>> As i come late, i hope that i did not miss something in the history...
> >>>>>> Don't hesitate to point me the discussions, if it is the case.
> >>>>>
> >>>>> Well, as you see, this is a v6 only of this patch set, and apart from it there have
> >>>>> been several side discussions and patch sets.
> >>>>>
> >>>>>> Regarding your patchset, it is quite confusing for me. It seems that
> >>>>>> you implement your own protocol on top of vhost forked from the RPMsg
> >>>>> one.
> >>>>>> But look to me that it is not the RPMsg protocol.
> >>>>>
> >>>>> I'm implementing a counterpart to the rpmsg protocol over VirtIO as initially
> >>>>> implemented by drivers/rpmsg/virtio_rpmsg_bus.c for the "main CPU" (in case
> >>>>> of remoteproc over VirtIO) or the guest side in case of Linux virtualisation.
> >>>>> Since my implementation can talk to that driver, I don't think, that I'm inventing
> >>>>> a new protocol. I'm adding support for the same protocol for the opposite side
> >>>>> of the VirtIO divide.
> >>>>
> >>>> The main point I would like to highlight here is related to the use of the name "RPMsg"
> >>>> more than how you implement your IPC protocol.
> >>>> If It is a counterpart, it probably does not respect interface for RPMsg clients.
> >>>> A good way to answer this, might be to respond to this question:
> >>>> Is the rpmsg sample client[4] can be used on top of your vhost RPMsg implementation?
> >>>> If the response is no, describe it as a RPMsg implementation could lead to confusion...
> >>>
> >>> Sorry, I don't quite understand your logic. RPMsg is a communication protocol, not an 
> >>> API. An RPMsg implementation has to be able to communicate with other compliant RPMsg 
> >>> implementations, it doesn't have to provide any specific API. Am I missing anything?
> >>
> >> You are right nothing is written in stone that compliance with the user RPMsg API defined
> >> in the Linux Documentation [5] is mandatory.
> > 
> > A quote from [5]:
> > 
> > <quote>
> > Rpmsg is a virtio-based messaging bus that allows kernel drivers to communicate
> > with remote processors available on the system.
> > </quote>
> > 
> > So, that document describes the API used by Linux drivers to talk to remote processors. 
> > It says nothing about VMs. What my patches do, they add a capability to the Linux RPMsg 
> > implementation to also be used with VMs. Moreover, this is a particularly good fit, 
> > because both cases can use VirtIO, so, the "VirtIO side" of the communication doesn't 
> > have to change, and indeed it remains unchanged and uses the API in [5]. But what I do, 
> > is I also add RPMsg support to the host side.
> 
> The feature you propose is very interesting and using RPMsg for this is clearly,
> for me, a good approach.
> 
> But I'm not sure that we are speaking about the same things...
>   
> Perhaps, I need to clarify my view with a new approach describing RPMsg layers. 
> 
> in next part I'm focusing only on Linux local side (I'm ignoring the remote side for now).
> We can divide RPMsg implementation in layers.
> 
> 1) Rpmsg service layer:
>   This layer implements a service on top of the RPMsg protocol.
>   It uses the RPMSG user API to:
>     - register/unregister a device
>     - create destroy endpoints
>     - send/receive messages
>   This layer is independent from the ways the message is sent (virtio, vhost,...)	 
>   In Linux kernel as example we can find the RPMsg sample device and rpmsg_char device 
> 
> 2) The RPMsg core layer:
>   This is the transport layer. It implements the RPMsg API
>   It a kind of message mixer/router layer based on local and distant addresses.
>   This layer is independent from the ways the message is sent ( virtio, vhost,...)	 
> 
> 3) The RPMsg bus layer:
>   This backend layer implements the RPMsg protocol over an IPC layer.
>   This layer depends on the platform.
>   Some exemples are :
>     - drivers/rpmsg/mtk_rpmsg.c
>     - drivers/rpmsg/qcom_glink_native.c
>     - drivers/rpmsg/virtio_rpmsg_bus.c
> 
> Regarding your implementation your drivers/vhost/rpmsg.c replaces the layers 2)
> and 3) and define a new "Vhost RPMsg" API, right?
> As consequence the layer 1) has to by modified or duplicated to support the
> "Vhost RPMsg" API.
> 
> What Vincent an I proposed (please tell me Vincent if i'm wrong) is that only the
> layer 3) is implemented for portability on vhost. This as been proposed in the
> "RFC patch 14/22" [6] from Kishon.
> 
> But I'm not a vhost expert, So perhaps it is not adapted...?
> 
> [6] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2219863.html 
> 
> > 
> >> IMO, as this API is defined in the Linux documentation [5] we should respect it, to ensure
> >> one generic implementation. The RPMsg sample client[4] uses this user API, so seems to me
> >> a good candidate to verify this. 
> >>
> >> That's said, shall we multiple the RPMsg implementations in Linux with several APIs,
> >> With the risk to make the RPMsg clients devices dependent on these implementations?
> >> That could lead to complex code or duplications...
> > 
> > So, no, in my understanding there aren't two competing alternative APIs, you'd never have 
> > to choose between them. If you're writing a driver for Linux to communicate with remote 
> > processors or to run on VMs, you use the existing API. If you're writing a driver for 
> > Linux to communicate with those VMs, you use the vhost API and whatever help is available 
> > for RPMsg processing.
> 
> This is what I would have expect here. To have only one driver per service, not
> to instantiate it for each type of type of communication.
> 
> > 
> > However, I can in principle imagine a single driver, written to work on both sides. 
> > Something like the rpmsg_char.c or maybe some networking driver. Is that what you're 
> > referring to? I can see that as a fun exercise, but are there any real uses for that? 
> > You could do the same with VirtIO, however, it has been decided to go with two 
> > distinct APIs: virtio for guests and vhost for the host, noone bothered to create a 
> > single API for both and nobody seems to miss one. Why would we want one with RPMsg?
> 
> Regarding the RFC [3] mentioned in a previous mail, perhaps this requirement
> exists. I added Kishon in copy. 
> 
> In ST, we have such requirement but not concerning vhost.Our need is to
> facilitate the services porting between an internal coprocessor (virtio) and an
> external coprocessor(serial link) using RPMsg.
> 
> The Sound open firmware project could also takes benefit of an uniformization of
> the communication with the audio DSP, using the RPMsg API to address in a same
> way an internal coprocessor, an external coprocessor or a virtual machine for
> the control part...
>    
> And of course to simplify the maintenance and evolution of the RPMsg protocol in
> Linux.
> 
> That's said our approach seems to me also valid as it respects the RPMsg protocol.
> 
> Now there are 2 different patch series with 2 different approaches sent to
> the mailing list. So i guess that maintainers will have to decide whether
> they will get the both or only one.
> 

I finally had the time to look at Kishon's pathset yesterday.  If we skim out
the parts that deal with the realities of the NTB his solution is quite
simple.  It also provides an implementation for both side of the channel, that
is host and guest.  Lastly current implementation such as rpmsg-char and
rpmsg-sample-client.c can run on it seamlessly.  

Based on the above and the use case described by Vincent I think following
Kishon's approach is the best way to move forward. 

> Thanks,
> Arnaud
> 
> > 
> > Thanks
> > Guennadi
> >> [5] https://elixir.bootlin.com/linux/v5.8.10/source/Documentation/rpmsg.txt#L66
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>   
> >>>
> >>> Thanks
> >>> Guennadi
> >>>
> >>>> [4] https://elixir.bootlin.com/linux/v5.9-rc5/source/samples/rpmsg/rpmsg_client_sample.c
> >>>>
> >>>> Regards,
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>>> So i would be agree with Vincent[2] which proposed to switch on a
> >>>>>> RPMsg API and creating a vhost rpmsg device. This is also proposed in
> >>>>>> the "Enhance VHOST to enable SoC-to-SoC communication" RFC[3].
> >>>>>> Do you think that this alternative could match with your need?
> >>>>>
> >>>>> As I replied to Vincent, I understand his proposal and the approach taken in the
> >>>>> series [3], but I'm not sure I agree, that adding yet another virtual device /
> >>>>> driver layer on the vhost side is a good idea. As far as I understand adding new
> >>>>> completely virtual devices isn't considered to be a good practice in the kernel.
> >>>>> Currently vhost is just a passive "library"
> >>>>> and my vhost-rpmsg support keeps it that way. Not sure I'm in favour of
> >>>>> converting vhost to a virtual device infrastructure.
> >>>>>
> >>>>> Thanks for pointing me out at [3], I should have a better look at it.
> >>>>>
> >>>>> Thanks
> >>>>> Guennadi
> >>>>>
> >>>>>> [1].
> >>>>>> https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338
> >>>>>> 335 [2].
> >>>>>> https://www.spinics.net/lists/linux-virtualization/msg44195.html
> >>>>>> [3]. https://www.spinics.net/lists/linux-remoteproc/msg06634.html
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Arnaud
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>> Guennadi