[PATCH v3 0/4] Enhance RPMsg buffer management

[PATCH v3 0/4] Enhance RPMsg buffer management

[Tanmay Shah]

Current design uses fixed (512 bytes) rpmsg buffer size in both rx and
tx directions. This design is not suitable if the payload is larger than
512 bytes or the payload is very small and doesn't need that much
memory. Instead introduce new virtio feature to retrieve rpmsg tx buf
size and rx buf size from the virtio config space in the resource table.

Changes in v3:
  - new patch [1/4] that renames variables with clear names.
  - %s/rbufs/rx_bufs/
  - %s/sbufs/tx_bufs/
  - %s/last_sbuf/last_tx_buf/
  - add num_rx_buf and num_tx_buf in the documentation
  - change version field from u16 to u8
  - introduce size field in the rpmsg_virtio_config structure
  - check version field is set to any non-zero value.
  - check size field is not 0.
  - Remove field for private config, as not needed for now.
  - add documentation of rpmsg_virtio_config structure
  - Check for error when retrieving MTU size in the sample driver
  - %s/mtu/MTU/

Changes in v2:
  - Change author
  - fix commit message with better explanation
  - %s/sbuf/tx_buf
  - %s/rbuf/rx_buf
  - %s/num_rbuf/num_rx_buf/
  - %s/num_sbuf/num_tx_buf/
  - %s/sbuf_size/tx_buf_size/
  - %s/rbuf_size/rx_buf_size/
  - fix typo
  - do not use ALIGN on buf size, rely on allocator
  - make err msg more explicit, %s/vdev config:/bad vdev config/
  - fix license and add AMD copyrights in the header virtio_rpmsg.h
  - Assign bit 1 to VIRTIO_RPMSG_F_BUFSZ feature
  - use __virtio32 over __u32
  - add version field to virtio rpmsg config structure
  - Introduce new patch to print rpmsg mtu size in the sample rpmsg driver
  - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h

Original seris:
https://lore.kernel.org/all/1548949280-31794-1-git-send-email-xiaoxiang@xiaomi.com/

Following modificaitons are done to the original series in v1:
  - Separate dma allocation is not done for tx and rx buffers. Instead
    allocated chunk of memory is split between tx and rx buffers.
  - If vdev doesn't support VIRTIO_RPMSG_F_BUFSZ feature then use the 
    default size of 512 bytes for buffers
  - Change MAX_RPMSG_BUF_SIZE to DEFAULT_RPMSG_BUF_SIZE
  - move virtio_rpmsg.h from uapi to linux dir
  - RPMsg buffer size must be set to hold rpmsg header at minimum in the
    vdev config space of the firmware.
  - align total buf size to page size when allocating and deallocating
    memory

Corresponding OpenAMP project PR:
  - open-amp library: https://github.com/OpenAMP/open-amp/pull/684
  - openamp-system-reference demo: 
    https://github.com/OpenAMP/openamp-system-reference/pull/106

Tanmay Shah (4):
  rpmsg: virtio_rpmsg_bus: rename rbufs and sbufs
  rpmsg: virtio_rpmsg_bus: allow different size of tx and rx bufs
  rpmsg: virtio_rpmsg_bus: get buffer size from config space
  samples: rpmsg: add MTU size info

 drivers/rpmsg/virtio_rpmsg_bus.c    | 148 +++++++++++++++++++---------
 include/linux/rpmsg/virtio_rpmsg.h  |  34 +++++++
 samples/rpmsg/rpmsg_client_sample.c |   9 ++
 3 files changed, 144 insertions(+), 47 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h


base-commit: 85842b61f64cac93d28e129d35193e329d463fd1
-- 
2.34.1

[PATCH v3 1/4] rpmsg: virtio_rpmsg_bus: rename rbufs and sbufs

[Tanmay Shah]

rename variables with clear names.
%s/rbufs/rx_bufs/
%s/sbufs/tx_bufs/
%s/last_sbuf/last_tx_buf/

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5ae15111fb4f..773547479d15 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -35,13 +35,13 @@
  * @vdev:	the virtio device
  * @rvq:	rx virtqueue
  * @svq:	tx virtqueue
- * @rbufs:	kernel address of rx buffers
- * @sbufs:	kernel address of tx buffers
- * @num_bufs:	total number of buffers for rx and tx
+ * @rx_bufs:	kernel address of rx buffers
+ * @tx_bufs:	kernel address of tx buffers
+ * @num_bufs:   total number of buffers for rx and tx
  * @buf_size:   size of one rx or tx buffer
- * @last_sbuf:	index of last tx buffer used
+ * @last_tx_buf: index of last tx buffer used
  * @bufs_dma:	dma base addr of the buffers
- * @tx_lock:	protects svq and sbufs, to allow concurrent senders.
+ * @tx_lock:	protects svq and tx_bufs, to allow concurrent senders.
  *		sending a message might require waking up a dozing remote
  *		processor, which involves sleeping, hence the mutex.
  * @endpoints:	idr of local endpoints, allows fast retrieval
@@ -55,10 +55,10 @@
 struct virtproc_info {
 	struct virtio_device *vdev;
 	struct virtqueue *rvq, *svq;
-	void *rbufs, *sbufs;
+	void *rx_bufs, *tx_bufs;
 	unsigned int num_bufs;
 	unsigned int buf_size;
-	int last_sbuf;
+	int last_tx_buf;
 	dma_addr_t bufs_dma;
 	struct mutex tx_lock;
 	struct idr endpoints;
@@ -444,8 +444,8 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 	 * either pick the next unused tx buffer
 	 * (half of our buffers are used for sending messages)
 	 */
-	if (vrp->last_sbuf < vrp->num_bufs / 2)
-		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
+	if (vrp->last_tx_buf < vrp->num_bufs / 2)
+		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -635,7 +635,7 @@ static __poll_t virtio_rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 	 *   allocated buffers are used for transmit, hence num_bufs / 2), or,
 	 * - we ask the virtqueue if there's a buffer available
 	 */
-	if (vrp->last_sbuf < vrp->num_bufs / 2 ||
+	if (vrp->last_tx_buf < vrp->num_bufs / 2 ||
 	    !virtqueue_enable_cb(vrp->svq))
 		mask |= EPOLLOUT;
 
@@ -873,15 +873,15 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		bufs_va, &vrp->bufs_dma);
 
 	/* half of the buffers is dedicated for RX */
-	vrp->rbufs = bufs_va;
+	vrp->rx_bufs = bufs_va;
 
 	/* and half is dedicated for TX */
-	vrp->sbufs = bufs_va + total_buf_space / 2;
+	vrp->tx_bufs = bufs_va + total_buf_space / 2;
 
 	/* set up the receive buffers */
 	for (i = 0; i < vrp->num_bufs / 2; i++) {
 		struct scatterlist sg;
-		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
+		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
 
 		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
 
@@ -980,7 +980,7 @@ static void rpmsg_remove(struct virtio_device *vdev)
 	vdev->config->del_vqs(vrp->vdev);
 
 	dma_free_coherent(vdev->dev.parent, total_buf_space,
-			  vrp->rbufs, vrp->bufs_dma);
+			  vrp->rx_bufs, vrp->bufs_dma);
 
 	kfree(vrp);
 }
-- 
2.34.1

[PATCH v3 2/4] rpmsg: virtio_rpmsg_bus: allow different size of tx and rx bufs

[Tanmay Shah]

Current design allocates memory for tx and rx buffers equally. The
throughput can be increased if the user is allowed to configure number
of tx and rx buffers as required. Hence, do not split number of tx & rx
buffers into half, but decide based on respective vring size.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 50 ++++++++++++++++----------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 773547479d15..99df1ae07055 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -37,7 +37,8 @@
  * @svq:	tx virtqueue
  * @rx_bufs:	kernel address of rx buffers
  * @tx_bufs:	kernel address of tx buffers
- * @num_bufs:   total number of buffers for rx and tx
+ * @num_rx_buf: total number of rx buffers
+ * @num_tx_buf: total number of tx buffers
  * @buf_size:   size of one rx or tx buffer
  * @last_tx_buf: index of last tx buffer used
  * @bufs_dma:	dma base addr of the buffers
@@ -56,7 +57,8 @@ struct virtproc_info {
 	struct virtio_device *vdev;
 	struct virtqueue *rvq, *svq;
 	void *rx_bufs, *tx_bufs;
-	unsigned int num_bufs;
+	unsigned int num_rx_buf;
+	unsigned int num_tx_buf;
 	unsigned int buf_size;
 	int last_tx_buf;
 	dma_addr_t bufs_dma;
@@ -110,7 +112,7 @@ struct virtio_rpmsg_channel {
 /*
  * 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).
+ * by the vring, up to a maximum of 256 in each direction.
  *
  * Each buffer will have 16 bytes for the msg header and 496 bytes for
  * the payload.
@@ -125,7 +127,7 @@ struct virtio_rpmsg_channel {
  * can change this without changing anything in the firmware of the remote
  * processor.
  */
-#define MAX_RPMSG_NUM_BUFS	(512)
+#define MAX_RPMSG_NUM_BUFS	(256)
 #define MAX_RPMSG_BUF_SIZE	(512)
 
 /*
@@ -440,11 +442,8 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 
 	mutex_lock(&vrp->tx_lock);
 
-	/*
-	 * either pick the next unused tx buffer
-	 * (half of our buffers are used for sending messages)
-	 */
-	if (vrp->last_tx_buf < vrp->num_bufs / 2)
+	/* either pick the next unused tx buffer */
+	if (vrp->last_tx_buf < vrp->num_tx_buf)
 		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
 	/* or recycle a used one */
 	else
@@ -631,11 +630,10 @@ static __poll_t virtio_rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 
 	/*
 	 * check for a free buffer, either:
-	 * - we haven't used all of the available transmit buffers (half of the
-	 *   allocated buffers are used for transmit, hence num_bufs / 2), or,
+	 * - we haven't used all of the available transmit buffers or,
 	 * - we ask the virtqueue if there's a buffer available
 	 */
-	if (vrp->last_tx_buf < vrp->num_bufs / 2 ||
+	if (vrp->last_tx_buf < vrp->num_tx_buf ||
 	    !virtqueue_enable_cb(vrp->svq))
 		mask |= EPOLLOUT;
 
@@ -846,19 +844,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	vrp->rvq = vqs[0];
 	vrp->svq = vqs[1];
 
-	/* we expect symmetric tx/rx vrings */
-	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
-		virtqueue_get_vring_size(vrp->svq));
-
 	/* we need less buffers if vrings are small */
-	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
-		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
+	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS)
+		vrp->num_rx_buf = virtqueue_get_vring_size(vrp->rvq);
+	else
+		vrp->num_rx_buf = MAX_RPMSG_NUM_BUFS;
+
+	if (virtqueue_get_vring_size(vrp->svq) < MAX_RPMSG_NUM_BUFS)
+		vrp->num_tx_buf = virtqueue_get_vring_size(vrp->svq);
 	else
-		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
+		vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
 
 	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
 
-	total_buf_space = vrp->num_bufs * vrp->buf_size;
+	total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp->buf_size;
 
 	/* allocate coherent memory for the buffers */
 	bufs_va = dma_alloc_coherent(vdev->dev.parent,
@@ -872,14 +871,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
 		bufs_va, &vrp->bufs_dma);
 
-	/* half of the buffers is dedicated for RX */
+	/* first part of the buffers is dedicated for RX */
 	vrp->rx_bufs = bufs_va;
 
-	/* and half is dedicated for TX */
-	vrp->tx_bufs = bufs_va + total_buf_space / 2;
+	/* and second part is dedicated for TX */
+	vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
 
 	/* set up the receive buffers */
-	for (i = 0; i < vrp->num_bufs / 2; i++) {
+	for (i = 0; i < vrp->num_rx_buf; i++) {
 		struct scatterlist sg;
 		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
 
@@ -966,7 +965,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
 static void rpmsg_remove(struct virtio_device *vdev)
 {
 	struct virtproc_info *vrp = vdev->priv;
-	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
+	unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
+	size_t total_buf_space = num_bufs * vrp->buf_size;
 	int ret;
 
 	virtio_reset_device(vdev);
-- 
2.34.1

[PATCH v3 3/4] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Tanmay Shah]

512 bytes isn't always suitable for all case, let firmware
maker decide the best value from resource table.
enable by VIRTIO_RPMSG_F_BUFSZ feature bit.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---

Changes in v3:
  - change version field from u16 to u8
  - introduce size field in the rpmsg_virtio_config structure
  - check version field is set to any non-zero value.
  - check size field is not 0.
  - Remove field for private config, as not needed for now.
  - add documentation of rpmsg_virtio_config structure

 drivers/rpmsg/virtio_rpmsg_bus.c   | 90 ++++++++++++++++++++++++------
 include/linux/rpmsg/virtio_rpmsg.h | 34 +++++++++++
 2 files changed, 106 insertions(+), 18 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 99df1ae07055..f1ab8e792f3d 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -20,6 +20,7 @@
 #include <linux/rpmsg.h>
 #include <linux/rpmsg/byteorder.h>
 #include <linux/rpmsg/ns.h>
+#include <linux/rpmsg/virtio_rpmsg.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
@@ -39,7 +40,8 @@
  * @tx_bufs:	kernel address of tx buffers
  * @num_rx_buf: total number of rx buffers
  * @num_tx_buf: total number of tx buffers
- * @buf_size:   size of one rx or tx buffer
+ * @rx_buf_size: size of one rx buffer
+ * @tx_buf_size: size of one tx buffer
  * @last_tx_buf: index of last tx buffer used
  * @bufs_dma:	dma base addr of the buffers
  * @tx_lock:	protects svq and tx_bufs, to allow concurrent senders.
@@ -59,7 +61,8 @@ struct virtproc_info {
 	void *rx_bufs, *tx_bufs;
 	unsigned int num_rx_buf;
 	unsigned int num_tx_buf;
-	unsigned int buf_size;
+	unsigned int rx_buf_size;
+	unsigned int tx_buf_size;
 	int last_tx_buf;
 	dma_addr_t bufs_dma;
 	struct mutex tx_lock;
@@ -68,9 +71,6 @@ struct virtproc_info {
 	wait_queue_head_t sendq;
 };
 
-/* 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
@@ -128,7 +128,9 @@ struct virtio_rpmsg_channel {
  * processor.
  */
 #define MAX_RPMSG_NUM_BUFS	(256)
-#define MAX_RPMSG_BUF_SIZE	(512)
+#define DEFAULT_RPMSG_BUF_SIZE	(512)
+
+#define RPMSG_VDEV_CONFIG_VER 1
 
 /*
  * Local addresses are dynamically allocated on-demand.
@@ -444,7 +446,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 
 	/* either pick the next unused tx buffer */
 	if (vrp->last_tx_buf < vrp->num_tx_buf)
-		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
+		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -514,7 +516,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 	 * messaging), or to improve the buffer allocator, to support
 	 * variable-length buffer sizes.
 	 */
-	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
+	if (len > vrp->tx_buf_size - sizeof(struct rpmsg_hdr)) {
 		dev_err(dev, "message is too big (%d)\n", len);
 		return -EMSGSIZE;
 	}
@@ -647,7 +649,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept)
 	struct rpmsg_device *rpdev = ept->rpdev;
 	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
 
-	return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
+	return vch->vrp->tx_buf_size - sizeof(struct rpmsg_hdr);
 }
 
 static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
@@ -673,7 +675,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 	 * We currently use fixed-sized buffers, so trivially sanitize
 	 * the reported payload length.
 	 */
-	if (len > vrp->buf_size ||
+	if (len > vrp->rx_buf_size ||
 	    msg_len > (len - sizeof(struct rpmsg_hdr))) {
 		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
 		return -EINVAL;
@@ -706,7 +708,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 		dev_warn_ratelimited(dev, "msg received with no recipient\n");
 
 	/* publish the real size of the buffer */
-	rpmsg_sg_init(&sg, msg, vrp->buf_size);
+	rpmsg_sg_init(&sg, msg, vrp->rx_buf_size);
 
 	/* add the buffer back to the remote processor's virtqueue */
 	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
@@ -824,6 +826,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	int err = 0, i;
 	size_t total_buf_space;
 	bool notify;
+	u8 version;
+	u16 size;
 
 	vrp = kzalloc_obj(*vrp);
 	if (!vrp)
@@ -855,9 +859,58 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	else
 		vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
 
-	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
+	/*
+	 * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
+	 * size from virtio device config space from the resource table.
+	 * If the feature is not supported, then assign default buf size.
+	 */
+	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     version, &version);
+		if (version == 0) {
+			dev_err(&vdev->dev, "invalid version of vdev config\n");
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		/*
+		 * The size field is not used for the remoteproc virtio transport,
+		 * but kept for any future transport to use
+		 */
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     size, &size);
+		if (size == 0) {
+			dev_err(&vdev->dev, "invalid size of vdev config\n");
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		/* note: tx and rx are defined from remote view */
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     txbuf_size, &vrp->rx_buf_size);
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     rxbuf_size, &vrp->tx_buf_size);
+
+		/* The buffers must hold at least the rpmsg header */
+		if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
+		    vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
+			dev_err(&vdev->dev,
+				"bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
+				vrp->rx_buf_size, vrp->tx_buf_size);
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		dev_dbg(&vdev->dev,
+			"vdev config: version=%d, rx buf sz = 0x%x, tx buf sz = 0x%x\n",
+			version, vrp->rx_buf_size, vrp->tx_buf_size);
+	} else {
+		vrp->rx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
+		vrp->tx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
+	}
 
-	total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp->buf_size;
+	total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
+			  (vrp->num_tx_buf * vrp->tx_buf_size);
 
 	/* allocate coherent memory for the buffers */
 	bufs_va = dma_alloc_coherent(vdev->dev.parent,
@@ -875,14 +928,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	vrp->rx_bufs = bufs_va;
 
 	/* and second part is dedicated for TX */
-	vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
+	vrp->tx_bufs = bufs_va + (vrp->num_rx_buf * vrp->rx_buf_size);
 
 	/* set up the receive buffers */
 	for (i = 0; i < vrp->num_rx_buf; i++) {
 		struct scatterlist sg;
-		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
+		void *cpu_addr = vrp->rx_bufs + i * vrp->rx_buf_size;
 
-		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
+		rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
 
 		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
 					  GFP_KERNEL);
@@ -965,8 +1018,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
 static void rpmsg_remove(struct virtio_device *vdev)
 {
 	struct virtproc_info *vrp = vdev->priv;
-	unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
-	size_t total_buf_space = num_bufs * vrp->buf_size;
+	size_t total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
+				 (vrp->num_tx_buf * vrp->tx_buf_size);
 	int ret;
 
 	virtio_reset_device(vdev);
@@ -992,6 +1045,7 @@ static struct virtio_device_id id_table[] = {
 
 static unsigned int features[] = {
 	VIRTIO_RPMSG_F_NS,
+	VIRTIO_RPMSG_F_BUFSZ,
 };
 
 static struct virtio_driver virtio_ipc_driver = {
diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/virtio_rpmsg.h
new file mode 100644
index 000000000000..77a530514d86
--- /dev/null
+++ b/include/linux/rpmsg/virtio_rpmsg.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) Pinecone Inc. 2019
+ * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
+ * Copyright (C) Advanced Micro Devices, Inc.
+ */
+
+#ifndef _LINUX_VIRTIO_RPMSG_H
+#define _LINUX_VIRTIO_RPMSG_H
+
+#include <linux/types.h>
+#include <linux/virtio_types.h>
+
+/* The feature bitmap for virtio rpmsg */
+#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
+#define VIRTIO_RPMSG_F_BUFSZ	1 /* RP get buffer size from config space */
+
+/**
+ * struct virtio_rpmsg_config - config space for rpmsg virtio device
+ *
+ * @version: version of this structure. current version is 1.
+ * @size:    size of this structure. unused for the remoteproc virtio backend.
+ * @txbuf_size: Tx buf size from remote's view. For Linux this is rx buf size.
+ * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx buf size.
+ */
+struct virtio_rpmsg_config {
+	u8 version;
+	__virtio16 size;
+	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
+	__virtio32 txbuf_size;
+	__virtio32 rxbuf_size;
+} __packed;
+
+#endif /* _LINUX_VIRTIO_RPMSG_H */
-- 
2.34.1

[PATCH v3 4/4] samples: rpmsg: add MTU size info

[Tanmay Shah]

RPMsg MTU size can be variable now and no longer hardcoded to 512 bytes.
Add log to the sample driver that prints current MTU size of the rpmsg
buffer.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---

Changes in v3:
  - Check for error when retrieving MTU size
  - %s/mtu/MTU/

 samples/rpmsg/rpmsg_client_sample.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/rpmsg_client_sample.c
index ae5081662283..55afa53189af 100644
--- a/samples/rpmsg/rpmsg_client_sample.c
+++ b/samples/rpmsg/rpmsg_client_sample.c
@@ -52,6 +52,7 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
 {
 	int ret;
 	struct instance_data *idata;
+	ssize_t mtu;
 
 	dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x!\n",
 					rpdev->src, rpdev->dst);
@@ -62,6 +63,14 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(&rpdev->dev, idata);
 
+	mtu = rpmsg_get_mtu(rpdev->ept);
+	if (mtu < 0) {
+		dev_warn(&rpdev->dev, "invalid rpmsg MTU size = %ld\n", mtu);
+		return mtu;
+	}
+
+	dev_info(&rpdev->dev, "rpmsg MTU size = %ld\n", mtu);
+
 	/* send a message to our remote processor */
 	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
 	if (ret) {
-- 
2.34.1

Re: [PATCH v3 3/4] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

Hi Tanmay,

On 5/29/26 18:43, Tanmay Shah wrote:
> 512 bytes isn't always suitable for all case, let firmware
> maker decide the best value from resource table.
> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> 
> Changes in v3:
>    - change version field from u16 to u8
>    - introduce size field in the rpmsg_virtio_config structure
>    - check version field is set to any non-zero value.
>    - check size field is not 0.
>    - Remove field for private config, as not needed for now.
>    - add documentation of rpmsg_virtio_config structure
> 
>   drivers/rpmsg/virtio_rpmsg_bus.c   | 90 ++++++++++++++++++++++++------
>   include/linux/rpmsg/virtio_rpmsg.h | 34 +++++++++++
>   2 files changed, 106 insertions(+), 18 deletions(-)
>   create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 99df1ae07055..f1ab8e792f3d 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -20,6 +20,7 @@
>   #include <linux/rpmsg.h>
>   #include <linux/rpmsg/byteorder.h>
>   #include <linux/rpmsg/ns.h>
> +#include <linux/rpmsg/virtio_rpmsg.h>
>   #include <linux/scatterlist.h>
>   #include <linux/slab.h>
>   #include <linux/sched.h>
> @@ -39,7 +40,8 @@
>    * @tx_bufs:	kernel address of tx buffers
>    * @num_rx_buf: total number of rx buffers
>    * @num_tx_buf: total number of tx buffers
> - * @buf_size:   size of one rx or tx buffer
> + * @rx_buf_size: size of one rx buffer
> + * @tx_buf_size: size of one tx buffer
>    * @last_tx_buf: index of last tx buffer used
>    * @bufs_dma:	dma base addr of the buffers
>    * @tx_lock:	protects svq and tx_bufs, to allow concurrent senders.
> @@ -59,7 +61,8 @@ struct virtproc_info {
>   	void *rx_bufs, *tx_bufs;
>   	unsigned int num_rx_buf;
>   	unsigned int num_tx_buf;
> -	unsigned int buf_size;
> +	unsigned int rx_buf_size;
> +	unsigned int tx_buf_size;
>   	int last_tx_buf;
>   	dma_addr_t bufs_dma;
>   	struct mutex tx_lock;
> @@ -68,9 +71,6 @@ struct virtproc_info {
>   	wait_queue_head_t sendq;
>   };
>   
> -/* 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
> @@ -128,7 +128,9 @@ struct virtio_rpmsg_channel {
>    * processor.
>    */
>   #define MAX_RPMSG_NUM_BUFS	(256)
> -#define MAX_RPMSG_BUF_SIZE	(512)
> +#define DEFAULT_RPMSG_BUF_SIZE	(512)
> +
> +#define RPMSG_VDEV_CONFIG_VER 1

I would rename it

#define RPMSG_VDEV_CONFIG_V1 1

>   
>   /*
>    * Local addresses are dynamically allocated on-demand.
> @@ -444,7 +446,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>   
>   	/* either pick the next unused tx buffer */
>   	if (vrp->last_tx_buf < vrp->num_tx_buf)
> -		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
> +		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>   	/* or recycle a used one */
>   	else
>   		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -514,7 +516,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>   	 * messaging), or to improve the buffer allocator, to support
>   	 * variable-length buffer sizes.
>   	 */
> -	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> +	if (len > vrp->tx_buf_size - sizeof(struct rpmsg_hdr)) {
>   		dev_err(dev, "message is too big (%d)\n", len);
>   		return -EMSGSIZE;
>   	}
> @@ -647,7 +649,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept)
>   	struct rpmsg_device *rpdev = ept->rpdev;
>   	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>   
> -	return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
> +	return vch->vrp->tx_buf_size - sizeof(struct rpmsg_hdr);
>   }
>   
>   static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> @@ -673,7 +675,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>   	 * We currently use fixed-sized buffers, so trivially sanitize
>   	 * the reported payload length.
>   	 */
> -	if (len > vrp->buf_size ||
> +	if (len > vrp->rx_buf_size ||
>   	    msg_len > (len - sizeof(struct rpmsg_hdr))) {
>   		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
>   		return -EINVAL;
> @@ -706,7 +708,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>   		dev_warn_ratelimited(dev, "msg received with no recipient\n");
>   
>   	/* publish the real size of the buffer */
> -	rpmsg_sg_init(&sg, msg, vrp->buf_size);
> +	rpmsg_sg_init(&sg, msg, vrp->rx_buf_size);
>   
>   	/* add the buffer back to the remote processor's virtqueue */
>   	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -824,6 +826,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
>   	int err = 0, i;
>   	size_t total_buf_space;
>   	bool notify;
> +	u8 version;
> +	u16 size;
>   
>   	vrp = kzalloc_obj(*vrp);
>   	if (!vrp)
> @@ -855,9 +859,58 @@ static int rpmsg_probe(struct virtio_device *vdev)
>   	else
>   		vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>   
> -	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +	/*
> +	 * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
> +	 * size from virtio device config space from the resource table.
> +	 * If the feature is not supported, then assign default buf size.
> +	 */

Seems to me that It would be nice to document the config space in rpmsg.rst

> +	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     version, &version);
> +		if (version == 0) {

		if (version != RPMSG_VDEV_CONFIG_V1) {

> +			dev_err(&vdev->dev, "invalid version of vdev config\n");
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		/*
> +		 * The size field is not used for the remoteproc virtio transport,
> +		 * but kept for any future transport to use
> +		 */

I suggest to not mention the virtio transport, indeed we should decouple 
the virtio device from the virtio transport.

> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     size, &size);
> +		if (size == 0) {

	if (size != sizeof(virtio_rpmsg_config)) {

> +			dev_err(&vdev->dev, "invalid size of vdev config\n");
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		/* note: tx and rx are defined from remote view */
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     txbuf_size, &vrp->rx_buf_size);
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     rxbuf_size, &vrp->tx_buf_size);

I wonder if we should not impose a size aligned on 64-bits

> +
> +		/* The buffers must hold at least the rpmsg header */
> +		if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
> +		    vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
> +			dev_err(&vdev->dev,
> +				"bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
> +				vrp->rx_buf_size, vrp->tx_buf_size);
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		dev_dbg(&vdev->dev,
> +			"vdev config: version=%d, rx buf sz = 0x%x, tx buf sz = 0x%x\n",
> +			version, vrp->rx_buf_size, vrp->tx_buf_size);
> +	} else {
> +		vrp->rx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
> +		vrp->tx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
> +	}
>   
> -	total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp->buf_size;
> +	total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
> +			  (vrp->num_tx_buf * vrp->tx_buf_size);
>   
>   	/* allocate coherent memory for the buffers */
>   	bufs_va = dma_alloc_coherent(vdev->dev.parent,
> @@ -875,14 +928,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>   	vrp->rx_bufs = bufs_va;
>   
>   	/* and second part is dedicated for TX */
> -	vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
> +	vrp->tx_bufs = bufs_va + (vrp->num_rx_buf * vrp->rx_buf_size);


We should have a cache or 64-bit alignement here. or perhaps such 
constraints should be specified in the config space?

>   
>   	/* set up the receive buffers */
>   	for (i = 0; i < vrp->num_rx_buf; i++) {
>   		struct scatterlist sg;
> -		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
> +		void *cpu_addr = vrp->rx_bufs + i * vrp->rx_buf_size;
>   
> -		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> +		rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
>   
>   		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>   					  GFP_KERNEL);
> @@ -965,8 +1018,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
>   static void rpmsg_remove(struct virtio_device *vdev)
>   {
>   	struct virtproc_info *vrp = vdev->priv;
> -	unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
> -	size_t total_buf_space = num_bufs * vrp->buf_size;
> +	size_t total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
> +				 (vrp->num_tx_buf * vrp->tx_buf_size);
>   	int ret;
>   
>   	virtio_reset_device(vdev);
> @@ -992,6 +1045,7 @@ static struct virtio_device_id id_table[] = {
>   
>   static unsigned int features[] = {
>   	VIRTIO_RPMSG_F_NS,
> +	VIRTIO_RPMSG_F_BUFSZ,
>   };
>   
>   static struct virtio_driver virtio_ipc_driver = {
> diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/virtio_rpmsg.h
> new file mode 100644
> index 000000000000..77a530514d86
> --- /dev/null
> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) Pinecone Inc. 2019
> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> + * Copyright (C) Advanced Micro Devices, Inc.

No year specified for AMD copyright?

> + */
> +
> +#ifndef _LINUX_VIRTIO_RPMSG_H
> +#define _LINUX_VIRTIO_RPMSG_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_types.h>
> +
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> +#define VIRTIO_RPMSG_F_BUFSZ	1 /* RP get buffer size from config space */
> +
> +/**
> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
> + *
> + * @version: version of this structure. current version is 1.
> + * @size:    size of this structure. unused for the remoteproc virtio backend.

reference to remoteproc to remove

> + * @txbuf_size: Tx buf size from remote's view. For Linux this is rx buf size.
> + * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx buf size.
> + */
> +struct virtio_rpmsg_config {
> +	u8 version;
> +	__virtio16 size;
> +	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> +	__virtio32 txbuf_size;
> +	__virtio32 rxbuf_size;
> +} __packed;


As mentioned above
- The size should be be a multiple of four to ensure 64-bit alignment.
- I would add an alignment field to align the address of the TX buffers 
to the cache line.

Thanks,
Arnaud


> +
> +#endif /* _LINUX_VIRTIO_RPMSG_H */

Re: [PATCH v3 4/4] samples: rpmsg: add MTU size info

[Arnaud POULIQUEN]

On 5/29/26 18:43, Tanmay Shah wrote:
> RPMsg MTU size can be variable now and no longer hardcoded to 512 bytes.
> Add log to the sample driver that prints current MTU size of the rpmsg
> buffer.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> 
> Changes in v3:
>    - Check for error when retrieving MTU size
>    - %s/mtu/MTU/
> 
>   samples/rpmsg/rpmsg_client_sample.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/rpmsg_client_sample.c
> index ae5081662283..55afa53189af 100644
> --- a/samples/rpmsg/rpmsg_client_sample.c
> +++ b/samples/rpmsg/rpmsg_client_sample.c
> @@ -52,6 +52,7 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
>   {
>   	int ret;
>   	struct instance_data *idata;
> +	ssize_t mtu;
>   
>   	dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x!\n",
>   					rpdev->src, rpdev->dst);
> @@ -62,6 +63,14 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
>   
>   	dev_set_drvdata(&rpdev->dev, idata);
>   
> +	mtu = rpmsg_get_mtu(rpdev->ept);
> +	if (mtu < 0) {
> +		dev_warn(&rpdev->dev, "invalid rpmsg MTU size = %ld\n", mtu);
> +		return mtu;
> +	}
> +
> +	dev_info(&rpdev->dev, "rpmsg MTU size = %ld\n", mtu);
> +

Do you really need this commit? rpmsg_send should return an error if the 
buffer size is insufficient [1].

[1] 
https://elixir.bootlin.com/linux/v7.0.10/source/drivers/rpmsg/virtio_rpmsg_bus.c#L517

Regards,
Arnaud

>   	/* send a message to our remote processor */
>   	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
>   	if (ret) {

Re: [PATCH v3 3/4] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

Thank You for the reviews, please find my comments below:

On 6/2/2026 3:25 AM, Arnaud POULIQUEN wrote:
> Hi Tanmay,
> 
> On 5/29/26 18:43, Tanmay Shah wrote:
>> 512 bytes isn't always suitable for all case, let firmware
>> maker decide the best value from resource table.
>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> Changes in v3:
>>    - change version field from u16 to u8
>>    - introduce size field in the rpmsg_virtio_config structure
>>    - check version field is set to any non-zero value.
>>    - check size field is not 0.
>>    - Remove field for private config, as not needed for now.
>>    - add documentation of rpmsg_virtio_config structure
>>
>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 90 ++++++++++++++++++++++++------
>>   include/linux/rpmsg/virtio_rpmsg.h | 34 +++++++++++
>>   2 files changed, 106 insertions(+), 18 deletions(-)
>>   create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>> virtio_rpmsg_bus.c
>> index 99df1ae07055..f1ab8e792f3d 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/rpmsg.h>
>>   #include <linux/rpmsg/byteorder.h>
>>   #include <linux/rpmsg/ns.h>
>> +#include <linux/rpmsg/virtio_rpmsg.h>
>>   #include <linux/scatterlist.h>
>>   #include <linux/slab.h>
>>   #include <linux/sched.h>
>> @@ -39,7 +40,8 @@
>>    * @tx_bufs:    kernel address of tx buffers
>>    * @num_rx_buf: total number of rx buffers
>>    * @num_tx_buf: total number of tx buffers
>> - * @buf_size:   size of one rx or tx buffer
>> + * @rx_buf_size: size of one rx buffer
>> + * @tx_buf_size: size of one tx buffer
>>    * @last_tx_buf: index of last tx buffer used
>>    * @bufs_dma:    dma base addr of the buffers
>>    * @tx_lock:    protects svq and tx_bufs, to allow concurrent senders.
>> @@ -59,7 +61,8 @@ struct virtproc_info {
>>       void *rx_bufs, *tx_bufs;
>>       unsigned int num_rx_buf;
>>       unsigned int num_tx_buf;
>> -    unsigned int buf_size;
>> +    unsigned int rx_buf_size;
>> +    unsigned int tx_buf_size;
>>       int last_tx_buf;
>>       dma_addr_t bufs_dma;
>>       struct mutex tx_lock;
>> @@ -68,9 +71,6 @@ struct virtproc_info {
>>       wait_queue_head_t sendq;
>>   };
>>   -/* 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
>> @@ -128,7 +128,9 @@ struct virtio_rpmsg_channel {
>>    * processor.
>>    */
>>   #define MAX_RPMSG_NUM_BUFS    (256)
>> -#define MAX_RPMSG_BUF_SIZE    (512)
>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>> +
>> +#define RPMSG_VDEV_CONFIG_VER 1
> 
> I would rename it
> 
> #define RPMSG_VDEV_CONFIG_V1 1

We might not need this define at all. I should have removed it in this
patch. Please see below [1].

> 
>>     /*
>>    * Local addresses are dynamically allocated on-demand.
>> @@ -444,7 +446,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>         /* either pick the next unused tx buffer */
>>       if (vrp->last_tx_buf < vrp->num_tx_buf)
>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>       /* or recycle a used one */
>>       else
>>           ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -514,7 +516,7 @@ static int rpmsg_send_offchannel_raw(struct
>> rpmsg_device *rpdev,
>>        * messaging), or to improve the buffer allocator, to support
>>        * variable-length buffer sizes.
>>        */
>> -    if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>> +    if (len > vrp->tx_buf_size - sizeof(struct rpmsg_hdr)) {
>>           dev_err(dev, "message is too big (%d)\n", len);
>>           return -EMSGSIZE;
>>       }
>> @@ -647,7 +649,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct
>> rpmsg_endpoint *ept)
>>       struct rpmsg_device *rpdev = ept->rpdev;
>>       struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>>   -    return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
>> +    return vch->vrp->tx_buf_size - sizeof(struct rpmsg_hdr);
>>   }
>>     static int rpmsg_recv_single(struct virtproc_info *vrp, struct
>> device *dev,
>> @@ -673,7 +675,7 @@ static int rpmsg_recv_single(struct virtproc_info
>> *vrp, struct device *dev,
>>        * We currently use fixed-sized buffers, so trivially sanitize
>>        * the reported payload length.
>>        */
>> -    if (len > vrp->buf_size ||
>> +    if (len > vrp->rx_buf_size ||
>>           msg_len > (len - sizeof(struct rpmsg_hdr))) {
>>           dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
>>           return -EINVAL;
>> @@ -706,7 +708,7 @@ static int rpmsg_recv_single(struct virtproc_info
>> *vrp, struct device *dev,
>>           dev_warn_ratelimited(dev, "msg received with no recipient\n");
>>         /* publish the real size of the buffer */
>> -    rpmsg_sg_init(&sg, msg, vrp->buf_size);
>> +    rpmsg_sg_init(&sg, msg, vrp->rx_buf_size);
>>         /* add the buffer back to the remote processor's virtqueue */
>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>> @@ -824,6 +826,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>       int err = 0, i;
>>       size_t total_buf_space;
>>       bool notify;
>> +    u8 version;
>> +    u16 size;
>>         vrp = kzalloc_obj(*vrp);
>>       if (!vrp)
>> @@ -855,9 +859,58 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>       else
>>           vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>   -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>> +    /*
>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>> +     * size from virtio device config space from the resource table.
>> +     * If the feature is not supported, then assign default buf size.
>> +     */
> 
> Seems to me that It would be nice to document the config space in rpmsg.rst
> 

Ack.

>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 version, &version);
>> +        if (version == 0) {
> 
>         if (version != RPMSG_VDEV_CONFIG_V1) {
> 

[1] Here we have to allow any non-zero version to be valid, and make
sure any future version is always backward compatible.

For example, if we need v2 of the structure, then that should be
compatible with v1. So, old kernel keeps working with the new firmware
with limited functionality supported by the kernel. And new kernel keep
working with the old firmware, with the limited functionality supported
by the firmware.

That is just my view. I am open to more ideas, thank you.

>> +            dev_err(&vdev->dev, "invalid version of vdev config\n");
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        /*
>> +         * The size field is not used for the remoteproc virtio
>> transport,
>> +         * but kept for any future transport to use
>> +         */
> 
> I suggest to not mention the virtio transport, indeed we should decouple
> the virtio device from the virtio transport.
> 

Ack.

>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 size, &size);
>> +        if (size == 0) {
> 
>     if (size != sizeof(virtio_rpmsg_config)) {
> 

So, I think sizeof(virtio_rpmsg_config) on the remote side may not be
the same as in the linux kernel. Remote side can have its private
variables which might not needed on the linux side:

For example, the open-amp library defines the structure differently than
linux:
https://github.com/OpenAMP/open-amp/blob/23d4c5d7a5c5dd08b74d4ba828243988592337cb/lib/include/openamp/rpmsg_virtio.h#L70

There is 'split_shpool' extra variable which is not needed by the linux.

That is why restriction on the size isn't needed IMHO.

>> +            dev_err(&vdev->dev, "invalid size of vdev config\n");
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        /* note: tx and rx are defined from remote view */
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 txbuf_size, &vrp->rx_buf_size);
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 rxbuf_size, &vrp->tx_buf_size);
> 
> I wonder if we should not impose a size aligned on 64-bits
> 

I think you mean size should be aligned to 64-bits.
Ack for that.

>> +
>> +        /* The buffers must hold at least the rpmsg header */
>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>> +            dev_err(&vdev->dev,
>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        dev_dbg(&vdev->dev,
>> +            "vdev config: version=%d, rx buf sz = 0x%x, tx buf sz =
>> 0x%x\n",
>> +            version, vrp->rx_buf_size, vrp->tx_buf_size);
>> +    } else {
>> +        vrp->rx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
>> +        vrp->tx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
>> +    }
>>   -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>> >buf_size;
>> +    total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
>> +              (vrp->num_tx_buf * vrp->tx_buf_size);
>>         /* allocate coherent memory for the buffers */
>>       bufs_va = dma_alloc_coherent(vdev->dev.parent,
>> @@ -875,14 +928,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>       vrp->rx_bufs = bufs_va;
>>         /* and second part is dedicated for TX */
>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>> +    vrp->tx_bufs = bufs_va + (vrp->num_rx_buf * vrp->rx_buf_size);
> 
> 
> We should have a cache or 64-bit alignement here. or perhaps such
> constraints should be specified in the config space?
> 

I prefer to specify in the config space.

>>         /* set up the receive buffers */
>>       for (i = 0; i < vrp->num_rx_buf; i++) {
>>           struct scatterlist sg;
>> -        void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
>> +        void *cpu_addr = vrp->rx_bufs + i * vrp->rx_buf_size;
>>   -        rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>> +        rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
>>             err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>                         GFP_KERNEL);
>> @@ -965,8 +1018,8 @@ static int rpmsg_remove_device(struct device
>> *dev, void *data)
>>   static void rpmsg_remove(struct virtio_device *vdev)
>>   {
>>       struct virtproc_info *vrp = vdev->priv;
>> -    unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
>> -    size_t total_buf_space = num_bufs * vrp->buf_size;
>> +    size_t total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
>> +                 (vrp->num_tx_buf * vrp->tx_buf_size);
>>       int ret;
>>         virtio_reset_device(vdev);
>> @@ -992,6 +1045,7 @@ static struct virtio_device_id id_table[] = {
>>     static unsigned int features[] = {
>>       VIRTIO_RPMSG_F_NS,
>> +    VIRTIO_RPMSG_F_BUFSZ,
>>   };
>>     static struct virtio_driver virtio_ipc_driver = {
>> diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/
>> virtio_rpmsg.h
>> new file mode 100644
>> index 000000000000..77a530514d86
>> --- /dev/null
>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) Pinecone Inc. 2019
>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>> + * Copyright (C) Advanced Micro Devices, Inc.
> 
> No year specified for AMD copyright?

Ack, will fix.

> 
>> + */
>> +
>> +#ifndef _LINUX_VIRTIO_RPMSG_H
>> +#define _LINUX_VIRTIO_RPMSG_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/virtio_types.h>
>> +
>> +/* The feature bitmap for virtio rpmsg */
>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service
>> notifications */
>> +#define VIRTIO_RPMSG_F_BUFSZ    1 /* RP get buffer size from config
>> space */
>> +
>> +/**
>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>> + *
>> + * @version: version of this structure. current version is 1.
>> + * @size:    size of this structure. unused for the remoteproc virtio
>> backend.
> 
> reference to remoteproc to remove
> 

Ack.

>> + * @txbuf_size: Tx buf size from remote's view. For Linux this is rx
>> buf size.
>> + * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx
>> buf size.
>> + */
>> +struct virtio_rpmsg_config {
>> +    u8 version;
>> +    __virtio16 size;
>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>> +    __virtio32 txbuf_size;
>> +    __virtio32 rxbuf_size;
>> +} __packed;
> 
> 
> As mentioned above
> - The size should be be a multiple of four to ensure 64-bit alignment.
> - I would add an alignment field to align the address of the TX buffers
> to the cache line.
> 

Ok, I will change and test.

> Thanks,
> Arnaud
> 
> 
>> +
>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> 

Re: [PATCH v3 4/4] samples: rpmsg: add MTU size info

[Shah, Tanmay]

On 6/2/2026 3:34 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 5/29/26 18:43, Tanmay Shah wrote:
>> RPMsg MTU size can be variable now and no longer hardcoded to 512 bytes.
>> Add log to the sample driver that prints current MTU size of the rpmsg
>> buffer.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> Changes in v3:
>>    - Check for error when retrieving MTU size
>>    - %s/mtu/MTU/
>>
>>   samples/rpmsg/rpmsg_client_sample.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/
>> rpmsg_client_sample.c
>> index ae5081662283..55afa53189af 100644
>> --- a/samples/rpmsg/rpmsg_client_sample.c
>> +++ b/samples/rpmsg/rpmsg_client_sample.c
>> @@ -52,6 +52,7 @@ static int rpmsg_sample_probe(struct rpmsg_device
>> *rpdev)
>>   {
>>       int ret;
>>       struct instance_data *idata;
>> +    ssize_t mtu;
>>         dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x!\n",
>>                       rpdev->src, rpdev->dst);
>> @@ -62,6 +63,14 @@ static int rpmsg_sample_probe(struct rpmsg_device
>> *rpdev)
>>         dev_set_drvdata(&rpdev->dev, idata);
>>   +    mtu = rpmsg_get_mtu(rpdev->ept);
>> +    if (mtu < 0) {
>> +        dev_warn(&rpdev->dev, "invalid rpmsg MTU size = %ld\n", mtu);
>> +        return mtu;
>> +    }
>> +
>> +    dev_info(&rpdev->dev, "rpmsg MTU size = %ld\n", mtu);
>> +
> 
> Do you really need this commit? rpmsg_send should return an error if the
> buffer size is insufficient [1].
> 
> [1] https://elixir.bootlin.com/linux/v7.0.10/source/drivers/rpmsg/
> virtio_rpmsg_bus.c#L517
> 

Here, I just want to demonstrate rpmsg_get_mtu() API. Now we can have a
configurable size of the RPMsg buffer. So rpmsg_get_mtu() API can be
used to check correct buffer length before even using rpmsg_send() API.

I think I should check msg length against mtu size as well.


> Regards,
> Arnaud
> 
>>       /* send a message to our remote processor */
>>       ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
>>       if (ret) {
> 

Re: [PATCH v3 3/4] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

Hi Tanmay,

On 6/4/26 22:31, Shah, Tanmay wrote:
> Thank You for the reviews, please find my comments below:
> 
> On 6/2/2026 3:25 AM, Arnaud POULIQUEN wrote:
>> Hi Tanmay,
>>
>> On 5/29/26 18:43, Tanmay Shah wrote:
>>> 512 bytes isn't always suitable for all case, let firmware
>>> maker decide the best value from resource table.
>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>>
>>> Changes in v3:
>>>     - change version field from u16 to u8
>>>     - introduce size field in the rpmsg_virtio_config structure
>>>     - check version field is set to any non-zero value.
>>>     - check size field is not 0.
>>>     - Remove field for private config, as not needed for now.
>>>     - add documentation of rpmsg_virtio_config structure
>>>
>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 90 ++++++++++++++++++++++++------
>>>    include/linux/rpmsg/virtio_rpmsg.h | 34 +++++++++++
>>>    2 files changed, 106 insertions(+), 18 deletions(-)
>>>    create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>> virtio_rpmsg_bus.c
>>> index 99df1ae07055..f1ab8e792f3d 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -20,6 +20,7 @@
>>>    #include <linux/rpmsg.h>
>>>    #include <linux/rpmsg/byteorder.h>
>>>    #include <linux/rpmsg/ns.h>
>>> +#include <linux/rpmsg/virtio_rpmsg.h>
>>>    #include <linux/scatterlist.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/sched.h>
>>> @@ -39,7 +40,8 @@
>>>     * @tx_bufs:    kernel address of tx buffers
>>>     * @num_rx_buf: total number of rx buffers
>>>     * @num_tx_buf: total number of tx buffers
>>> - * @buf_size:   size of one rx or tx buffer
>>> + * @rx_buf_size: size of one rx buffer
>>> + * @tx_buf_size: size of one tx buffer
>>>     * @last_tx_buf: index of last tx buffer used
>>>     * @bufs_dma:    dma base addr of the buffers
>>>     * @tx_lock:    protects svq and tx_bufs, to allow concurrent senders.
>>> @@ -59,7 +61,8 @@ struct virtproc_info {
>>>        void *rx_bufs, *tx_bufs;
>>>        unsigned int num_rx_buf;
>>>        unsigned int num_tx_buf;
>>> -    unsigned int buf_size;
>>> +    unsigned int rx_buf_size;
>>> +    unsigned int tx_buf_size;
>>>        int last_tx_buf;
>>>        dma_addr_t bufs_dma;
>>>        struct mutex tx_lock;
>>> @@ -68,9 +71,6 @@ struct virtproc_info {
>>>        wait_queue_head_t sendq;
>>>    };
>>>    -/* 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
>>> @@ -128,7 +128,9 @@ struct virtio_rpmsg_channel {
>>>     * processor.
>>>     */
>>>    #define MAX_RPMSG_NUM_BUFS    (256)
>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>> +
>>> +#define RPMSG_VDEV_CONFIG_VER 1
>>
>> I would rename it
>>
>> #define RPMSG_VDEV_CONFIG_V1 1
> 
> We might not need this define at all. I should have removed it in this
> patch. Please see below [1].
> 
>>
>>>      /*
>>>     * Local addresses are dynamically allocated on-demand.
>>> @@ -444,7 +446,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>          /* either pick the next unused tx buffer */
>>>        if (vrp->last_tx_buf < vrp->num_tx_buf)
>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>>        /* or recycle a used one */
>>>        else
>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>> @@ -514,7 +516,7 @@ static int rpmsg_send_offchannel_raw(struct
>>> rpmsg_device *rpdev,
>>>         * messaging), or to improve the buffer allocator, to support
>>>         * variable-length buffer sizes.
>>>         */
>>> -    if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>>> +    if (len > vrp->tx_buf_size - sizeof(struct rpmsg_hdr)) {
>>>            dev_err(dev, "message is too big (%d)\n", len);
>>>            return -EMSGSIZE;
>>>        }
>>> @@ -647,7 +649,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct
>>> rpmsg_endpoint *ept)
>>>        struct rpmsg_device *rpdev = ept->rpdev;
>>>        struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>>>    -    return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
>>> +    return vch->vrp->tx_buf_size - sizeof(struct rpmsg_hdr);
>>>    }
>>>      static int rpmsg_recv_single(struct virtproc_info *vrp, struct
>>> device *dev,
>>> @@ -673,7 +675,7 @@ static int rpmsg_recv_single(struct virtproc_info
>>> *vrp, struct device *dev,
>>>         * We currently use fixed-sized buffers, so trivially sanitize
>>>         * the reported payload length.
>>>         */
>>> -    if (len > vrp->buf_size ||
>>> +    if (len > vrp->rx_buf_size ||
>>>            msg_len > (len - sizeof(struct rpmsg_hdr))) {
>>>            dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
>>>            return -EINVAL;
>>> @@ -706,7 +708,7 @@ static int rpmsg_recv_single(struct virtproc_info
>>> *vrp, struct device *dev,
>>>            dev_warn_ratelimited(dev, "msg received with no recipient\n");
>>>          /* publish the real size of the buffer */
>>> -    rpmsg_sg_init(&sg, msg, vrp->buf_size);
>>> +    rpmsg_sg_init(&sg, msg, vrp->rx_buf_size);
>>>          /* add the buffer back to the remote processor's virtqueue */
>>>        err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>> @@ -824,6 +826,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>        int err = 0, i;
>>>        size_t total_buf_space;
>>>        bool notify;
>>> +    u8 version;
>>> +    u16 size;
>>>          vrp = kzalloc_obj(*vrp);
>>>        if (!vrp)
>>> @@ -855,9 +859,58 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>        else
>>>            vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>>    -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>> +    /*
>>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>>> +     * size from virtio device config space from the resource table.
>>> +     * If the feature is not supported, then assign default buf size.
>>> +     */
>>
>> Seems to me that It would be nice to document the config space in rpmsg.rst
>>
> 
> Ack.
> 
>>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 version, &version);
>>> +        if (version == 0) {
>>
>>          if (version != RPMSG_VDEV_CONFIG_V1) {
>>
> 
> [1] Here we have to allow any non-zero version to be valid, and make
> sure any future version is always backward compatible.
> 
> For example, if we need v2 of the structure, then that should be
> compatible with v1. So, old kernel keeps working with the new firmware
> with limited functionality supported by the kernel. And new kernel keep
> working with the old firmware, with the limited functionality supported
> by the firmware.
> 
> That is just my view. I am open to more ideas, thank you.

My concern is that you allow any version here, while we define only a V1.
If we need a V2 we will probably have to modify this part of the code.

> 
>>> +            dev_err(&vdev->dev, "invalid version of vdev config\n");
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        /*
>>> +         * The size field is not used for the remoteproc virtio
>>> transport,
>>> +         * but kept for any future transport to use
>>> +         */
>>
>> I suggest to not mention the virtio transport, indeed we should decouple
>> the virtio device from the virtio transport.
>>
> 
> Ack.
> 
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 size, &size);
>>> +        if (size == 0) {
>>
>>      if (size != sizeof(virtio_rpmsg_config)) {
>>
> 
> So, I think sizeof(virtio_rpmsg_config) on the remote side may not be
> the same as in the linux kernel. Remote side can have its private
> variables which might not needed on the linux side:
> 
> For example, the open-amp library defines the structure differently than
> linux:
> https://github.com/OpenAMP/open-amp/blob/23d4c5d7a5c5dd08b74d4ba828243988592337cb/lib/include/openamp/rpmsg_virtio.h#L70
> 
> There is 'split_shpool' extra variable which is not needed by the linux.

The 'split_shpool' setting is currently an internal OpenAMP configuration
and is not part of a configuration space. If there is a need to split
memory regions for RX and TX buffers, the implementation should use another
method, perhaps with DA and PA addresses that specify the RX and TX buffer
locations, as for the vrings in the resource table.
I would suggest to not address this in version 1.

 From my perspective, the configuration space is a common structure that
the device and the driver share. In that case, checking the size makes
sense.
Thanks,
Arnaud

> 
> That is why restriction on the size isn't needed IMHO.
> 
>>> +            dev_err(&vdev->dev, "invalid size of vdev config\n");
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        /* note: tx and rx are defined from remote view */
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 txbuf_size, &vrp->rx_buf_size);
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 rxbuf_size, &vrp->tx_buf_size);
>>
>> I wonder if we should not impose a size aligned on 64-bits
>>
> 
> I think you mean size should be aligned to 64-bits.
> Ack for that.
> 
>>> +
>>> +        /* The buffers must hold at least the rpmsg header */
>>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>>> +            dev_err(&vdev->dev,
>>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        dev_dbg(&vdev->dev,
>>> +            "vdev config: version=%d, rx buf sz = 0x%x, tx buf sz =
>>> 0x%x\n",
>>> +            version, vrp->rx_buf_size, vrp->tx_buf_size);
>>> +    } else {
>>> +        vrp->rx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
>>> +        vrp->tx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
>>> +    }
>>>    -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>>>> buf_size;
>>> +    total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
>>> +              (vrp->num_tx_buf * vrp->tx_buf_size);
>>>          /* allocate coherent memory for the buffers */
>>>        bufs_va = dma_alloc_coherent(vdev->dev.parent,
>>> @@ -875,14 +928,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>        vrp->rx_bufs = bufs_va;
>>>          /* and second part is dedicated for TX */
>>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>> +    vrp->tx_bufs = bufs_va + (vrp->num_rx_buf * vrp->rx_buf_size);
>>
>>
>> We should have a cache or 64-bit alignement here. or perhaps such
>> constraints should be specified in the config space?
>>
> 
> I prefer to specify in the config space.
> 
>>>          /* set up the receive buffers */
>>>        for (i = 0; i < vrp->num_rx_buf; i++) {
>>>            struct scatterlist sg;
>>> -        void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
>>> +        void *cpu_addr = vrp->rx_bufs + i * vrp->rx_buf_size;
>>>    -        rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>> +        rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
>>>              err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>>                          GFP_KERNEL);
>>> @@ -965,8 +1018,8 @@ static int rpmsg_remove_device(struct device
>>> *dev, void *data)
>>>    static void rpmsg_remove(struct virtio_device *vdev)
>>>    {
>>>        struct virtproc_info *vrp = vdev->priv;
>>> -    unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
>>> -    size_t total_buf_space = num_bufs * vrp->buf_size;
>>> +    size_t total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
>>> +                 (vrp->num_tx_buf * vrp->tx_buf_size);
>>>        int ret;
>>>          virtio_reset_device(vdev);
>>> @@ -992,6 +1045,7 @@ static struct virtio_device_id id_table[] = {
>>>      static unsigned int features[] = {
>>>        VIRTIO_RPMSG_F_NS,
>>> +    VIRTIO_RPMSG_F_BUFSZ,
>>>    };
>>>      static struct virtio_driver virtio_ipc_driver = {
>>> diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/
>>> virtio_rpmsg.h
>>> new file mode 100644
>>> index 000000000000..77a530514d86
>>> --- /dev/null
>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>> @@ -0,0 +1,34 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) Pinecone Inc. 2019
>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>> + * Copyright (C) Advanced Micro Devices, Inc.
>>
>> No year specified for AMD copyright?
> 
> Ack, will fix.
> 
>>
>>> + */
>>> +
>>> +#ifndef _LINUX_VIRTIO_RPMSG_H
>>> +#define _LINUX_VIRTIO_RPMSG_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/virtio_types.h>
>>> +
>>> +/* The feature bitmap for virtio rpmsg */
>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service
>>> notifications */
>>> +#define VIRTIO_RPMSG_F_BUFSZ    1 /* RP get buffer size from config
>>> space */
>>> +
>>> +/**
>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>>> + *
>>> + * @version: version of this structure. current version is 1.
>>> + * @size:    size of this structure. unused for the remoteproc virtio
>>> backend.
>>
>> reference to remoteproc to remove
>>
> 
> Ack.
> 
>>> + * @txbuf_size: Tx buf size from remote's view. For Linux this is rx
>>> buf size.
>>> + * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx
>>> buf size.
>>> + */
>>> +struct virtio_rpmsg_config {
>>> +    u8 version;
>>> +    __virtio16 size;
>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>> +    __virtio32 txbuf_size;
>>> +    __virtio32 rxbuf_size;
>>> +} __packed;
>>
>>
>> As mentioned above
>> - The size should be be a multiple of four to ensure 64-bit alignment.
>> - I would add an alignment field to align the address of the TX buffers
>> to the cache line.
>>
> 
> Ok, I will change and test.
> 
>> Thanks,
>> Arnaud
>>
>>
>>> +
>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>
> 

Re: [PATCH v3 4/4] samples: rpmsg: add MTU size info

[Mathieu Poirier]

On Thu, Jun 04, 2026 at 05:28:13PM -0500, Shah, Tanmay wrote:
> 
> 
> On 6/2/2026 3:34 AM, Arnaud POULIQUEN wrote:
> > 
> > 
> > On 5/29/26 18:43, Tanmay Shah wrote:
> >> RPMsg MTU size can be variable now and no longer hardcoded to 512 bytes.
> >> Add log to the sample driver that prints current MTU size of the rpmsg
> >> buffer.
> >>
> >> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >> ---
> >>
> >> Changes in v3:
> >>    - Check for error when retrieving MTU size
> >>    - %s/mtu/MTU/
> >>
> >>   samples/rpmsg/rpmsg_client_sample.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/
> >> rpmsg_client_sample.c
> >> index ae5081662283..55afa53189af 100644
> >> --- a/samples/rpmsg/rpmsg_client_sample.c
> >> +++ b/samples/rpmsg/rpmsg_client_sample.c
> >> @@ -52,6 +52,7 @@ static int rpmsg_sample_probe(struct rpmsg_device
> >> *rpdev)
> >>   {
> >>       int ret;
> >>       struct instance_data *idata;
> >> +    ssize_t mtu;
> >>         dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x!\n",
> >>                       rpdev->src, rpdev->dst);
> >> @@ -62,6 +63,14 @@ static int rpmsg_sample_probe(struct rpmsg_device
> >> *rpdev)
> >>         dev_set_drvdata(&rpdev->dev, idata);
> >>   +    mtu = rpmsg_get_mtu(rpdev->ept);
> >> +    if (mtu < 0) {
> >> +        dev_warn(&rpdev->dev, "invalid rpmsg MTU size = %ld\n", mtu);
> >> +        return mtu;
> >> +    }
> >> +
> >> +    dev_info(&rpdev->dev, "rpmsg MTU size = %ld\n", mtu);
> >> +
> > 
> > Do you really need this commit? rpmsg_send should return an error if the
> > buffer size is insufficient [1].
> > 
> > [1] https://elixir.bootlin.com/linux/v7.0.10/source/drivers/rpmsg/
> > virtio_rpmsg_bus.c#L517
> > 
> 
> Here, I just want to demonstrate rpmsg_get_mtu() API. Now we can have a
> configurable size of the RPMsg buffer. So rpmsg_get_mtu() API can be
> used to check correct buffer length before even using rpmsg_send() API.
> 
> I think I should check msg length against mtu size as well.

Do you plan on sending a new revision of this set or should I move forward with
this one?

> 
> 
> > Regards,
> > Arnaud
> > 
> >>       /* send a message to our remote processor */
> >>       ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
> >>       if (ret) {
> > 
> 

Re: [PATCH v3 4/4] samples: rpmsg: add MTU size info

[Shah, Tanmay]

On 6/9/2026 11:11 AM, Mathieu Poirier wrote:
> On Thu, Jun 04, 2026 at 05:28:13PM -0500, Shah, Tanmay wrote:
>>
>>
>> On 6/2/2026 3:34 AM, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 5/29/26 18:43, Tanmay Shah wrote:
>>>> RPMsg MTU size can be variable now and no longer hardcoded to 512 bytes.
>>>> Add log to the sample driver that prints current MTU size of the rpmsg
>>>> buffer.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>>    - Check for error when retrieving MTU size
>>>>    - %s/mtu/MTU/
>>>>
>>>>   samples/rpmsg/rpmsg_client_sample.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/
>>>> rpmsg_client_sample.c
>>>> index ae5081662283..55afa53189af 100644
>>>> --- a/samples/rpmsg/rpmsg_client_sample.c
>>>> +++ b/samples/rpmsg/rpmsg_client_sample.c
>>>> @@ -52,6 +52,7 @@ static int rpmsg_sample_probe(struct rpmsg_device
>>>> *rpdev)
>>>>   {
>>>>       int ret;
>>>>       struct instance_data *idata;
>>>> +    ssize_t mtu;
>>>>         dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x!\n",
>>>>                       rpdev->src, rpdev->dst);
>>>> @@ -62,6 +63,14 @@ static int rpmsg_sample_probe(struct rpmsg_device
>>>> *rpdev)
>>>>         dev_set_drvdata(&rpdev->dev, idata);
>>>>   +    mtu = rpmsg_get_mtu(rpdev->ept);
>>>> +    if (mtu < 0) {
>>>> +        dev_warn(&rpdev->dev, "invalid rpmsg MTU size = %ld\n", mtu);
>>>> +        return mtu;
>>>> +    }
>>>> +
>>>> +    dev_info(&rpdev->dev, "rpmsg MTU size = %ld\n", mtu);
>>>> +
>>>
>>> Do you really need this commit? rpmsg_send should return an error if the
>>> buffer size is insufficient [1].
>>>
>>> [1] https://elixir.bootlin.com/linux/v7.0.10/source/drivers/rpmsg/
>>> virtio_rpmsg_bus.c#L517
>>>
>>
>> Here, I just want to demonstrate rpmsg_get_mtu() API. Now we can have a
>> configurable size of the RPMsg buffer. So rpmsg_get_mtu() API can be
>> used to check correct buffer length before even using rpmsg_send() API.
>>
>> I think I should check msg length against mtu size as well.
> 
> Do you plan on sending a new revision of this set or should I move forward with
> this one?
> 

Hi,

I plan to send a new revision, but would like your input on the idea of
this patch. I would like to use rpmsg_get_mtu() size API in the sample
demo to print tx buffer size. This is because buffer size can change now
from one firmware to another firmware.

May be we can print it only once during the driver probe. Open to any
idea that show use of rpmsg_get_mtu().

Thanks,
Tanmay

>>
>>
>>> Regards,
>>> Arnaud
>>>
>>>>       /* send a message to our remote processor */
>>>>       ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
>>>>       if (ret) {
>>>
>>

Re: [PATCH v3 3/4] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 6/5/2026 3:25 AM, Arnaud POULIQUEN wrote:
> Hi Tanmay,
> 
> On 6/4/26 22:31, Shah, Tanmay wrote:
>> Thank You for the reviews, please find my comments below:
>>
>> On 6/2/2026 3:25 AM, Arnaud POULIQUEN wrote:
>>> Hi Tanmay,
>>>
>>> On 5/29/26 18:43, Tanmay Shah wrote:
>>>> 512 bytes isn't always suitable for all case, let firmware
>>>> maker decide the best value from resource table.
>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>>     - change version field from u16 to u8
>>>>     - introduce size field in the rpmsg_virtio_config structure
>>>>     - check version field is set to any non-zero value.
>>>>     - check size field is not 0.
>>>>     - Remove field for private config, as not needed for now.
>>>>     - add documentation of rpmsg_virtio_config structure
>>>>
>>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 90 +++++++++++++++++++++++
>>>> +------
>>>>    include/linux/rpmsg/virtio_rpmsg.h | 34 +++++++++++
>>>>    2 files changed, 106 insertions(+), 18 deletions(-)
>>>>    create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>>
>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>>> virtio_rpmsg_bus.c
>>>> index 99df1ae07055..f1ab8e792f3d 100644
>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> @@ -20,6 +20,7 @@
>>>>    #include <linux/rpmsg.h>
>>>>    #include <linux/rpmsg/byteorder.h>
>>>>    #include <linux/rpmsg/ns.h>
>>>> +#include <linux/rpmsg/virtio_rpmsg.h>
>>>>    #include <linux/scatterlist.h>
>>>>    #include <linux/slab.h>
>>>>    #include <linux/sched.h>
>>>> @@ -39,7 +40,8 @@
>>>>     * @tx_bufs:    kernel address of tx buffers
>>>>     * @num_rx_buf: total number of rx buffers
>>>>     * @num_tx_buf: total number of tx buffers
>>>> - * @buf_size:   size of one rx or tx buffer
>>>> + * @rx_buf_size: size of one rx buffer
>>>> + * @tx_buf_size: size of one tx buffer
>>>>     * @last_tx_buf: index of last tx buffer used
>>>>     * @bufs_dma:    dma base addr of the buffers
>>>>     * @tx_lock:    protects svq and tx_bufs, to allow concurrent
>>>> senders.
>>>> @@ -59,7 +61,8 @@ struct virtproc_info {
>>>>        void *rx_bufs, *tx_bufs;
>>>>        unsigned int num_rx_buf;
>>>>        unsigned int num_tx_buf;
>>>> -    unsigned int buf_size;
>>>> +    unsigned int rx_buf_size;
>>>> +    unsigned int tx_buf_size;
>>>>        int last_tx_buf;
>>>>        dma_addr_t bufs_dma;
>>>>        struct mutex tx_lock;
>>>> @@ -68,9 +71,6 @@ struct virtproc_info {
>>>>        wait_queue_head_t sendq;
>>>>    };
>>>>    -/* 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
>>>> @@ -128,7 +128,9 @@ struct virtio_rpmsg_channel {
>>>>     * processor.
>>>>     */
>>>>    #define MAX_RPMSG_NUM_BUFS    (256)
>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>>> +
>>>> +#define RPMSG_VDEV_CONFIG_VER 1
>>>
>>> I would rename it
>>>
>>> #define RPMSG_VDEV_CONFIG_V1 1
>>
>> We might not need this define at all. I should have removed it in this
>> patch. Please see below [1].
>>
>>>
>>>>      /*
>>>>     * Local addresses are dynamically allocated on-demand.
>>>> @@ -444,7 +446,7 @@ static void *get_a_tx_buf(struct virtproc_info
>>>> *vrp)
>>>>          /* either pick the next unused tx buffer */
>>>>        if (vrp->last_tx_buf < vrp->num_tx_buf)
>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>>>        /* or recycle a used one */
>>>>        else
>>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>>> @@ -514,7 +516,7 @@ static int rpmsg_send_offchannel_raw(struct
>>>> rpmsg_device *rpdev,
>>>>         * messaging), or to improve the buffer allocator, to support
>>>>         * variable-length buffer sizes.
>>>>         */
>>>> -    if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>>>> +    if (len > vrp->tx_buf_size - sizeof(struct rpmsg_hdr)) {
>>>>            dev_err(dev, "message is too big (%d)\n", len);
>>>>            return -EMSGSIZE;
>>>>        }
>>>> @@ -647,7 +649,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct
>>>> rpmsg_endpoint *ept)
>>>>        struct rpmsg_device *rpdev = ept->rpdev;
>>>>        struct virtio_rpmsg_channel *vch =
>>>> to_virtio_rpmsg_channel(rpdev);
>>>>    -    return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
>>>> +    return vch->vrp->tx_buf_size - sizeof(struct rpmsg_hdr);
>>>>    }
>>>>      static int rpmsg_recv_single(struct virtproc_info *vrp, struct
>>>> device *dev,
>>>> @@ -673,7 +675,7 @@ static int rpmsg_recv_single(struct virtproc_info
>>>> *vrp, struct device *dev,
>>>>         * We currently use fixed-sized buffers, so trivially sanitize
>>>>         * the reported payload length.
>>>>         */
>>>> -    if (len > vrp->buf_size ||
>>>> +    if (len > vrp->rx_buf_size ||
>>>>            msg_len > (len - sizeof(struct rpmsg_hdr))) {
>>>>            dev_warn(dev, "inbound msg too big: (%d, %d)\n", len,
>>>> msg_len);
>>>>            return -EINVAL;
>>>> @@ -706,7 +708,7 @@ static int rpmsg_recv_single(struct virtproc_info
>>>> *vrp, struct device *dev,
>>>>            dev_warn_ratelimited(dev, "msg received with no
>>>> recipient\n");
>>>>          /* publish the real size of the buffer */
>>>> -    rpmsg_sg_init(&sg, msg, vrp->buf_size);
>>>> +    rpmsg_sg_init(&sg, msg, vrp->rx_buf_size);
>>>>          /* add the buffer back to the remote processor's virtqueue */
>>>>        err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>>> @@ -824,6 +826,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>        int err = 0, i;
>>>>        size_t total_buf_space;
>>>>        bool notify;
>>>> +    u8 version;
>>>> +    u16 size;
>>>>          vrp = kzalloc_obj(*vrp);
>>>>        if (!vrp)
>>>> @@ -855,9 +859,58 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>        else
>>>>            vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>>>    -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>>> +    /*
>>>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure
>>>> buf
>>>> +     * size from virtio device config space from the resource table.
>>>> +     * If the feature is not supported, then assign default buf size.
>>>> +     */
>>>
>>> Seems to me that It would be nice to document the config space in
>>> rpmsg.rst
>>>
>>
>> Ack.
>>
>>>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 version, &version);
>>>> +        if (version == 0) {
>>>
>>>          if (version != RPMSG_VDEV_CONFIG_V1) {
>>>
>>
>> [1] Here we have to allow any non-zero version to be valid, and make
>> sure any future version is always backward compatible.
>>
>> For example, if we need v2 of the structure, then that should be
>> compatible with v1. So, old kernel keeps working with the new firmware
>> with limited functionality supported by the kernel. And new kernel keep
>> working with the old firmware, with the limited functionality supported
>> by the firmware.
>>
>> That is just my view. I am open to more ideas, thank you.
> 
> My concern is that you allow any version here, while we define only a V1.
> If we need a V2 we will probably have to modify this part of the code.
> 

So that means, the firmware that has v2, will never work with the old
kernel that supports only v1. I would like to design in a way, where
firmware has v2, but it is back compatible with v1. New firmware can
still work with old kernel with only functionality that was supported in
v1.

Mathieu do you have any opinion on this? I don't know if there is any
standard accepted design pattern for this type of compatibility.


use  | Linux  | firmware |
case | kernel | version  |  Note    |
     |        |          |          |
  1  |    v1  |    v1    |   works  |
  2  |    v2  |    v1    |  old firmware features work.
  3  |    v1  |    v2    |  new firmware features won't work.

In use-case 3, Do we want to fail completely and say old Linux will not
support new firmware at all ?

Thanks,
Tanmay


>>
>>>> +            dev_err(&vdev->dev, "invalid version of vdev config\n");
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        /*
>>>> +         * The size field is not used for the remoteproc virtio
>>>> transport,
>>>> +         * but kept for any future transport to use
>>>> +         */
>>>
>>> I suggest to not mention the virtio transport, indeed we should decouple
>>> the virtio device from the virtio transport.
>>>
>>
>> Ack.
>>
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 size, &size);
>>>> +        if (size == 0) {
>>>
>>>      if (size != sizeof(virtio_rpmsg_config)) {
>>>
>>
>> So, I think sizeof(virtio_rpmsg_config) on the remote side may not be
>> the same as in the linux kernel. Remote side can have its private
>> variables which might not needed on the linux side:
>>
>> For example, the open-amp library defines the structure differently than
>> linux:
>> https://github.com/OpenAMP/open-amp/
>> blob/23d4c5d7a5c5dd08b74d4ba828243988592337cb/lib/include/openamp/
>> rpmsg_virtio.h#L70
>>
>> There is 'split_shpool' extra variable which is not needed by the linux.
> 
> The 'split_shpool' setting is currently an internal OpenAMP configuration
> and is not part of a configuration space. If there is a need to split
> memory regions for RX and TX buffers, the implementation should use another
> method, perhaps with DA and PA addresses that specify the RX and TX buffer
> locations, as for the vrings in the resource table.
> I would suggest to not address this in version 1.
> 
> From my perspective, the configuration space is a common structure that
> the device and the driver share. In that case, checking the size makes
> sense.

Ack.

> Thanks,
> Arnaud
> 
>>
>> That is why restriction on the size isn't needed IMHO.
>>
>>>> +            dev_err(&vdev->dev, "invalid size of vdev config\n");
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        /* note: tx and rx are defined from remote view */
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 txbuf_size, &vrp->rx_buf_size);
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 rxbuf_size, &vrp->tx_buf_size);
>>>
>>> I wonder if we should not impose a size aligned on 64-bits
>>>
>>
>> I think you mean size should be aligned to 64-bits.
>> Ack for that.
>>
>>>> +
>>>> +        /* The buffers must hold at least the rpmsg header */
>>>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>>>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>>>> +            dev_err(&vdev->dev,
>>>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>>>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        dev_dbg(&vdev->dev,
>>>> +            "vdev config: version=%d, rx buf sz = 0x%x, tx buf sz =
>>>> 0x%x\n",
>>>> +            version, vrp->rx_buf_size, vrp->tx_buf_size);
>>>> +    } else {
>>>> +        vrp->rx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
>>>> +        vrp->tx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
>>>> +    }
>>>>    -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>>>>> buf_size;
>>>> +    total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
>>>> +              (vrp->num_tx_buf * vrp->tx_buf_size);
>>>>          /* allocate coherent memory for the buffers */
>>>>        bufs_va = dma_alloc_coherent(vdev->dev.parent,
>>>> @@ -875,14 +928,14 @@ static int rpmsg_probe(struct virtio_device
>>>> *vdev)
>>>>        vrp->rx_bufs = bufs_va;
>>>>          /* and second part is dedicated for TX */
>>>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>>> +    vrp->tx_bufs = bufs_va + (vrp->num_rx_buf * vrp->rx_buf_size);
>>>
>>>
>>> We should have a cache or 64-bit alignement here. or perhaps such
>>> constraints should be specified in the config space?
>>>
>>
>> I prefer to specify in the config space.
>>
>>>>          /* set up the receive buffers */
>>>>        for (i = 0; i < vrp->num_rx_buf; i++) {
>>>>            struct scatterlist sg;
>>>> -        void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
>>>> +        void *cpu_addr = vrp->rx_bufs + i * vrp->rx_buf_size;
>>>>    -        rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>>> +        rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
>>>>              err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>>>                          GFP_KERNEL);
>>>> @@ -965,8 +1018,8 @@ static int rpmsg_remove_device(struct device
>>>> *dev, void *data)
>>>>    static void rpmsg_remove(struct virtio_device *vdev)
>>>>    {
>>>>        struct virtproc_info *vrp = vdev->priv;
>>>> -    unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
>>>> -    size_t total_buf_space = num_bufs * vrp->buf_size;
>>>> +    size_t total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
>>>> +                 (vrp->num_tx_buf * vrp->tx_buf_size);
>>>>        int ret;
>>>>          virtio_reset_device(vdev);
>>>> @@ -992,6 +1045,7 @@ static struct virtio_device_id id_table[] = {
>>>>      static unsigned int features[] = {
>>>>        VIRTIO_RPMSG_F_NS,
>>>> +    VIRTIO_RPMSG_F_BUFSZ,
>>>>    };
>>>>      static struct virtio_driver virtio_ipc_driver = {
>>>> diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/
>>>> virtio_rpmsg.h
>>>> new file mode 100644
>>>> index 000000000000..77a530514d86
>>>> --- /dev/null
>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>> @@ -0,0 +1,34 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Copyright (C) Pinecone Inc. 2019
>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>>> + * Copyright (C) Advanced Micro Devices, Inc.
>>>
>>> No year specified for AMD copyright?
>>
>> Ack, will fix.
>>
>>>
>>>> + */
>>>> +
>>>> +#ifndef _LINUX_VIRTIO_RPMSG_H
>>>> +#define _LINUX_VIRTIO_RPMSG_H
>>>> +
>>>> +#include <linux/types.h>
>>>> +#include <linux/virtio_types.h>
>>>> +
>>>> +/* The feature bitmap for virtio rpmsg */
>>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service
>>>> notifications */
>>>> +#define VIRTIO_RPMSG_F_BUFSZ    1 /* RP get buffer size from config
>>>> space */
>>>> +
>>>> +/**
>>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>>>> + *
>>>> + * @version: version of this structure. current version is 1.
>>>> + * @size:    size of this structure. unused for the remoteproc virtio
>>>> backend.
>>>
>>> reference to remoteproc to remove
>>>
>>
>> Ack.
>>
>>>> + * @txbuf_size: Tx buf size from remote's view. For Linux this is rx
>>>> buf size.
>>>> + * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx
>>>> buf size.
>>>> + */
>>>> +struct virtio_rpmsg_config {
>>>> +    u8 version;
>>>> +    __virtio16 size;
>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>> +    __virtio32 txbuf_size;
>>>> +    __virtio32 rxbuf_size;
>>>> +} __packed;
>>>
>>>
>>> As mentioned above
>>> - The size should be be a multiple of four to ensure 64-bit alignment.

Here, do you mean to document that size should be a multiple of 4 ? How
to enforce the alignment to 4? We can only document right?

Thanks,
Tanmay

>>> - I would add an alignment field to align the address of the TX buffers
>>> to the cache line.
>>>
>>
>> Ok, I will change and test.
>>
>>> Thanks,
>>> Arnaud
>>>
>>>
>>>> +
>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>
>>
>