[PATCH v2 0/3] Enhance RPMsg buffer management

[PATCH v2 0/3] Enhance RPMsg buffer management

[Tanmay Shah]

Take rx and tx buffer size from the virtio device config space in the
resource table. This allows each firmware to configure RPMsg buffer size
as needed in each direction.

Test performed:
  - Test [1/2] patch with existing firmware as it is, rpmsg working.
  - Verify [2/2] patch works with the existing firmware
  - Verify [2/2] patch works with the firmware that configures
    different tx & rx buf size via vdev config space.

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

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

Tanmay Shah (3):
  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    | 124 ++++++++++++++++++----------
 include/linux/rpmsg/virtio_rpmsg.h  |  27 ++++++
 samples/rpmsg/rpmsg_client_sample.c |   3 +
 3 files changed, 109 insertions(+), 45 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h


base-commit: fcdf2df56d34a3f04cab0725c5bc3abdaa73c2be
-- 
2.34.1

[PATCH v2 1/3] 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>
---

Test performed:
  - Test this patch with existing firmware as it is, rpmsg working.

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/

 drivers/rpmsg/virtio_rpmsg_bus.c | 68 ++++++++++++++++----------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5ae15111fb4f..e59d8cf9b975 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -35,13 +35,14 @@
  * @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
- * @buf_size:   size of one rx or tx buffer
+ * @rx_bufs:	kernel address of rx buffers
+ * @tx_bufs:	kernel address of tx buffers
+ * @num_rx_buf:	total number of buffers for rx
+ * @num_tx_buf:	total number of buffers for tx
+ * @buf_size:	size of one rx or tx buffer
  * @last_sbuf:	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,8 +56,9 @@
 struct virtproc_info {
 	struct virtio_device *vdev;
 	struct virtqueue *rvq, *svq;
-	void *rbufs, *sbufs;
-	unsigned int num_bufs;
+	void *rx_bufs, *tx_bufs;
+	unsigned int num_rx_buf;
+	unsigned int num_tx_buf;
 	unsigned int buf_size;
 	int last_sbuf;
 	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,12 +442,9 @@ 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_sbuf < vrp->num_bufs / 2)
-		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
+	/* either pick the next unused tx buffer */
+	if (vrp->last_sbuf < vrp->num_tx_buf)
+		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -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_sbuf < vrp->num_bufs / 2 ||
+	if (vrp->last_sbuf < 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,16 +871,16 @@ 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 */
-	vrp->rbufs = bufs_va;
+	/* first part of the buffers is dedicated for RX */
+	vrp->rx_bufs = bufs_va;
 
-	/* and half is dedicated for TX */
-	vrp->sbufs = 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->rbufs + i * vrp->buf_size;
+		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
 
 		rpmsg_sg_init(&sg, cpu_addr, 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);
@@ -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 v2 2/3] 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>
---

Test done:
  - Verify this patch works with the existing firmware
  - Verify this patch works with the firmware that configures
    differt tx & rx buf size

Changes in v2:
  - %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
  - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h

 drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
 include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
 2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
  * @num_tx_buf:	total number of buffers for tx
- * @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_sbuf:	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_sbuf;
 	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,7 @@ struct virtio_rpmsg_channel {
  * processor.
  */
 #define MAX_RPMSG_NUM_BUFS	(256)
-#define MAX_RPMSG_BUF_SIZE	(512)
+#define DEFAULT_RPMSG_BUF_SIZE	(512)
 
 /*
  * Local addresses are dynamically allocated on-demand.
@@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 
 	/* either pick the next unused tx buffer */
 	if (vrp->last_sbuf < vrp->num_tx_buf)
-		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
+		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	int err = 0, i;
 	size_t total_buf_space;
 	bool notify;
+	u16 version;
 
 	vrp = kzalloc_obj(*vrp);
 	if (!vrp)
@@ -855,9 +856,41 @@ 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)) {
+		/* note: virtio_rpmsg_config is defined from remote view */
+		version = 0;
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     version, &version);
+		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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
--- /dev/null
+++ b/include/linux/rpmsg/virtio_rpmsg.h
@@ -0,0 +1,27 @@
+/* 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 {
+	__virtio16 version;
+	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
+	__virtio32 txbuf_size;
+	__virtio32 rxbuf_size;
+	__virtio32 reserved[14]; /* Reserve for the future use */
+	/* Put the customize config here */
+} __packed;
+
+#endif /* _LINUX_VIRTIO_RPMSG_H */
-- 
2.34.1

[PATCH v2 3/3] 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 rpmsg
buffer.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 samples/rpmsg/rpmsg_client_sample.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/rpmsg_client_sample.c
index ae5081662283..bc6d16e81d69 100644
--- a/samples/rpmsg/rpmsg_client_sample.c
+++ b/samples/rpmsg/rpmsg_client_sample.c
@@ -62,6 +62,9 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(&rpdev->dev, idata);
 
+	dev_info(&rpdev->dev, "rpmsg mtu size = %ld\n",
+		 rpmsg_get_mtu(rpdev->ept));
+
 	/* send a message to our remote processor */
 	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
 	if (ret) {
-- 
2.34.1

Re: [PATCH v2 1/3] rpmsg: virtio_rpmsg_bus: allow different size of tx and rx bufs

[Mathieu Poirier]

Hi Tanmay

(Apologies for the late review)

On Wed, Apr 29, 2026 at 09:10:51AM -0700, Tanmay Shah wrote:
> 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>
> ---
> 
> Test performed:
>   - Test this patch with existing firmware as it is, rpmsg working.
> 
> 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/

Please split this patch in two parts - one to do the refactoring of the
tx/rx_buf and another one for the varying size.

> 
>  drivers/rpmsg/virtio_rpmsg_bus.c | 68 ++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 5ae15111fb4f..e59d8cf9b975 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -35,13 +35,14 @@
>   * @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
> - * @buf_size:   size of one rx or tx buffer
> + * @rx_bufs:	kernel address of rx buffers
> + * @tx_bufs:	kernel address of tx buffers
> + * @num_rx_buf:	total number of buffers for rx
> + * @num_tx_buf:	total number of buffers for tx
> + * @buf_size:	size of one rx or tx buffer
>   * @last_sbuf:	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,8 +56,9 @@
>  struct virtproc_info {
>  	struct virtio_device *vdev;
>  	struct virtqueue *rvq, *svq;
> -	void *rbufs, *sbufs;
> -	unsigned int num_bufs;
> +	void *rx_bufs, *tx_bufs;
> +	unsigned int num_rx_buf;
> +	unsigned int num_tx_buf;
>  	unsigned int buf_size;
>  	int last_sbuf;
>  	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,12 +442,9 @@ 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_sbuf < vrp->num_bufs / 2)
> -		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> +	/* either pick the next unused tx buffer */
> +	if (vrp->last_sbuf < vrp->num_tx_buf)
> +		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
>  		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -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_sbuf < vrp->num_bufs / 2 ||
> +	if (vrp->last_sbuf < 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,16 +871,16 @@ 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 */
> -	vrp->rbufs = bufs_va;
> +	/* first part of the buffers is dedicated for RX */
> +	vrp->rx_bufs = bufs_va;
>  
> -	/* and half is dedicated for TX */
> -	vrp->sbufs = 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->rbufs + i * vrp->buf_size;
> +		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
>  
>  		rpmsg_sg_init(&sg, cpu_addr, 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);
> @@ -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
> 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Mathieu Poirier]

On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
> ---
> 
> Test done:
>   - Verify this patch works with the existing firmware
>   - Verify this patch works with the firmware that configures
>     differt tx & rx buf size
> 
> Changes in v2:
>   - %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
>   - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
> 
>  drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
>  include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>  2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>   * @num_tx_buf:	total number of buffers for tx
> - * @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_sbuf:	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_sbuf;
>  	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,7 @@ struct virtio_rpmsg_channel {
>   * processor.
>   */
>  #define MAX_RPMSG_NUM_BUFS	(256)
> -#define MAX_RPMSG_BUF_SIZE	(512)
> +#define DEFAULT_RPMSG_BUF_SIZE	(512)
>  
>  /*
>   * Local addresses are dynamically allocated on-demand.
> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>  
>  	/* either pick the next unused tx buffer */
>  	if (vrp->last_sbuf < vrp->num_tx_buf)
> -		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
> +		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
>  		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	int err = 0, i;
>  	size_t total_buf_space;
>  	bool notify;
> +	u16 version;
>  
>  	vrp = kzalloc_obj(*vrp);
>  	if (!vrp)
> @@ -855,9 +856,41 @@ 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)) {
> +		/* note: virtio_rpmsg_config is defined from remote view */
> +		version = 0;
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     version, &version);
> +		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);
> +

A check is also needed to make sure the version received from the resource table
is '0'.

> +		/* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
> --- /dev/null
> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> @@ -0,0 +1,27 @@
> +/* 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 {
> +	__virtio16 version;
> +	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> +	__virtio32 txbuf_size;
> +	__virtio32 rxbuf_size;
> +	__virtio32 reserved[14]; /* Reserve for the future use */

66 byte for the configuratio space?  I'm puzzled about the reserved[14], how did
you come up with that number?  

The rest looks good to me.

> +	/* Put the customize config here */
> +} __packed;
> +
> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Mathieu Poirier]

On Tue, May 19, 2026 at 11:36:47AM -0600, Mathieu Poirier wrote:
> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
> > ---
> > 
> > Test done:
> >   - Verify this patch works with the existing firmware
> >   - Verify this patch works with the firmware that configures
> >     differt tx & rx buf size
> > 
> > Changes in v2:
> >   - %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
> >   - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
> > 
> >  drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
> >  include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
> >  2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
> >   * @num_tx_buf:	total number of buffers for tx
> > - * @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_sbuf:	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_sbuf;
> >  	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,7 @@ struct virtio_rpmsg_channel {
> >   * processor.
> >   */
> >  #define MAX_RPMSG_NUM_BUFS	(256)
> > -#define MAX_RPMSG_BUF_SIZE	(512)
> > +#define DEFAULT_RPMSG_BUF_SIZE	(512)
> >  
> >  /*
> >   * Local addresses are dynamically allocated on-demand.
> > @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >  
> >  	/* either pick the next unused tx buffer */
> >  	if (vrp->last_sbuf < vrp->num_tx_buf)
> > -		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
> > +		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
> >  	/* or recycle a used one */
> >  	else
> >  		ret = virtqueue_get_buf(vrp->svq, &len);
> > @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	int err = 0, i;
> >  	size_t total_buf_space;
> >  	bool notify;
> > +	u16 version;
> >  
> >  	vrp = kzalloc_obj(*vrp);
> >  	if (!vrp)
> > @@ -855,9 +856,41 @@ 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)) {
> > +		/* note: virtio_rpmsg_config is defined from remote view */
> > +		version = 0;
> > +		virtio_cread(vdev, struct virtio_rpmsg_config,
> > +			     version, &version);
> > +		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);
> > +
> 
> A check is also needed to make sure the version received from the resource table
> is '0'.
> 
> > +		/* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
> > --- /dev/null
> > +++ b/include/linux/rpmsg/virtio_rpmsg.h
> > @@ -0,0 +1,27 @@
> > +/* 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 {
> > +	__virtio16 version;
> > +	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> > +	__virtio32 txbuf_size;
> > +	__virtio32 rxbuf_size;
> > +	__virtio32 reserved[14]; /* Reserve for the future use */
> 
> 66 byte for the configuratio space?  I'm puzzled about the reserved[14], how did
> you come up with that number?  
> 
> The rest looks good to me.

... and I am done reviewing this set.

> 
> > +	/* Put the customize config here */
> > +} __packed;
> > +
> > +#endif /* _LINUX_VIRTIO_RPMSG_H */
> > -- 
> > 2.34.1
> > 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

On 5/19/26 19:36, Mathieu Poirier wrote:
> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
>> ---
>>
>> Test done:
>>    - Verify this patch works with the existing firmware
>>    - Verify this patch works with the firmware that configures
>>      differt tx & rx buf size
>>
>> Changes in v2:
>>    - %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
>>    - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
>>
>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
>>   include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>>   2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>>    * @num_tx_buf:	total number of buffers for tx
>> - * @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_sbuf:	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_sbuf;
>>   	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,7 @@ struct virtio_rpmsg_channel {
>>    * processor.
>>    */
>>   #define MAX_RPMSG_NUM_BUFS	(256)
>> -#define MAX_RPMSG_BUF_SIZE	(512)
>> +#define DEFAULT_RPMSG_BUF_SIZE	(512)
>>   
>>   /*
>>    * Local addresses are dynamically allocated on-demand.
>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>   
>>   	/* either pick the next unused tx buffer */
>>   	if (vrp->last_sbuf < vrp->num_tx_buf)
>> -		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>> +		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>>   	/* or recycle a used one */
>>   	else
>>   		ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>   	int err = 0, i;
>>   	size_t total_buf_space;
>>   	bool notify;
>> +	u16 version;
>>   
>>   	vrp = kzalloc_obj(*vrp);
>>   	if (!vrp)
>> @@ -855,9 +856,41 @@ 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)) {
>> +		/* note: virtio_rpmsg_config is defined from remote view */
>> +		version = 0;
>> +		virtio_cread(vdev, struct virtio_rpmsg_config,
>> +			     version, &version);
>> +		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);
>> +
> 
> A check is also needed to make sure the version received from the resource table
> is '0'.
> 
>> +		/* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
>> --- /dev/null
>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>> @@ -0,0 +1,27 @@
>> +/* 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 {
>> +	__virtio16 version;
>> +	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>> +	__virtio32 txbuf_size;
>> +	__virtio32 rxbuf_size;
>> +	__virtio32 reserved[14]; /* Reserve for the future use */
> 
> 66 byte for the configuratio space?  I'm puzzled about the reserved[14], how did
> you come up with that number?

Is it useful to define the reserved field?
The version should allow us to determine the content.
An other solution is to introduce a'size' field to determine the size of
the structure:
struct virtio_rpmsg_config {
	__virtio16 version;
	__virtio16 size; /* size of the configuration space */
	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
	__virtio32 txbuf_size;
	__virtio32 rxbuf_size;
	__u8 private[0]; /* customized config */
};

> 
> The rest looks good to me.
> 

Looks good to me too.

Thanks,
Arnaud

>> +	/* Put the customize config here */
>> +} __packed;
>> +
>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>> -- 
>> 2.34.1
>>

Re: [PATCH v2 1/3] rpmsg: virtio_rpmsg_bus: allow different size of tx and rx bufs

[Shah, Tanmay]

Hello,

Thanks for the reviews.

On 5/19/2026 11:57 AM, Mathieu Poirier wrote:
> Hi Tanmay
> 
> (Apologies for the late review)
> 

No problem.

> On Wed, Apr 29, 2026 at 09:10:51AM -0700, Tanmay Shah wrote:
>> 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>
>> ---
>>
>> Test performed:
>>   - Test this patch with existing firmware as it is, rpmsg working.
>>
>> 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/
> 
> Please split this patch in two parts - one to do the refactoring of the
> tx/rx_buf and another one for the varying size.

Ack.

> 
>>
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 68 ++++++++++++++++----------------
>>  1 file changed, 34 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 5ae15111fb4f..e59d8cf9b975 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -35,13 +35,14 @@
>>   * @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
>> - * @buf_size:   size of one rx or tx buffer
>> + * @rx_bufs:	kernel address of rx buffers
>> + * @tx_bufs:	kernel address of tx buffers
>> + * @num_rx_buf:	total number of buffers for rx
>> + * @num_tx_buf:	total number of buffers for tx
>> + * @buf_size:	size of one rx or tx buffer
>>   * @last_sbuf:	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,8 +56,9 @@
>>  struct virtproc_info {
>>  	struct virtio_device *vdev;
>>  	struct virtqueue *rvq, *svq;
>> -	void *rbufs, *sbufs;
>> -	unsigned int num_bufs;
>> +	void *rx_bufs, *tx_bufs;
>> +	unsigned int num_rx_buf;
>> +	unsigned int num_tx_buf;
>>  	unsigned int buf_size;
>>  	int last_sbuf;
>>  	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,12 +442,9 @@ 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_sbuf < vrp->num_bufs / 2)
>> -		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>> +	/* either pick the next unused tx buffer */
>> +	if (vrp->last_sbuf < vrp->num_tx_buf)
>> +		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>  	/* or recycle a used one */
>>  	else
>>  		ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -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_sbuf < vrp->num_bufs / 2 ||
>> +	if (vrp->last_sbuf < 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,16 +871,16 @@ 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 */
>> -	vrp->rbufs = bufs_va;
>> +	/* first part of the buffers is dedicated for RX */
>> +	vrp->rx_bufs = bufs_va;
>>  
>> -	/* and half is dedicated for TX */
>> -	vrp->sbufs = 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->rbufs + i * vrp->buf_size;
>> +		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
>>  
>>  		rpmsg_sg_init(&sg, cpu_addr, 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);
>> @@ -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
>>

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 5/19/26 19:36, Mathieu Poirier wrote:
>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
>>> ---
>>>
>>> Test done:
>>>    - Verify this patch works with the existing firmware
>>>    - Verify this patch works with the firmware that configures
>>>      differt tx & rx buf size
>>>
>>> Changes in v2:
>>>    - %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
>>>    - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
>>>
>>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
>>>   include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>>>   2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>>>    * @num_tx_buf:    total number of buffers for tx
>>> - * @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_sbuf:    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_sbuf;
>>>       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,7 @@ struct virtio_rpmsg_channel {
>>>    * processor.
>>>    */
>>>   #define MAX_RPMSG_NUM_BUFS    (256)
>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>>     /*
>>>    * Local addresses are dynamically allocated on-demand.
>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>         /* either pick the next unused tx buffer */
>>>       if (vrp->last_sbuf < vrp->num_tx_buf)
>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>>>       /* or recycle a used one */
>>>       else
>>>           ret = virtqueue_get_buf(vrp->svq, &len);
>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>       int err = 0, i;
>>>       size_t total_buf_space;
>>>       bool notify;
>>> +    u16 version;
>>>         vrp = kzalloc_obj(*vrp);
>>>       if (!vrp)
>>> @@ -855,9 +856,41 @@ 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)) {
>>> +        /* note: virtio_rpmsg_config is defined from remote view */
>>> +        version = 0;
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 version, &version);
>>> +        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);
>>> +
>>
>> A check is also needed to make sure the version received from the
>> resource table
>> is '0'.

I think we should start with versaion = 1. So, can I check the version
number for 1 ?

>>
>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
>>> --- /dev/null
>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>> @@ -0,0 +1,27 @@
>>> +/* 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 {
>>> +    __virtio16 version;
>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>> +    __virtio32 txbuf_size;
>>> +    __virtio32 rxbuf_size;
>>> +    __virtio32 reserved[14]; /* Reserve for the future use */
>>
>> 66 byte for the configuratio space?  I'm puzzled about the
>> reserved[14], how did
>> you come up with that number?

I kept the reserved bytes from the original series as it is. The
original series did not contain version field. With version I don't
think we need reserved field at all. At best, if we want to append
padding bytes, then I think __virtio16 reserved; makes sense. That will
make the structure aligned to 4 byte.

> 
> Is it useful to define the reserved field?

I think reserved field is only useful to keep the structure size aligned
to 4 bytes.

> The version should allow us to determine the content.

Correct, but I think the structure can be aligned if used correct
reserved bytes.

> An other solution is to introduce a'size' field to determine the size of
> the structure:

I think, the resource table already contains config_len field which is
size of the structure:

https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/remoteproc.h#L275

> struct virtio_rpmsg_config {
>     __virtio16 version;
>     __virtio16 size; /* size of the configuration space */
>     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>     __virtio32 txbuf_size;
>     __virtio32 rxbuf_size;
>     __u8 private[0]; /* customized config */

Do we need customized config? I think I should remove this comment from
the original patch as well.

Thanks,
Tanmay

> };
> 
>>
>> The rest looks good to me.
>>
> 
> Looks good to me too.
> 
> Thanks,
> Arnaud
> 
>>> +    /* Put the customize config here */
>>> +} __packed;
>>> +
>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>> -- 
>>> 2.34.1
>>>
> 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

On 5/20/26 16:55, Shah, Tanmay wrote:
> 
> 
> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
>>
>>
>> On 5/19/26 19:36, Mathieu Poirier wrote:
>>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
>>>> ---
>>>>
>>>> Test done:
>>>>     - Verify this patch works with the existing firmware
>>>>     - Verify this patch works with the firmware that configures
>>>>       differt tx & rx buf size
>>>>
>>>> Changes in v2:
>>>>     - %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
>>>>     - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
>>>>
>>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
>>>>    include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>>>>    2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>>>>     * @num_tx_buf:    total number of buffers for tx
>>>> - * @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_sbuf:    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_sbuf;
>>>>        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,7 @@ struct virtio_rpmsg_channel {
>>>>     * processor.
>>>>     */
>>>>    #define MAX_RPMSG_NUM_BUFS    (256)
>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>>>      /*
>>>>     * Local addresses are dynamically allocated on-demand.
>>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>>          /* either pick the next unused tx buffer */
>>>>        if (vrp->last_sbuf < vrp->num_tx_buf)
>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>>>>        /* or recycle a used one */
>>>>        else
>>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>        int err = 0, i;
>>>>        size_t total_buf_space;
>>>>        bool notify;
>>>> +    u16 version;
>>>>          vrp = kzalloc_obj(*vrp);
>>>>        if (!vrp)
>>>> @@ -855,9 +856,41 @@ 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)) {
>>>> +        /* note: virtio_rpmsg_config is defined from remote view */
>>>> +        version = 0;
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 version, &version);
>>>> +        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);
>>>> +
>>>
>>> A check is also needed to make sure the version received from the
>>> resource table
>>> is '0'.
> 
> I think we should start with versaion = 1. So, can I check the version
> number for 1 ?
> 
>>>
>>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
>>>> --- /dev/null
>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>> @@ -0,0 +1,27 @@
>>>> +/* 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 {
>>>> +    __virtio16 version;
>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>> +    __virtio32 txbuf_size;
>>>> +    __virtio32 rxbuf_size;
>>>> +    __virtio32 reserved[14]; /* Reserve for the future use */
>>>
>>> 66 byte for the configuratio space?  I'm puzzled about the
>>> reserved[14], how did
>>> you come up with that number?
> 
> I kept the reserved bytes from the original series as it is. The
> original series did not contain version field. With version I don't
> think we need reserved field at all. At best, if we want to append
> padding bytes, then I think __virtio16 reserved; makes sense. That will
> make the structure aligned to 4 byte.
> 
>>
>> Is it useful to define the reserved field?
> 
> I think reserved field is only useful to keep the structure size aligned
> to 4 bytes.
> 
>> The version should allow us to determine the content.
> 
> Correct, but I think the structure can be aligned if used correct
> reserved bytes.
> 
>> An other solution is to introduce a'size' field to determine the size of
>> the structure:
> 
> I think, the resource table already contains config_len field which is
> size of the structure:
> 
> https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/remoteproc.h#L275


The resource table is the solution for the remoteproc virtio backend, 
The solution should be able to work with some other virtio backends.
In such case we can not rely on the resource table.

The resource table is a solution for the remoteproc virtio backend. 
However, the solution should also be able to work with other virtio 
backends.
In that case, it may not be possible to rely on the resource table.

Regards,
Arnaud

> 
>> struct virtio_rpmsg_config {
>>      __virtio16 version;
>>      __virtio16 size; /* size of the configuration space */
>>      /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>      __virtio32 txbuf_size;
>>      __virtio32 rxbuf_size;
>>      __u8 private[0]; /* customized config */
> 
> Do we need customized config? I think I should remove this comment from
> the original patch as well.
> 
> Thanks,
> Tanmay
> 
>> };
>>
>>>
>>> The rest looks good to me.
>>>
>>
>> Looks good to me too.
>>
>> Thanks,
>> Arnaud
>>
>>>> +    /* Put the customize config here */
>>>> +} __packed;
>>>> +
>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>> -- 
>>>> 2.34.1
>>>>
>>
> 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Mathieu Poirier]

On Wed, May 20, 2026 at 09:55:33AM -0500, Shah, Tanmay wrote:
> 
> 
> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
> > 
> > 
> > On 5/19/26 19:36, Mathieu Poirier wrote:
> >> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
> >>> ---
> >>>
> >>> Test done:
> >>>    - Verify this patch works with the existing firmware
> >>>    - Verify this patch works with the firmware that configures
> >>>      differt tx & rx buf size
> >>>
> >>> Changes in v2:
> >>>    - %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
> >>>    - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
> >>>
> >>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
> >>>   include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
> >>>   2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
> >>>    * @num_tx_buf:    total number of buffers for tx
> >>> - * @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_sbuf:    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_sbuf;
> >>>       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,7 @@ struct virtio_rpmsg_channel {
> >>>    * processor.
> >>>    */
> >>>   #define MAX_RPMSG_NUM_BUFS    (256)
> >>> -#define MAX_RPMSG_BUF_SIZE    (512)
> >>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
> >>>     /*
> >>>    * Local addresses are dynamically allocated on-demand.
> >>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >>>         /* either pick the next unused tx buffer */
> >>>       if (vrp->last_sbuf < vrp->num_tx_buf)
> >>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
> >>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
> >>>       /* or recycle a used one */
> >>>       else
> >>>           ret = virtqueue_get_buf(vrp->svq, &len);
> >>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>       int err = 0, i;
> >>>       size_t total_buf_space;
> >>>       bool notify;
> >>> +    u16 version;
> >>>         vrp = kzalloc_obj(*vrp);
> >>>       if (!vrp)
> >>> @@ -855,9 +856,41 @@ 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)) {
> >>> +        /* note: virtio_rpmsg_config is defined from remote view */
> >>> +        version = 0;
> >>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
> >>> +                 version, &version);
> >>> +        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);
> >>> +
> >>
> >> A check is also needed to make sure the version received from the
> >> resource table
> >> is '0'.
> 
> I think we should start with versaion = 1. So, can I check the version
> number for 1 ?

I've been thinking about that and I agree it should be something else than '0'.
Since we have a u16, I suggest to make bit 15-8 a magic number (surprise us!)
and bit 7-0 the actual version number.

> 
> >>
> >>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
> >>> --- /dev/null
> >>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> >>> @@ -0,0 +1,27 @@
> >>> +/* 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 {
> >>> +    __virtio16 version;
> >>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>> +    __virtio32 txbuf_size;
> >>> +    __virtio32 rxbuf_size;
> >>> +    __virtio32 reserved[14]; /*: Reserve for the future use */
> >>
> >> 66 byte for the configuratio space?  I'm puzzled about the
> >> reserved[14], how did
> >> you come up with that number?
> 
> I kept the reserved bytes from the original series as it is. The
> original series did not contain version field. With version I don't
> think we need reserved field at all. At best, if we want to append
> padding bytes, then I think __virtio16 reserved; makes sense. That will
> make the structure aligned to 4 byte.
> 
> > 
> > Is it useful to define the reserved field?
> 
> I think reserved field is only useful to keep the structure size aligned
> to 4 bytes.
> 
> > The version should allow us to determine the content.
> 
> Correct, but I think the structure can be aligned if used correct
> reserved bytes.
> 
> > An other solution is to introduce a'size' field to determine the size of
> > the structure:
> 
> I think, the resource table already contains config_len field which is
> size of the structure:
> 
> https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/remoteproc.h#L275
> 
> > struct virtio_rpmsg_config {
> >     __virtio16 version;
> >     __virtio16 size; /* size of the configuration space */
> >     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >     __virtio32 txbuf_size;
> >     __virtio32 rxbuf_size;
> >     __u8 private[0]; /* customized config */
> 
> Do we need customized config? I think I should remove this comment from
> the original patch as well.
> 
> Thanks,
> Tanmay
> 
> > };
> > 
> >>
> >> The rest looks good to me.
> >>
> > 
> > Looks good to me too.
> > 
> > Thanks,
> > Arnaud
> > 
> >>> +    /* Put the customize config here */
> >>> +} __packed;
> >>> +
> >>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> >>> -- 
> >>> 2.34.1
> >>>
> > 
> 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 5/20/2026 12:51 PM, Mathieu Poirier wrote:
> On Wed, May 20, 2026 at 09:55:33AM -0500, Shah, Tanmay wrote:
>>
>>
>> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 5/19/26 19:36, Mathieu Poirier wrote:
>>>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
>>>>> ---
>>>>>
>>>>> Test done:
>>>>>    - Verify this patch works with the existing firmware
>>>>>    - Verify this patch works with the firmware that configures
>>>>>      differt tx & rx buf size
>>>>>
>>>>> Changes in v2:
>>>>>    - %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
>>>>>    - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
>>>>>
>>>>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
>>>>>   include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>>>>>   2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>>>>>    * @num_tx_buf:    total number of buffers for tx
>>>>> - * @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_sbuf:    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_sbuf;
>>>>>       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,7 @@ struct virtio_rpmsg_channel {
>>>>>    * processor.
>>>>>    */
>>>>>   #define MAX_RPMSG_NUM_BUFS    (256)
>>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>>>>     /*
>>>>>    * Local addresses are dynamically allocated on-demand.
>>>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>>>         /* either pick the next unused tx buffer */
>>>>>       if (vrp->last_sbuf < vrp->num_tx_buf)
>>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>>>>>       /* or recycle a used one */
>>>>>       else
>>>>>           ret = virtqueue_get_buf(vrp->svq, &len);
>>>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>       int err = 0, i;
>>>>>       size_t total_buf_space;
>>>>>       bool notify;
>>>>> +    u16 version;
>>>>>         vrp = kzalloc_obj(*vrp);
>>>>>       if (!vrp)
>>>>> @@ -855,9 +856,41 @@ 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)) {
>>>>> +        /* note: virtio_rpmsg_config is defined from remote view */
>>>>> +        version = 0;
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 version, &version);
>>>>> +        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);
>>>>> +
>>>>
>>>> A check is also needed to make sure the version received from the
>>>> resource table
>>>> is '0'.
>>
>> I think we should start with versaion = 1. So, can I check the version
>> number for 1 ?
> 
> I've been thinking about that and I agree it should be something else than '0'.
> Since we have a u16, I suggest to make bit 15-8 a magic number (surprise us!)
> and bit 7-0 the actual version number.
> 

Hi Mathieu,

For xlnx platform driver, one magic number is already used for the
resource table metadata structure which is used to retrieve the resource
table. We can use magic number for the virtio config strucutre, but
after that we should limit the introduction of the magic numbers to any
future structures. We don't want too many magic numbers.

So I wanted input on this: Do we want to use magic number for 'version'
field of this structure or we want to keep use of the 'magic number'
reserved for any future use case? If we use it now, the I prefer to not
introduce in the future, as there will be too many magic numbers at that
point.

Thanks,
Tanmay

>>
>>>>
>>>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
>>>>> --- /dev/null
>>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>>> @@ -0,0 +1,27 @@
>>>>> +/* 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 {
>>>>> +    __virtio16 version;
>>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>>> +    __virtio32 txbuf_size;
>>>>> +    __virtio32 rxbuf_size;
>>>>> +    __virtio32 reserved[14]; /*: Reserve for the future use */
>>>>
>>>> 66 byte for the configuratio space?  I'm puzzled about the
>>>> reserved[14], how did
>>>> you come up with that number?
>>
>> I kept the reserved bytes from the original series as it is. The
>> original series did not contain version field. With version I don't
>> think we need reserved field at all. At best, if we want to append
>> padding bytes, then I think __virtio16 reserved; makes sense. That will
>> make the structure aligned to 4 byte.
>>
>>>
>>> Is it useful to define the reserved field?
>>
>> I think reserved field is only useful to keep the structure size aligned
>> to 4 bytes.
>>
>>> The version should allow us to determine the content.
>>
>> Correct, but I think the structure can be aligned if used correct
>> reserved bytes.
>>
>>> An other solution is to introduce a'size' field to determine the size of
>>> the structure:
>>
>> I think, the resource table already contains config_len field which is
>> size of the structure:
>>
>> https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/remoteproc.h#L275
>>
>>> struct virtio_rpmsg_config {
>>>     __virtio16 version;
>>>     __virtio16 size; /* size of the configuration space */
>>>     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>     __virtio32 txbuf_size;
>>>     __virtio32 rxbuf_size;
>>>     __u8 private[0]; /* customized config */
>>
>> Do we need customized config? I think I should remove this comment from
>> the original patch as well.
>>
>> Thanks,
>> Tanmay
>>
>>> };
>>>
>>>>
>>>> The rest looks good to me.
>>>>
>>>
>>> Looks good to me too.
>>>
>>> Thanks,
>>> Arnaud
>>>
>>>>> +    /* Put the customize config here */
>>>>> +} __packed;
>>>>> +
>>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>
>>

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 5/20/2026 10:43 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 5/20/26 16:55, Shah, Tanmay wrote:
>>
>>
>> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 5/19/26 19:36, Mathieu Poirier wrote:
>>>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
>>>>> ---
>>>>>
>>>>> Test done:
>>>>>     - Verify this patch works with the existing firmware
>>>>>     - Verify this patch works with the firmware that configures
>>>>>       differt tx & rx buf size
>>>>>
>>>>> Changes in v2:
>>>>>     - %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
>>>>>     - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
>>>>>
>>>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 70 +++++++++++++++++++++
>>>>> +--------
>>>>>    include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>>>>>    2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>>>>>     * @num_tx_buf:    total number of buffers for tx
>>>>> - * @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_sbuf:    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_sbuf;
>>>>>        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,7 @@ struct virtio_rpmsg_channel {
>>>>>     * processor.
>>>>>     */
>>>>>    #define MAX_RPMSG_NUM_BUFS    (256)
>>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>>>>      /*
>>>>>     * Local addresses are dynamically allocated on-demand.
>>>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info
>>>>> *vrp)
>>>>>          /* either pick the next unused tx buffer */
>>>>>        if (vrp->last_sbuf < vrp->num_tx_buf)
>>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>>>>>        /* or recycle a used one */
>>>>>        else
>>>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>        int err = 0, i;
>>>>>        size_t total_buf_space;
>>>>>        bool notify;
>>>>> +    u16 version;
>>>>>          vrp = kzalloc_obj(*vrp);
>>>>>        if (!vrp)
>>>>> @@ -855,9 +856,41 @@ 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)) {
>>>>> +        /* note: virtio_rpmsg_config is defined from remote view */
>>>>> +        version = 0;
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 version, &version);
>>>>> +        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);
>>>>> +
>>>>
>>>> A check is also needed to make sure the version received from the
>>>> resource table
>>>> is '0'.
>>
>> I think we should start with versaion = 1. So, can I check the version
>> number for 1 ?
>>
>>>>
>>>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
>>>>> --- /dev/null
>>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>>> @@ -0,0 +1,27 @@
>>>>> +/* 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 {
>>>>> +    __virtio16 version;
>>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>>> +    __virtio32 txbuf_size;
>>>>> +    __virtio32 rxbuf_size;
>>>>> +    __virtio32 reserved[14]; /* Reserve for the future use */
>>>>
>>>> 66 byte for the configuratio space?  I'm puzzled about the
>>>> reserved[14], how did
>>>> you come up with that number?
>>
>> I kept the reserved bytes from the original series as it is. The
>> original series did not contain version field. With version I don't
>> think we need reserved field at all. At best, if we want to append
>> padding bytes, then I think __virtio16 reserved; makes sense. That will
>> make the structure aligned to 4 byte.
>>
>>>
>>> Is it useful to define the reserved field?
>>
>> I think reserved field is only useful to keep the structure size aligned
>> to 4 bytes.
>>
>>> The version should allow us to determine the content.
>>
>> Correct, but I think the structure can be aligned if used correct
>> reserved bytes.
>>
>>> An other solution is to introduce a'size' field to determine the size of
>>> the structure:
>>
>> I think, the resource table already contains config_len field which is
>> size of the structure:
>>
>> https://github.com/torvalds/linux/
>> blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/
>> remoteproc.h#L275
> 
> 
> The resource table is the solution for the remoteproc virtio backend,
> The solution should be able to work with some other virtio backends.
> In such case we can not rely on the resource table.
> 

For now we have only the remoteproc virtio backend. If we ever introduce
new backend, then can we update this structure with new 'version' num
and include size field at that time? Since, now there is no real use of it.

> The resource table is a solution for the remoteproc virtio backend.
> However, the solution should also be able to work with other virtio
> backends.
> In that case, it may not be possible to rely on the resource table.
> 

Another concern is, do we need the size of the structure in the
first-place even to retrieve the 'size' field from the structure or
verify the integrity of the structure?

For example:
https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/drivers/remoteproc/remoteproc_virtio.c#L301


Here to verify the integrity of the resource table, we are using virtio
config space size.

If we do then it's better to provide the 'size' field some other way
before parsing of the rpmsg config space in any virtio backend. Like in
the remoteproc virtio backend it's provided via the resource table.

Let me know if I am overthinking :-) and we want the 'size' field in the
config space. I am just trying to make it as minimal as possible.

Thanks,
Tanmay

> Regards,
> Arnaud
> 
>>
>>> struct virtio_rpmsg_config {
>>>      __virtio16 version;
>>>      __virtio16 size; /* size of the configuration space */
>>>      /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>      __virtio32 txbuf_size;
>>>      __virtio32 rxbuf_size;
>>>      __u8 private[0]; /* customized config */
>>
>> Do we need customized config? I think I should remove this comment from
>> the original patch as well.
>>
>> Thanks,
>> Tanmay
>>
>>> };
>>>
>>>>
>>>> The rest looks good to me.
>>>>
>>>
>>> Looks good to me too.
>>>
>>> Thanks,
>>> Arnaud
>>>
>>>>> +    /* Put the customize config here */
>>>>> +} __packed;
>>>>> +
>>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>
>>
> 

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Mathieu Poirier]

On Thu, 21 May 2026 at 07:23, Shah, Tanmay <tanmays@amd.com> wrote:
>
>
>
> On 5/20/2026 12:51 PM, Mathieu Poirier wrote:
> > On Wed, May 20, 2026 at 09:55:33AM -0500, Shah, Tanmay wrote:
> >>
> >>
> >> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
> >>>
> >>>
> >>> On 5/19/26 19:36, Mathieu Poirier wrote:
> >>>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
> >>>>> ---
> >>>>>
> >>>>> Test done:
> >>>>>    - Verify this patch works with the existing firmware
> >>>>>    - Verify this patch works with the firmware that configures
> >>>>>      differt tx & rx buf size
> >>>>>
> >>>>> Changes in v2:
> >>>>>    - %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
> >>>>>    - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
> >>>>>
> >>>>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 70 ++++++++++++++++++++++--------
> >>>>>   include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
> >>>>>   2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
> >>>>>    * @num_tx_buf:    total number of buffers for tx
> >>>>> - * @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_sbuf:    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_sbuf;
> >>>>>       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,7 @@ struct virtio_rpmsg_channel {
> >>>>>    * processor.
> >>>>>    */
> >>>>>   #define MAX_RPMSG_NUM_BUFS    (256)
> >>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
> >>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
> >>>>>     /*
> >>>>>    * Local addresses are dynamically allocated on-demand.
> >>>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >>>>>         /* either pick the next unused tx buffer */
> >>>>>       if (vrp->last_sbuf < vrp->num_tx_buf)
> >>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
> >>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
> >>>>>       /* or recycle a used one */
> >>>>>       else
> >>>>>           ret = virtqueue_get_buf(vrp->svq, &len);
> >>>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>       int err = 0, i;
> >>>>>       size_t total_buf_space;
> >>>>>       bool notify;
> >>>>> +    u16 version;
> >>>>>         vrp = kzalloc_obj(*vrp);
> >>>>>       if (!vrp)
> >>>>> @@ -855,9 +856,41 @@ 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)) {
> >>>>> +        /* note: virtio_rpmsg_config is defined from remote view */
> >>>>> +        version = 0;
> >>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>>> +                 version, &version);
> >>>>> +        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);
> >>>>> +
> >>>>
> >>>> A check is also needed to make sure the version received from the
> >>>> resource table
> >>>> is '0'.
> >>
> >> I think we should start with versaion = 1. So, can I check the version
> >> number for 1 ?
> >
> > I've been thinking about that and I agree it should be something else than '0'.
> > Since we have a u16, I suggest to make bit 15-8 a magic number (surprise us!)
> > and bit 7-0 the actual version number.
> >
>
> Hi Mathieu,
>
> For xlnx platform driver, one magic number is already used for the
> resource table metadata structure which is used to retrieve the resource
> table. We can use magic number for the virtio config strucutre, but
> after that we should limit the introduction of the magic numbers to any
> future structures. We don't want too many magic numbers.


I looked at several device configuration format in the OASIS
specification and none have a magic number, so just forget I ever
mentioned it.

>
>
> So I wanted input on this: Do we want to use magic number for 'version'
> field of this structure or we want to keep use of the 'magic number'
> reserved for any future use case? If we use it now, the I prefer to not
> introduce in the future, as there will be too many magic numbers at that
> point.
>
> Thanks,
> Tanmay
>
> >>
> >>>>
> >>>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
> >>>>> --- /dev/null
> >>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> >>>>> @@ -0,0 +1,27 @@
> >>>>> +/* 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 {
> >>>>> +    __virtio16 version;
> >>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>>> +    __virtio32 txbuf_size;
> >>>>> +    __virtio32 rxbuf_size;
> >>>>> +    __virtio32 reserved[14]; /*: Reserve for the future use */
> >>>>
> >>>> 66 byte for the configuratio space?  I'm puzzled about the
> >>>> reserved[14], how did
> >>>> you come up with that number?
> >>
> >> I kept the reserved bytes from the original series as it is. The
> >> original series did not contain version field. With version I don't
> >> think we need reserved field at all. At best, if we want to append
> >> padding bytes, then I think __virtio16 reserved; makes sense. That will
> >> make the structure aligned to 4 byte.
> >>
> >>>
> >>> Is it useful to define the reserved field?
> >>
> >> I think reserved field is only useful to keep the structure size aligned
> >> to 4 bytes.
> >>
> >>> The version should allow us to determine the content.
> >>
> >> Correct, but I think the structure can be aligned if used correct
> >> reserved bytes.
> >>
> >>> An other solution is to introduce a'size' field to determine the size of
> >>> the structure:
> >>
> >> I think, the resource table already contains config_len field which is
> >> size of the structure:
> >>
> >> https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/remoteproc.h#L275
> >>
> >>> struct virtio_rpmsg_config {
> >>>     __virtio16 version;
> >>>     __virtio16 size; /* size of the configuration space */
> >>>     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>     __virtio32 txbuf_size;
> >>>     __virtio32 rxbuf_size;
> >>>     __u8 private[0]; /* customized config */
> >>
> >> Do we need customized config? I think I should remove this comment from
> >> the original patch as well.
> >>
> >> Thanks,
> >> Tanmay
> >>
> >>> };
> >>>
> >>>>
> >>>> The rest looks good to me.
> >>>>
> >>>
> >>> Looks good to me too.
> >>>
> >>> Thanks,
> >>> Arnaud
> >>>
> >>>>> +    /* Put the customize config here */
> >>>>> +} __packed;
> >>>>> +
> >>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> >>>>> --
> >>>>> 2.34.1
> >>>>>
> >>>
> >>
>

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Mathieu Poirier]

On Thu, 21 May 2026 at 09:08, Shah, Tanmay <tanmays@amd.com> wrote:
>
>
>
> On 5/20/2026 10:43 AM, Arnaud POULIQUEN wrote:
> >
> >
> > On 5/20/26 16:55, Shah, Tanmay wrote:
> >>
> >>
> >> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
> >>>
> >>>
> >>> On 5/19/26 19:36, Mathieu Poirier wrote:
> >>>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
> >>>>> ---
> >>>>>
> >>>>> Test done:
> >>>>>     - Verify this patch works with the existing firmware
> >>>>>     - Verify this patch works with the firmware that configures
> >>>>>       differt tx & rx buf size
> >>>>>
> >>>>> Changes in v2:
> >>>>>     - %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
> >>>>>     - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
> >>>>>
> >>>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 70 +++++++++++++++++++++
> >>>>> +--------
> >>>>>    include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
> >>>>>    2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
> >>>>>     * @num_tx_buf:    total number of buffers for tx
> >>>>> - * @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_sbuf:    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_sbuf;
> >>>>>        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,7 @@ struct virtio_rpmsg_channel {
> >>>>>     * processor.
> >>>>>     */
> >>>>>    #define MAX_RPMSG_NUM_BUFS    (256)
> >>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
> >>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
> >>>>>      /*
> >>>>>     * Local addresses are dynamically allocated on-demand.
> >>>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info
> >>>>> *vrp)
> >>>>>          /* either pick the next unused tx buffer */
> >>>>>        if (vrp->last_sbuf < vrp->num_tx_buf)
> >>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
> >>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
> >>>>>        /* or recycle a used one */
> >>>>>        else
> >>>>>            ret = virtqueue_get_buf(vrp->svq, &len);
> >>>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>        int err = 0, i;
> >>>>>        size_t total_buf_space;
> >>>>>        bool notify;
> >>>>> +    u16 version;
> >>>>>          vrp = kzalloc_obj(*vrp);
> >>>>>        if (!vrp)
> >>>>> @@ -855,9 +856,41 @@ 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)) {
> >>>>> +        /* note: virtio_rpmsg_config is defined from remote view */
> >>>>> +        version = 0;
> >>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>>> +                 version, &version);
> >>>>> +        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);
> >>>>> +
> >>>>
> >>>> A check is also needed to make sure the version received from the
> >>>> resource table
> >>>> is '0'.
> >>
> >> I think we should start with versaion = 1. So, can I check the version
> >> number for 1 ?
> >>
> >>>>
> >>>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
> >>>>> --- /dev/null
> >>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> >>>>> @@ -0,0 +1,27 @@
> >>>>> +/* 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 {
> >>>>> +    __virtio16 version;
> >>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>>> +    __virtio32 txbuf_size;
> >>>>> +    __virtio32 rxbuf_size;
> >>>>> +    __virtio32 reserved[14]; /* Reserve for the future use */
> >>>>
> >>>> 66 byte for the configuratio space?  I'm puzzled about the
> >>>> reserved[14], how did
> >>>> you come up with that number?
> >>
> >> I kept the reserved bytes from the original series as it is. The
> >> original series did not contain version field. With version I don't
> >> think we need reserved field at all. At best, if we want to append
> >> padding bytes, then I think __virtio16 reserved; makes sense. That will
> >> make the structure aligned to 4 byte.
> >>
> >>>
> >>> Is it useful to define the reserved field?
> >>
> >> I think reserved field is only useful to keep the structure size aligned
> >> to 4 bytes.
> >>
> >>> The version should allow us to determine the content.
> >>
> >> Correct, but I think the structure can be aligned if used correct
> >> reserved bytes.
> >>
> >>> An other solution is to introduce a'size' field to determine the size of
> >>> the structure:
> >>
> >> I think, the resource table already contains config_len field which is
> >> size of the structure:
> >>
> >> https://github.com/torvalds/linux/
> >> blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/
> >> remoteproc.h#L275
> >
> >
> > The resource table is the solution for the remoteproc virtio backend,
> > The solution should be able to work with some other virtio backends.
> > In such case we can not rely on the resource table.
> >
>
> For now we have only the remoteproc virtio backend. If we ever introduce
> new backend, then can we update this structure with new 'version' num
> and include size field at that time? Since, now there is no real use of it.
>
> > The resource table is a solution for the remoteproc virtio backend.
> > However, the solution should also be able to work with other virtio
> > backends.
> > In that case, it may not be possible to rely on the resource table.
> >
>
> Another concern is, do we need the size of the structure in the
> first-place even to retrieve the 'size' field from the structure or
> verify the integrity of the structure?
>
> For example:
> https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/drivers/remoteproc/remoteproc_virtio.c#L301
>
>
> Here to verify the integrity of the resource table, we are using virtio
> config space size.
>
> If we do then it's better to provide the 'size' field some other way
> before parsing of the rpmsg config space in any virtio backend. Like in
> the remoteproc virtio backend it's provided via the resource table.
>
> Let me know if I am overthinking :-) and we want the 'size' field in the
> config space. I am just trying to make it as minimal as possible.

I think it is better to have a 'size' field in the structure.

>
> Thanks,
> Tanmay
>
> > Regards,
> > Arnaud
> >
> >>
> >>> struct virtio_rpmsg_config {
> >>>      __virtio16 version;
> >>>      __virtio16 size; /* size of the configuration space */
> >>>      /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>      __virtio32 txbuf_size;
> >>>      __virtio32 rxbuf_size;
> >>>      __u8 private[0]; /* customized config */
> >>
> >> Do we need customized config? I think I should remove this comment from
> >> the original patch as well.
> >>
> >> Thanks,
> >> Tanmay
> >>
> >>> };
> >>>
> >>>>
> >>>> The rest looks good to me.
> >>>>
> >>>
> >>> Looks good to me too.
> >>>
> >>> Thanks,
> >>> Arnaud
> >>>
> >>>>> +    /* Put the customize config here */
> >>>>> +} __packed;
> >>>>> +
> >>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> >>>>> --
> >>>>> 2.34.1
> >>>>>
> >>>
> >>
> >
>

Re: [PATCH v2 2/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 5/21/2026 11:58 AM, Mathieu Poirier wrote:
> On Thu, 21 May 2026 at 09:08, Shah, Tanmay <tanmays@amd.com> wrote:
>>
>>
>>
>> On 5/20/2026 10:43 AM, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 5/20/26 16:55, Shah, Tanmay wrote:
>>>>
>>>>
>>>> On 5/20/2026 2:44 AM, Arnaud POULIQUEN wrote:
>>>>>
>>>>>
>>>>> On 5/19/26 19:36, Mathieu Poirier wrote:
>>>>>> On Wed, Apr 29, 2026 at 09:10:52AM -0700, 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>
>>>>>>> ---
>>>>>>>
>>>>>>> Test done:
>>>>>>>     - Verify this patch works with the existing firmware
>>>>>>>     - Verify this patch works with the firmware that configures
>>>>>>>       differt tx & rx buf size
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>>     - %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
>>>>>>>     - move linux/virtio_rpmsg.h to linux/rpmsg/virtio_rpmsg.h
>>>>>>>
>>>>>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 70 +++++++++++++++++++++
>>>>>>> +--------
>>>>>>>    include/linux/rpmsg/virtio_rpmsg.h | 27 ++++++++++++
>>>>>>>    2 files changed, 79 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 e59d8cf9b975..8116d94413cc 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 buffers for rx
>>>>>>>     * @num_tx_buf:    total number of buffers for tx
>>>>>>> - * @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_sbuf:    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_sbuf;
>>>>>>>        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,7 @@ struct virtio_rpmsg_channel {
>>>>>>>     * processor.
>>>>>>>     */
>>>>>>>    #define MAX_RPMSG_NUM_BUFS    (256)
>>>>>>> -#define MAX_RPMSG_BUF_SIZE    (512)
>>>>>>> +#define DEFAULT_RPMSG_BUF_SIZE    (512)
>>>>>>>      /*
>>>>>>>     * Local addresses are dynamically allocated on-demand.
>>>>>>> @@ -444,7 +444,7 @@ static void *get_a_tx_buf(struct virtproc_info
>>>>>>> *vrp)
>>>>>>>          /* either pick the next unused tx buffer */
>>>>>>>        if (vrp->last_sbuf < vrp->num_tx_buf)
>>>>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>>>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_sbuf++;
>>>>>>>        /* or recycle a used one */
>>>>>>>        else
>>>>>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>>>>>> @@ -514,7 +514,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 +647,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 +673,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 +706,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 +824,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>>        int err = 0, i;
>>>>>>>        size_t total_buf_space;
>>>>>>>        bool notify;
>>>>>>> +    u16 version;
>>>>>>>          vrp = kzalloc_obj(*vrp);
>>>>>>>        if (!vrp)
>>>>>>> @@ -855,9 +856,41 @@ 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)) {
>>>>>>> +        /* note: virtio_rpmsg_config is defined from remote view */
>>>>>>> +        version = 0;
>>>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>>>> +                 version, &version);
>>>>>>> +        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);
>>>>>>> +
>>>>>>
>>>>>> A check is also needed to make sure the version received from the
>>>>>> resource table
>>>>>> is '0'.
>>>>
>>>> I think we should start with versaion = 1. So, can I check the version
>>>> number for 1 ?
>>>>
>>>>>>
>>>>>>> +        /* 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 = %d, tx buf sz = %d\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 +908,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 +998,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 +1025,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..285918be68b9
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>>>>> @@ -0,0 +1,27 @@
>>>>>>> +/* 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 {
>>>>>>> +    __virtio16 version;
>>>>>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>>>>> +    __virtio32 txbuf_size;
>>>>>>> +    __virtio32 rxbuf_size;
>>>>>>> +    __virtio32 reserved[14]; /* Reserve for the future use */
>>>>>>
>>>>>> 66 byte for the configuratio space?  I'm puzzled about the
>>>>>> reserved[14], how did
>>>>>> you come up with that number?
>>>>
>>>> I kept the reserved bytes from the original series as it is. The
>>>> original series did not contain version field. With version I don't
>>>> think we need reserved field at all. At best, if we want to append
>>>> padding bytes, then I think __virtio16 reserved; makes sense. That will
>>>> make the structure aligned to 4 byte.
>>>>
>>>>>
>>>>> Is it useful to define the reserved field?
>>>>
>>>> I think reserved field is only useful to keep the structure size aligned
>>>> to 4 bytes.
>>>>
>>>>> The version should allow us to determine the content.
>>>>
>>>> Correct, but I think the structure can be aligned if used correct
>>>> reserved bytes.
>>>>
>>>>> An other solution is to introduce a'size' field to determine the size of
>>>>> the structure:
>>>>
>>>> I think, the resource table already contains config_len field which is
>>>> size of the structure:
>>>>
>>>> https://github.com/torvalds/linux/
>>>> blob/27fa82620cbaa89a7fc11ac3057701d598813e87/include/linux/
>>>> remoteproc.h#L275
>>>
>>>
>>> The resource table is the solution for the remoteproc virtio backend,
>>> The solution should be able to work with some other virtio backends.
>>> In such case we can not rely on the resource table.
>>>
>>
>> For now we have only the remoteproc virtio backend. If we ever introduce
>> new backend, then can we update this structure with new 'version' num
>> and include size field at that time? Since, now there is no real use of it.
>>
>>> The resource table is a solution for the remoteproc virtio backend.
>>> However, the solution should also be able to work with other virtio
>>> backends.
>>> In that case, it may not be possible to rely on the resource table.
>>>
>>
>> Another concern is, do we need the size of the structure in the
>> first-place even to retrieve the 'size' field from the structure or
>> verify the integrity of the structure?
>>
>> For example:
>> https://github.com/torvalds/linux/blob/27fa82620cbaa89a7fc11ac3057701d598813e87/drivers/remoteproc/remoteproc_virtio.c#L301
>>
>>
>> Here to verify the integrity of the resource table, we are using virtio
>> config space size.
>>
>> If we do then it's better to provide the 'size' field some other way
>> before parsing of the rpmsg config space in any virtio backend. Like in
>> the remoteproc virtio backend it's provided via the resource table.
>>
>> Let me know if I am overthinking :-) and we want the 'size' field in the
>> config space. I am just trying to make it as minimal as possible.
> 
> I think it is better to have a 'size' field in the structure.
> 

Ack. I will work on new revision as per our conversation here, and send v3.

Thanks.

>>
>> Thanks,
>> Tanmay
>>
>>> Regards,
>>> Arnaud
>>>
>>>>
>>>>> struct virtio_rpmsg_config {
>>>>>      __virtio16 version;
>>>>>      __virtio16 size; /* size of the configuration space */
>>>>>      /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>>>      __virtio32 txbuf_size;
>>>>>      __virtio32 rxbuf_size;
>>>>>      __u8 private[0]; /* customized config */
>>>>
>>>> Do we need customized config? I think I should remove this comment from
>>>> the original patch as well.
>>>>
>>>> Thanks,
>>>> Tanmay
>>>>
>>>>> };
>>>>>
>>>>>>
>>>>>> The rest looks good to me.
>>>>>>
>>>>>
>>>>> Looks good to me too.
>>>>>
>>>>> Thanks,
>>>>> Arnaud
>>>>>
>>>>>>> +    /* Put the customize config here */
>>>>>>> +} __packed;
>>>>>>> +
>>>>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>>>>> --
>>>>>>> 2.34.1
>>>>>>>
>>>>>
>>>>
>>>
>>