[PATCH v4 0/5] Enhance RPMsg buffer management

[PATCH v4 0/5] Enhance RPMsg buffer management

[Tanmay Shah]

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

Changes in v4:
  - Introduce new patch to modify rpmsg.rst documentation
  - check version is always 1.
  - check size field is same as size of struct virtio_rpmsg_config
  - introduce alignment field
  - check alignment field is power of 2
  - check tx and rx buf size is aligned with alignment passed in the
    structure
  - check msg size is < MTU size

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

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

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

 Documentation/staging/rpmsg.rst     |  19 +++
 drivers/rpmsg/virtio_rpmsg_bus.c    | 185 +++++++++++++++++++++-------
 include/linux/rpmsg/virtio_rpmsg.h  |  50 ++++++++
 samples/rpmsg/rpmsg_client_sample.c |  20 ++-
 4 files changed, 226 insertions(+), 48 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h


base-commit: 85842b61f64cac93d28e129d35193e329d463fd1
-- 
2.34.1

[PATCH v4 1/5] rpmsg: virtio_rpmsg_bus: rename rbufs and sbufs

[Tanmay Shah]

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

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

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

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

[Tanmay Shah]

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

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

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

[PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Tanmay Shah]

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

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

Changes in v4: squash to virtio rpmsg config patch
  - Introduce new patch to modify rpmsg.rst documentation
  - check version is always 1.
  - check size field is same as size of struct virtio_rpmsg_config
  - introduce alignment field
  - check alignment field is power of 2
  - check tx and rx buf size is aligned with alignment passed in the
    structure

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

 drivers/rpmsg/virtio_rpmsg_bus.c   | 129 ++++++++++++++++++++++++-----
 include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
 2 files changed, 160 insertions(+), 19 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 99df1ae07055..a59925f870a4 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -15,11 +15,13 @@
 #include <linux/idr.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #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 +41,8 @@
  * @tx_bufs:	kernel address of tx buffers
  * @num_rx_buf: total number of rx buffers
  * @num_tx_buf: total number of tx buffers
- * @buf_size:   size of one rx or tx buffer
+ * @rx_buf_size: size of one rx buffer
+ * @tx_buf_size: size of one tx buffer
  * @last_tx_buf: index of last tx buffer used
  * @bufs_dma:	dma base addr of the buffers
  * @tx_lock:	protects svq and tx_bufs, to allow concurrent senders.
@@ -59,7 +62,8 @@ struct virtproc_info {
 	void *rx_bufs, *tx_bufs;
 	unsigned int num_rx_buf;
 	unsigned int num_tx_buf;
-	unsigned int buf_size;
+	unsigned int rx_buf_size;
+	unsigned int tx_buf_size;
 	int last_tx_buf;
 	dma_addr_t bufs_dma;
 	struct mutex tx_lock;
@@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 
 	/* either pick the next unused tx buffer */
 	if (vrp->last_tx_buf < vrp->num_tx_buf)
-		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
+		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -514,7 +515,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 +648,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 +674,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 +707,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);
@@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	struct virtproc_info *vrp;
 	struct virtio_rpmsg_channel *vch = NULL;
 	struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
+	u16 rpmsg_buf_align = 0;
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
 	bool notify;
+	u8 version;
+	u16 size;
 
 	vrp = kzalloc_obj(*vrp);
 	if (!vrp)
@@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	else
 		vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
 
-	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
+	/*
+	 * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
+	 * size from virtio device config space from the resource table.
+	 * If the feature is not supported, then assign default buf size.
+	 */
+	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     version, &version);
+
+		/* for now we support only v1 */
+		if (version != RPMSG_VDEV_CONFIG_V1) {
+			dev_err(&vdev->dev,
+				"unsupported vdev config version %u\n", version);
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		/* size of the config space must match */
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     size, &size);
+		if (size != sizeof(struct virtio_rpmsg_config)) {
+			dev_err(&vdev->dev, "invalid size of vdev config %u\n",
+				size);
+			err = -EINVAL;
+			goto vqs_del;
+		}
 
-	total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp->buf_size;
+		/*
+		 * Optional alignment applied to each buffer size and to the TX
+		 * buffer base address (e.g. to align buffers on a cache line).
+		 * It must be a power of two; zero means no extra alignment.
+		 */
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     rpmsg_buf_align, &rpmsg_buf_align);
+		if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
+			dev_err(&vdev->dev,
+				"bad vdev config: rpmsg_buf_align %u is not a power of two\n",
+				rpmsg_buf_align);
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		/* note: tx and rx are defined from remote view */
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     txbuf_size, &vrp->rx_buf_size);
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     rxbuf_size, &vrp->tx_buf_size);
+
+		/* The buffers must hold at least the rpmsg header */
+		if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
+		    vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
+			dev_err(&vdev->dev,
+				"bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
+				vrp->rx_buf_size, vrp->tx_buf_size);
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		/*
+		 * The buffer size must be aligned to the provided alignment for
+		 * so that the start address of tx bufs can be aligned.
+		 */
+		if (rpmsg_buf_align &&
+		    (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
+		     !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
+			dev_err(&vdev->dev,
+				"bad vdev config: buf sizes (rx %u, tx %u) not aligned to %u\n",
+				vrp->rx_buf_size, vrp->tx_buf_size,
+				rpmsg_buf_align);
+			err = -EINVAL;
+			goto vqs_del;
+		}
+
+		dev_dbg(&vdev->dev,
+			"vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz = 0x%x\n",
+			version, rpmsg_buf_align, 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->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,
@@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	/* first part of the buffers is dedicated for RX */
 	vrp->rx_bufs = bufs_va;
 
-	/* and second part is dedicated for TX */
-	vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
+	/*
+	 * Here buf_va is aligned to a page. Also rx buf size is aligned with
+	 * cache line alignment provided by the firmware, so tx buf's start
+	 * address is guranteed to be aligned with the alignment provided by
+	 * the firmware.
+	 */
+	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 +1055,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 +1082,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..7e14da68fd17
--- /dev/null
+++ b/include/linux/rpmsg/virtio_rpmsg.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) Pinecone Inc. 2019
+ * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
+ * Copyright (C) Advanced Micro Devices, Inc. 2026
+ */
+
+#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 */
+
+/* Version of struct virtio_rpmsg_config understood by this driver */
+#define RPMSG_VDEV_CONFIG_V1	1
+
+/**
+ * struct virtio_rpmsg_config - config space for rpmsg virtio device
+ *
+ * @version:	version of this structure, currently %RPMSG_VDEV_CONFIG_V1.
+ * @reserved:	reserved for padding, must be zero.
+ * @size:	size of this structure in bytes.
+ * @rpmsg_buf_align:	required alignment in bytes for each buffer. Must be a
+ *		power of two so that both the buffer sizes and the TX buffer
+ *		base address can be aligned (e.g. to a cache line).
+ * @reserved1:	reserved for padding, must be zero. Keeps the following 32-bit
+ *		fields naturally aligned.
+ * @txbuf_size:	Tx buf size from remote's view. For Linux this is rx buf size.
+ * @rxbuf_size:	Rx buf size from remote's view. For Linux this is tx buf size.
+ *
+ * This is the configuration structure shared by the device and the driver,
+ * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are laid out so
+ * the structure is naturally 32-bit aligned.
+ */
+struct virtio_rpmsg_config {
+	u8 version;
+	u8 reserved;
+	__virtio16 size;
+	__virtio16 rpmsg_buf_align;
+	__virtio16 reserved1;
+	/* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
+	__virtio32 txbuf_size;
+	__virtio32 rxbuf_size;
+} __packed;
+
+#endif /* _LINUX_VIRTIO_RPMSG_H */
-- 
2.34.1

[PATCH v4 4/5] docs: rpmsg: add virtio config space details

[Tanmay Shah]

The virtio config space can provide single rpmsg buffer size in each
direction. Document details of the configurable buffer size via virtio
config space.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 Documentation/staging/rpmsg.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/staging/rpmsg.rst b/Documentation/staging/rpmsg.rst
index 42bac1149d9d..c06602e169a9 100644
--- a/Documentation/staging/rpmsg.rst
+++ b/Documentation/staging/rpmsg.rst
@@ -300,3 +300,22 @@ by the bus, and can then start sending messages to the remote service.
 
 The plan is also to add static creation of rpmsg channels via the virtio
 config space, but it's not implemented yet.
+
+Configurable buffer sizes
+=========================
+
+By default each rpmsg buffer is 512 bytes, and the same size is used for both
+the receive and transmit directions. Remote processors that need a different
+size (for example a larger MTU, or different RX and TX sizes) can advertise it
+through the virtio device config space by setting the VIRTIO_RPMSG_F_BUFSZ
+feature bit.
+
+When this feature is negotiated, the driver reads struct virtio_rpmsg_config
+from the config space. This structure provides the size of a single buffer for
+each direction (TX and RX), and the driver splits the total buffer space into
+TX and RX buffers accordingly. Both buffer sizes must be aligned to the
+rpmsg_buf_align field provided in the config space; a value of 0 means the
+sizes are assumed to be already aligned.
+
+If the feature is not negotiated, the default 512-byte buffers are used for
+both directions.
-- 
2.34.1

[PATCH v4 5/5] samples: rpmsg: add MTU size info

[Tanmay Shah]

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

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

Changes in v4:
  - check msg size is < MTU size

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

 samples/rpmsg/rpmsg_client_sample.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/samples/rpmsg/rpmsg_client_sample.c b/samples/rpmsg/rpmsg_client_sample.c
index ae5081662283..4c43436aadb6 100644
--- a/samples/rpmsg/rpmsg_client_sample.c
+++ b/samples/rpmsg/rpmsg_client_sample.c
@@ -52,6 +52,7 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
 {
 	int ret;
 	struct instance_data *idata;
+	ssize_t mtu, msg_len;
 
 	dev_info(&rpdev->dev, "new channel: 0x%x -> 0x%x!\n",
 					rpdev->src, rpdev->dst);
@@ -62,8 +63,25 @@ static int rpmsg_sample_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(&rpdev->dev, idata);
 
+	mtu = rpmsg_get_mtu(rpdev->ept);
+	if (mtu < 0) {
+		dev_warn(&rpdev->dev, "invalid rpmsg MTU size = %ld\n", mtu);
+		return mtu;
+	}
+
+	dev_info(&rpdev->dev, "rpmsg MTU size = %ld\n", mtu);
+
+	msg_len = strlen(MSG);
+	/* make sure our message fits in a single rpmsg buffer */
+	if (msg_len > mtu) {
+		dev_err(&rpdev->dev,
+			"message size %zu exceeds rpmsg MTU size %ld\n",
+			strlen(MSG), mtu);
+		return -EMSGSIZE;
+	}
+
 	/* send a message to our remote processor */
-	ret = rpmsg_send(rpdev->ept, MSG, strlen(MSG));
+	ret = rpmsg_send(rpdev->ept, MSG, msg_len);
 	if (ret) {
 		dev_err(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
 		return ret;
-- 
2.34.1

Re: [PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

Hi Tanmay,

On 6/15/26 22:20, Tanmay Shah wrote:
> 512 bytes isn't always suitable for all case, let firmware
> maker decide the best value from resource table.
> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> 
> Changes in v4: squash to virtio rpmsg config patch
>    - Introduce new patch to modify rpmsg.rst documentation
>    - check version is always 1.
>    - check size field is same as size of struct virtio_rpmsg_config
>    - introduce alignment field
>    - check alignment field is power of 2
>    - check tx and rx buf size is aligned with alignment passed in the
>      structure
> 
> Changes in v3:
>    - change version field from u16 to u8
>    - introduce size field in the rpmsg_virtio_config structure
>    - check version field is set to any non-zero value.
>    - check size field is not 0.
>    - Remove field for private config, as not needed for now.
>    - add documentation of rpmsg_virtio_config structure
> 
>   drivers/rpmsg/virtio_rpmsg_bus.c   | 129 ++++++++++++++++++++++++-----
>   include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
>   2 files changed, 160 insertions(+), 19 deletions(-)
>   create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 99df1ae07055..a59925f870a4 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -15,11 +15,13 @@
>   #include <linux/idr.h>
>   #include <linux/jiffies.h>
>   #include <linux/kernel.h>
> +#include <linux/log2.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #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 +41,8 @@
>    * @tx_bufs:	kernel address of tx buffers
>    * @num_rx_buf: total number of rx buffers
>    * @num_tx_buf: total number of tx buffers
> - * @buf_size:   size of one rx or tx buffer
> + * @rx_buf_size: size of one rx buffer
> + * @tx_buf_size: size of one tx buffer
>    * @last_tx_buf: index of last tx buffer used
>    * @bufs_dma:	dma base addr of the buffers
>    * @tx_lock:	protects svq and tx_bufs, to allow concurrent senders.
> @@ -59,7 +62,8 @@ struct virtproc_info {
>   	void *rx_bufs, *tx_bufs;
>   	unsigned int num_rx_buf;
>   	unsigned int num_tx_buf;
> -	unsigned int buf_size;
> +	unsigned int rx_buf_size;
> +	unsigned int tx_buf_size;
>   	int last_tx_buf;
>   	dma_addr_t bufs_dma;
>   	struct mutex tx_lock;
> @@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>   
>   	/* either pick the next unused tx buffer */
>   	if (vrp->last_tx_buf < vrp->num_tx_buf)
> -		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
> +		ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>   	/* or recycle a used one */
>   	else
>   		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -514,7 +515,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 +648,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 +674,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 +707,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);
> @@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device *vdev)
>   	struct virtproc_info *vrp;
>   	struct virtio_rpmsg_channel *vch = NULL;
>   	struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
> +	u16 rpmsg_buf_align = 0;
>   	void *bufs_va;
>   	int err = 0, i;
>   	size_t total_buf_space;
>   	bool notify;
> +	u8 version;
> +	u16 size;
>   
>   	vrp = kzalloc_obj(*vrp);
>   	if (!vrp)
> @@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device *vdev)
>   	else
>   		vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>   
> -	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +	/*
> +	 * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
> +	 * size from virtio device config space from the resource table.
> +	 * If the feature is not supported, then assign default buf size.
> +	 */
> +	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     version, &version);
> +
> +		/* for now we support only v1 */
> +		if (version != RPMSG_VDEV_CONFIG_V1) {
> +			dev_err(&vdev->dev,
> +				"unsupported vdev config version %u\n", version);
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		/* size of the config space must match */
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     size, &size);
> +		if (size != sizeof(struct virtio_rpmsg_config)) {
> +			dev_err(&vdev->dev, "invalid size of vdev config %u\n",
> +				size);
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
>   
> -	total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp->buf_size;
> +		/*
> +		 * Optional alignment applied to each buffer size and to the TX
> +		 * buffer base address (e.g. to align buffers on a cache line).
> +		 * It must be a power of two; zero means no extra alignment.
> +		 */
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     rpmsg_buf_align, &rpmsg_buf_align);
> +		if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
> +			dev_err(&vdev->dev,
> +				"bad vdev config: rpmsg_buf_align %u is not a power of two\n",
> +				rpmsg_buf_align);
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		/* note: tx and rx are defined from remote view */
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     txbuf_size, &vrp->rx_buf_size);
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     rxbuf_size, &vrp->tx_buf_size);
> +
> +		/* The buffers must hold at least the rpmsg header */
> +		if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
> +		    vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
> +			dev_err(&vdev->dev,
> +				"bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
> +				vrp->rx_buf_size, vrp->tx_buf_size);
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		/*
> +		 * The buffer size must be aligned to the provided alignment for
> +		 * so that the start address of tx bufs can be aligned.
> +		 */

'tx' to remove as  it also concerns Rx buffers


What about removing this check to manage alignment during buffer allocation?

For example, if the alignment is on a 64-bit address and the tx_buffer 
and rx_buffer sizes are 40 bytes, 48 bytes can be allocated in memory 
for each buffer, and the virtio descriptor can be filled with aligned 
addresses.

In other words, the rpmsg_buf_align field contains the alignment 
constraint from the remote processor. If the Linux kernel wants to 
impose another alignment constraint, it must test or update 
rpmsg_buf_align, but it must not impose alignment on the buffer size.


> +		if (rpmsg_buf_align &&
> +		    (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
> +		     !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
> +			dev_err(&vdev->dev,
> +				"bad vdev config: buf sizes (rx %u, tx %u) not aligned to %u\n",
> +				vrp->rx_buf_size, vrp->tx_buf_size,
> +				rpmsg_buf_align);
> +			err = -EINVAL;
> +			goto vqs_del;
> +		}
> +
> +		dev_dbg(&vdev->dev,
> +			"vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz = 0x%x\n",
> +			version, rpmsg_buf_align, 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->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,
> @@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>   	/* first part of the buffers is dedicated for RX */
>   	vrp->rx_bufs = bufs_va;
>   
> -	/* and second part is dedicated for TX */
> -	vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
> +	/*
> +	 * Here buf_va is aligned to a page. Also rx buf size is aligned with
> +	 * cache line alignment provided by the firmware, so tx buf's start
> +	 * address is guranteed to be aligned with the alignment provided by
> +	 * the firmware.
> +	 */
> +	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 +1055,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 +1082,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..7e14da68fd17
> --- /dev/null
> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) Pinecone Inc. 2019
> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> + * Copyright (C) Advanced Micro Devices, Inc. 2026
> + */
> +
> +#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 */
> +
> +/* Version of struct virtio_rpmsg_config understood by this driver */
> +#define RPMSG_VDEV_CONFIG_V1	1
> +
> +/**
> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
> + *
> + * @version:	version of this structure, currently %RPMSG_VDEV_CONFIG_V1.
> + * @reserved:	reserved for padding, must be zero.
> + * @size:	size of this structure in bytes.
> + * @rpmsg_buf_align:	required alignment in bytes for each buffer. Must be a
> + *		power of two so that both the buffer sizes and the TX buffer
> + *		base address can be aligned (e.g. to a cache line).
> + * @reserved1:	reserved for padding, must be zero. Keeps the following 32-bit
> + *		fields naturally aligned.
> + * @txbuf_size:	Tx buf size from remote's view. For Linux this is rx buf size.
> + * @rxbuf_size:	Rx buf size from remote's view. For Linux this is tx buf size.
> + *
> + * This is the configuration structure shared by the device and the driver,
> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are laid out so
> + * the structure is naturally 32-bit aligned.
> + */
> +struct virtio_rpmsg_config {
> +	u8 version;
> +	u8 reserved;

Why about defining the version type to u16 to avoid the reserved field?

> +	__virtio16 size;
> +	__virtio16 rpmsg_buf_align;
> +	__virtio16 reserved1;

Seems useless if __packed prevents the compiler from inserting extra padding
bytes between fields,

> +	/* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
> +	__virtio32 txbuf_size;
> +	__virtio32 rxbuf_size;
> +} __packed;

proposal

+struct virtio_rpmsg_config {
+	__virtio16 version;
+	__virtio16 size;
+	/* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
+	__virtio32 txbuf_size;
+	__virtio32 rxbuf_size;
+	__virtio16 rpmsg_buf_align;
+} __packed;
+

Regards,
Arnaud

> +
> +#endif /* _LINUX_VIRTIO_RPMSG_H */

Re: [PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 6/17/2026 4:15 AM, Arnaud POULIQUEN wrote:
> Hi Tanmay,
> 
> On 6/15/26 22:20, Tanmay Shah wrote:
>> 512 bytes isn't always suitable for all case, let firmware
>> maker decide the best value from resource table.
>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> Changes in v4: squash to virtio rpmsg config patch
>>    - Introduce new patch to modify rpmsg.rst documentation
>>    - check version is always 1.
>>    - check size field is same as size of struct virtio_rpmsg_config
>>    - introduce alignment field
>>    - check alignment field is power of 2
>>    - check tx and rx buf size is aligned with alignment passed in the
>>      structure
>>
>> Changes in v3:
>>    - change version field from u16 to u8
>>    - introduce size field in the rpmsg_virtio_config structure
>>    - check version field is set to any non-zero value.
>>    - check size field is not 0.
>>    - Remove field for private config, as not needed for now.
>>    - add documentation of rpmsg_virtio_config structure
>>
>>   drivers/rpmsg/virtio_rpmsg_bus.c   | 129 ++++++++++++++++++++++++-----
>>   include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
>>   2 files changed, 160 insertions(+), 19 deletions(-)
>>   create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>> virtio_rpmsg_bus.c
>> index 99df1ae07055..a59925f870a4 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -15,11 +15,13 @@
>>   #include <linux/idr.h>
>>   #include <linux/jiffies.h>
>>   #include <linux/kernel.h>
>> +#include <linux/log2.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>>   #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 +41,8 @@
>>    * @tx_bufs:    kernel address of tx buffers
>>    * @num_rx_buf: total number of rx buffers
>>    * @num_tx_buf: total number of tx buffers
>> - * @buf_size:   size of one rx or tx buffer
>> + * @rx_buf_size: size of one rx buffer
>> + * @tx_buf_size: size of one tx buffer
>>    * @last_tx_buf: index of last tx buffer used
>>    * @bufs_dma:    dma base addr of the buffers
>>    * @tx_lock:    protects svq and tx_bufs, to allow concurrent senders.
>> @@ -59,7 +62,8 @@ struct virtproc_info {
>>       void *rx_bufs, *tx_bufs;
>>       unsigned int num_rx_buf;
>>       unsigned int num_tx_buf;
>> -    unsigned int buf_size;
>> +    unsigned int rx_buf_size;
>> +    unsigned int tx_buf_size;
>>       int last_tx_buf;
>>       dma_addr_t bufs_dma;
>>       struct mutex tx_lock;
>> @@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>         /* either pick the next unused tx buffer */
>>       if (vrp->last_tx_buf < vrp->num_tx_buf)
>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>       /* or recycle a used one */
>>       else
>>           ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -514,7 +515,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 +648,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 +674,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 +707,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);
>> @@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>       struct virtproc_info *vrp;
>>       struct virtio_rpmsg_channel *vch = NULL;
>>       struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
>> +    u16 rpmsg_buf_align = 0;
>>       void *bufs_va;
>>       int err = 0, i;
>>       size_t total_buf_space;
>>       bool notify;
>> +    u8 version;
>> +    u16 size;
>>         vrp = kzalloc_obj(*vrp);
>>       if (!vrp)
>> @@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>       else
>>           vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>   -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>> +    /*
>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>> +     * size from virtio device config space from the resource table.
>> +     * If the feature is not supported, then assign default buf size.
>> +     */
>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 version, &version);
>> +
>> +        /* for now we support only v1 */
>> +        if (version != RPMSG_VDEV_CONFIG_V1) {
>> +            dev_err(&vdev->dev,
>> +                "unsupported vdev config version %u\n", version);
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        /* size of the config space must match */
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 size, &size);
>> +        if (size != sizeof(struct virtio_rpmsg_config)) {
>> +            dev_err(&vdev->dev, "invalid size of vdev config %u\n",
>> +                size);
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>>   -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>> >buf_size;
>> +        /*
>> +         * Optional alignment applied to each buffer size and to the TX
>> +         * buffer base address (e.g. to align buffers on a cache line).
>> +         * It must be a power of two; zero means no extra alignment.
>> +         */
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 rpmsg_buf_align, &rpmsg_buf_align);
>> +        if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
>> +            dev_err(&vdev->dev,
>> +                "bad vdev config: rpmsg_buf_align %u is not a power
>> of two\n",
>> +                rpmsg_buf_align);
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        /* note: tx and rx are defined from remote view */
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 txbuf_size, &vrp->rx_buf_size);
>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>> +                 rxbuf_size, &vrp->tx_buf_size);
>> +
>> +        /* The buffers must hold at least the rpmsg header */
>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>> +            dev_err(&vdev->dev,
>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        /*
>> +         * The buffer size must be aligned to the provided alignment for
>> +         * so that the start address of tx bufs can be aligned.
>> +         */
> 
> 'tx' to remove as  it also concerns Rx buffers
> 

Ack.

> 
> What about removing this check to manage alignment during buffer
> allocation?
> 
> For example, if the alignment is on a 64-bit address and the tx_buffer
> and rx_buffer sizes are 40 bytes, 48 bytes can be allocated in memory
> for each buffer, and the virtio descriptor can be filled with aligned
> addresses.
> 
> In other words, the rpmsg_buf_align field contains the alignment
> constraint from the remote processor. If the Linux kernel wants to
> impose another alignment constraint, it must test or update
> rpmsg_buf_align, but it must not impose alignment on the buffer size.
> 
> 

This part I don't understand. `rpmsg_buf_align` is alignment for only
single buffer size. The linux kernel is checking that single rx buf size
and tx buf size is aligned with `rpmsg_buf_align` as firmware has claimed.

For reference the openamp-system-reference PR:
https://github.com/OpenAMP/openamp-system-reference/pull/106/changes

	.vdev_config = {
		.version = 1,
		.reserved = 0,
		.size = (uint16_t)(sizeof(struct rpmsg_virtio_config) - sizeof(bool)),
		.alignment = RPMSG_BUF_ALIGN,
		.reserved1 = 0,
		/* Tx for host */
		.h2r_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
		/* Rx for host */
		.r2h_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
	},

IIUC, The linux kernel is not really supposed to modify
`rpmsg_buf_align`. It only uses it to check that firmware has assigned
correct size of single rx and tx buffer.


When the linux kernel uses dma_alloc_coherent() API it aligns total
buffer size with page size. That is different than single tx buf size
and single rx buf size. The total buf size alignment to page size is
irrelevant to `rpmsg_buf_align` field.

Please let me know if I am missing something or didn't understand your
comment. I prefer that `rpmsg_buf_align` should be only modified by the
firmware and not the linux kernel.



>> +        if (rpmsg_buf_align &&
>> +            (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
>> +             !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
>> +            dev_err(&vdev->dev,
>> +                "bad vdev config: buf sizes (rx %u, tx %u) not
>> aligned to %u\n",
>> +                vrp->rx_buf_size, vrp->tx_buf_size,
>> +                rpmsg_buf_align);
>> +            err = -EINVAL;
>> +            goto vqs_del;
>> +        }
>> +
>> +        dev_dbg(&vdev->dev,
>> +            "vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz =
>> 0x%x\n",
>> +            version, rpmsg_buf_align, 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->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,
>> @@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>       /* first part of the buffers is dedicated for RX */
>>       vrp->rx_bufs = bufs_va;
>>   -    /* and second part is dedicated for TX */
>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>> +    /*
>> +     * Here buf_va is aligned to a page. Also rx buf size is aligned
>> with
>> +     * cache line alignment provided by the firmware, so tx buf's start
>> +     * address is guranteed to be aligned with the alignment provided by
>> +     * the firmware.
>> +     */
>> +    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 +1055,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 +1082,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..7e14da68fd17
>> --- /dev/null
>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>> @@ -0,0 +1,50 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) Pinecone Inc. 2019
>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>> + * Copyright (C) Advanced Micro Devices, Inc. 2026
>> + */
>> +
>> +#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 */
>> +
>> +/* Version of struct virtio_rpmsg_config understood by this driver */
>> +#define RPMSG_VDEV_CONFIG_V1    1
>> +
>> +/**
>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>> + *
>> + * @version:    version of this structure, currently
>> %RPMSG_VDEV_CONFIG_V1.
>> + * @reserved:    reserved for padding, must be zero.
>> + * @size:    size of this structure in bytes.
>> + * @rpmsg_buf_align:    required alignment in bytes for each buffer.
>> Must be a
>> + *        power of two so that both the buffer sizes and the TX buffer
>> + *        base address can be aligned (e.g. to a cache line).
>> + * @reserved1:    reserved for padding, must be zero. Keeps the
>> following 32-bit
>> + *        fields naturally aligned.
>> + * @txbuf_size:    Tx buf size from remote's view. For Linux this is
>> rx buf size.
>> + * @rxbuf_size:    Rx buf size from remote's view. For Linux this is
>> tx buf size.
>> + *
>> + * This is the configuration structure shared by the device and the
>> driver,
>> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are laid
>> out so
>> + * the structure is naturally 32-bit aligned.
>> + */
>> +struct virtio_rpmsg_config {
>> +    u8 version;
>> +    u8 reserved;
> 
> Why about defining the version type to u16 to avoid the reserved field?
> 
>> +    __virtio16 size;
>> +    __virtio16 rpmsg_buf_align;
>> +    __virtio16 reserved1;
> 
> Seems useless if __packed prevents the compiler from inserting extra
> padding
> bytes between fields,
> 
>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>> +    __virtio32 txbuf_size;
>> +    __virtio32 rxbuf_size;
>> +} __packed;
> 
> proposal
> 
> +struct virtio_rpmsg_config {
> +    __virtio16 version;
> +    __virtio16 size;
> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
> +    __virtio32 txbuf_size;
> +    __virtio32 rxbuf_size;
> +    __virtio16 rpmsg_buf_align;
> +} __packed;
> +
> 

I am okay with the above proposal with minor difference:

My proposal:

+struct virtio_rpmsg_config {
+	u8 version;
+	__virtio16 size;
+	__virtio16 rpmsg_buf_align;
+	/* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
+	__virtio32 txbuf_size;
+	__virtio32 rxbuf_size;
+} __packed;

I just want to keep version field 8-bit, as we will probably never use
upper byte of that field if we use 16-bit. Rest is okay. If the
strucutre is packed then reserved bytes are not needed.

Please let me know your view.

Thanks,
Tanmay


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

Re: [PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

On 6/17/26 19:41, Shah, Tanmay wrote:
> 
> 
> On 6/17/2026 4:15 AM, Arnaud POULIQUEN wrote:
>> Hi Tanmay,
>>
>> On 6/15/26 22:20, Tanmay Shah wrote:
>>> 512 bytes isn't always suitable for all case, let firmware
>>> maker decide the best value from resource table.
>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>>
>>> Changes in v4: squash to virtio rpmsg config patch
>>>     - Introduce new patch to modify rpmsg.rst documentation
>>>     - check version is always 1.
>>>     - check size field is same as size of struct virtio_rpmsg_config
>>>     - introduce alignment field
>>>     - check alignment field is power of 2
>>>     - check tx and rx buf size is aligned with alignment passed in the
>>>       structure
>>>
>>> Changes in v3:
>>>     - change version field from u16 to u8
>>>     - introduce size field in the rpmsg_virtio_config structure
>>>     - check version field is set to any non-zero value.
>>>     - check size field is not 0.
>>>     - Remove field for private config, as not needed for now.
>>>     - add documentation of rpmsg_virtio_config structure
>>>
>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 129 ++++++++++++++++++++++++-----
>>>    include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
>>>    2 files changed, 160 insertions(+), 19 deletions(-)
>>>    create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>> virtio_rpmsg_bus.c
>>> index 99df1ae07055..a59925f870a4 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -15,11 +15,13 @@
>>>    #include <linux/idr.h>
>>>    #include <linux/jiffies.h>
>>>    #include <linux/kernel.h>
>>> +#include <linux/log2.h>
>>>    #include <linux/module.h>
>>>    #include <linux/mutex.h>
>>>    #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 +41,8 @@
>>>     * @tx_bufs:    kernel address of tx buffers
>>>     * @num_rx_buf: total number of rx buffers
>>>     * @num_tx_buf: total number of tx buffers
>>> - * @buf_size:   size of one rx or tx buffer
>>> + * @rx_buf_size: size of one rx buffer
>>> + * @tx_buf_size: size of one tx buffer
>>>     * @last_tx_buf: index of last tx buffer used
>>>     * @bufs_dma:    dma base addr of the buffers
>>>     * @tx_lock:    protects svq and tx_bufs, to allow concurrent senders.
>>> @@ -59,7 +62,8 @@ struct virtproc_info {
>>>        void *rx_bufs, *tx_bufs;
>>>        unsigned int num_rx_buf;
>>>        unsigned int num_tx_buf;
>>> -    unsigned int buf_size;
>>> +    unsigned int rx_buf_size;
>>> +    unsigned int tx_buf_size;
>>>        int last_tx_buf;
>>>        dma_addr_t bufs_dma;
>>>        struct mutex tx_lock;
>>> @@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>          /* either pick the next unused tx buffer */
>>>        if (vrp->last_tx_buf < vrp->num_tx_buf)
>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>>        /* or recycle a used one */
>>>        else
>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>> @@ -514,7 +515,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 +648,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 +674,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 +707,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);
>>> @@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>        struct virtproc_info *vrp;
>>>        struct virtio_rpmsg_channel *vch = NULL;
>>>        struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
>>> +    u16 rpmsg_buf_align = 0;
>>>        void *bufs_va;
>>>        int err = 0, i;
>>>        size_t total_buf_space;
>>>        bool notify;
>>> +    u8 version;
>>> +    u16 size;
>>>          vrp = kzalloc_obj(*vrp);
>>>        if (!vrp)
>>> @@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>        else
>>>            vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>>    -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>> +    /*
>>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>>> +     * size from virtio device config space from the resource table.
>>> +     * If the feature is not supported, then assign default buf size.
>>> +     */
>>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 version, &version);
>>> +
>>> +        /* for now we support only v1 */
>>> +        if (version != RPMSG_VDEV_CONFIG_V1) {
>>> +            dev_err(&vdev->dev,
>>> +                "unsupported vdev config version %u\n", version);
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        /* size of the config space must match */
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 size, &size);
>>> +        if (size != sizeof(struct virtio_rpmsg_config)) {
>>> +            dev_err(&vdev->dev, "invalid size of vdev config %u\n",
>>> +                size);
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>>    -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>>>> buf_size;
>>> +        /*
>>> +         * Optional alignment applied to each buffer size and to the TX
>>> +         * buffer base address (e.g. to align buffers on a cache line).
>>> +         * It must be a power of two; zero means no extra alignment.
>>> +         */
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 rpmsg_buf_align, &rpmsg_buf_align);
>>> +        if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
>>> +            dev_err(&vdev->dev,
>>> +                "bad vdev config: rpmsg_buf_align %u is not a power
>>> of two\n",
>>> +                rpmsg_buf_align);
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        /* note: tx and rx are defined from remote view */
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 txbuf_size, &vrp->rx_buf_size);
>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                 rxbuf_size, &vrp->tx_buf_size);
>>> +
>>> +        /* The buffers must hold at least the rpmsg header */
>>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>>> +            dev_err(&vdev->dev,
>>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        /*
>>> +         * The buffer size must be aligned to the provided alignment for
>>> +         * so that the start address of tx bufs can be aligned.
>>> +         */
>>
>> 'tx' to remove as  it also concerns Rx buffers
>>
> 
> Ack.
> 
>>
>> What about removing this check to manage alignment during buffer
>> allocation?
>>
>> For example, if the alignment is on a 64-bit address and the tx_buffer
>> and rx_buffer sizes are 40 bytes, 48 bytes can be allocated in memory
>> for each buffer, and the virtio descriptor can be filled with aligned
>> addresses.
>>
>> In other words, the rpmsg_buf_align field contains the alignment
>> constraint from the remote processor. If the Linux kernel wants to
>> impose another alignment constraint, it must test or update
>> rpmsg_buf_align, but it must not impose alignment on the buffer size.
>>
>>
> 
> This part I don't understand. `rpmsg_buf_align` is alignment for only
> single buffer size. The linux kernel is checking that single rx buf size
> and tx buf size is aligned with `rpmsg_buf_align` as firmware has claimed.
> 
> For reference the openamp-system-reference PR:
> https://github.com/OpenAMP/openamp-system-reference/pull/106/changes
> 
> 	.vdev_config = {
> 		.version = 1,
> 		.reserved = 0,
> 		.size = (uint16_t)(sizeof(struct rpmsg_virtio_config) - sizeof(bool)),
> 		.alignment = RPMSG_BUF_ALIGN,
> 		.reserved1 = 0,
> 		/* Tx for host */
> 		.h2r_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
> 		/* Rx for host */
> 		.r2h_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
> 	},
> 
> IIUC, The linux kernel is not really supposed to modify
> `rpmsg_buf_align`. It only uses it to check that firmware has assigned
> correct size of single rx and tx buffer.
> 
> 
> When the linux kernel uses dma_alloc_coherent() API it aligns total
> buffer size with page size. That is different than single tx buf size
> and single rx buf size. The total buf size alignment to page size is
> irrelevant to `rpmsg_buf_align` field.
> 
> Please let me know if I am missing something or didn't understand your
> comment. I prefer that `rpmsg_buf_align` should be only modified by the
> firmware and not the linux kernel.


Sorry it was unclear, let try to reexplain my suggestion:

Two alignment constraints can apply:
- The remote processor can require an alignment through
   vdev_config::alignment.
- The main processor, which runs Linux or another operating system (OS),
   can require a different alignment, for example, for cache alignment.
In current Linux implementation no constraint in Linux.
nevertheless  I would be in favor of taking into account such future
constraint without imposing constraint on the buffer sizes.
Based on that in short term the local 'rpmsg_buf_align' would still 
computed
only from vdev_config::alignment (not update of vdev_config::alignment).

virtio_cread(vdev, struct virtio_rpmsg_config,
                  rpmsg_buf_align, &rpmsg_buf_align);

Then you could use use ALIGN() helper:

unsigned int rx_buf_align_size = ALIGN(vrp->rx_buf_size,
				       rpmsg_buf_align);
unsigned int tx_buf_align_size = ALIGN(vrp->tx_buf_size,
				       rpmsg_buf_align);

total_buf_space = (vrp->num_rx_buf * rx_buf_align_size) +
		  (vrp->num_tx_buf * tx_buf_align_size);

vrp->tx_bufs = bufs_va + vrp->num_rx_buf * rx_buf_align_size;

Apply the same rule to cpu_addr in the vring descriptor:

void *cpu_addr = vrp->rx_bufs + i * rx_buf_align_size;

rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);

With this approach, the buffer addresses remain aligned
independently of vdev_config::Rxbuf_size and vdev_config::txbuf_size.
Don't hesitate if it is still not clear!
> 
> 
>>> +        if (rpmsg_buf_align &&
>>> +            (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
>>> +             !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
>>> +            dev_err(&vdev->dev,
>>> +                "bad vdev config: buf sizes (rx %u, tx %u) not
>>> aligned to %u\n",
>>> +                vrp->rx_buf_size, vrp->tx_buf_size,
>>> +                rpmsg_buf_align);
>>> +            err = -EINVAL;
>>> +            goto vqs_del;
>>> +        }
>>> +
>>> +        dev_dbg(&vdev->dev,
>>> +            "vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz =
>>> 0x%x\n",
>>> +            version, rpmsg_buf_align, 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->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,
>>> @@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>        /* first part of the buffers is dedicated for RX */
>>>        vrp->rx_bufs = bufs_va;
>>>    -    /* and second part is dedicated for TX */
>>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>> +    /*
>>> +     * Here buf_va is aligned to a page. Also rx buf size is aligned
>>> with
>>> +     * cache line alignment provided by the firmware, so tx buf's start
>>> +     * address is guranteed to be aligned with the alignment provided by
>>> +     * the firmware.
>>> +     */
>>> +    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 +1055,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 +1082,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..7e14da68fd17
>>> --- /dev/null
>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>> @@ -0,0 +1,50 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) Pinecone Inc. 2019
>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>> + * Copyright (C) Advanced Micro Devices, Inc. 2026
>>> + */
>>> +
>>> +#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 */
>>> +
>>> +/* Version of struct virtio_rpmsg_config understood by this driver */
>>> +#define RPMSG_VDEV_CONFIG_V1    1
>>> +
>>> +/**
>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>>> + *
>>> + * @version:    version of this structure, currently
>>> %RPMSG_VDEV_CONFIG_V1.
>>> + * @reserved:    reserved for padding, must be zero.
>>> + * @size:    size of this structure in bytes.
>>> + * @rpmsg_buf_align:    required alignment in bytes for each buffer.
>>> Must be a
>>> + *        power of two so that both the buffer sizes and the TX buffer
>>> + *        base address can be aligned (e.g. to a cache line).
>>> + * @reserved1:    reserved for padding, must be zero. Keeps the
>>> following 32-bit
>>> + *        fields naturally aligned.
>>> + * @txbuf_size:    Tx buf size from remote's view. For Linux this is
>>> rx buf size.
>>> + * @rxbuf_size:    Rx buf size from remote's view. For Linux this is
>>> tx buf size.
>>> + *
>>> + * This is the configuration structure shared by the device and the
>>> driver,
>>> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are laid
>>> out so
>>> + * the structure is naturally 32-bit aligned.
>>> + */
>>> +struct virtio_rpmsg_config {
>>> +    u8 version;
>>> +    u8 reserved;
>>
>> Why about defining the version type to u16 to avoid the reserved field?
>>
>>> +    __virtio16 size;
>>> +    __virtio16 rpmsg_buf_align;
>>> +    __virtio16 reserved1;
>>
>> Seems useless if __packed prevents the compiler from inserting extra
>> padding
>> bytes between fields,
>>
>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>> +    __virtio32 txbuf_size;
>>> +    __virtio32 rxbuf_size;
>>> +} __packed;
>>
>> proposal
>>
>> +struct virtio_rpmsg_config {
>> +    __virtio16 version;
>> +    __virtio16 size;
>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>> +    __virtio32 txbuf_size;
>> +    __virtio32 rxbuf_size;
>> +    __virtio16 rpmsg_buf_align;
>> +} __packed;
>> +
>>
> 
> I am okay with the above proposal with minor difference:
> 
> My proposal:
> 
> +struct virtio_rpmsg_config {
> +	u8 version;
> +	__virtio16 size;
> +	__virtio16 rpmsg_buf_align;
> +	/* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
> +	__virtio32 txbuf_size;
> +	__virtio32 rxbuf_size;
> +} __packed;
> 
> I just want to keep version field 8-bit, as we will probably never use
> upper byte of that field if we use 16-bit. Rest is okay. If the
> strucutre is packed then reserved bytes are not needed.
> 
> Please let me know your view.

No strong opinion on that. In the end, this structure is read only one time.
If it is acceptable to Mathieu, it is acceptable to me.

Thanks,
Arnaud

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

Re: [PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 6/18/2026 3:32 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 6/17/26 19:41, Shah, Tanmay wrote:
>>
>>
>> On 6/17/2026 4:15 AM, Arnaud POULIQUEN wrote:
>>> Hi Tanmay,
>>>
>>> On 6/15/26 22:20, Tanmay Shah wrote:
>>>> 512 bytes isn't always suitable for all case, let firmware
>>>> maker decide the best value from resource table.
>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>
>>>> Changes in v4: squash to virtio rpmsg config patch
>>>>     - Introduce new patch to modify rpmsg.rst documentation
>>>>     - check version is always 1.
>>>>     - check size field is same as size of struct virtio_rpmsg_config
>>>>     - introduce alignment field
>>>>     - check alignment field is power of 2
>>>>     - check tx and rx buf size is aligned with alignment passed in the
>>>>       structure
>>>>
>>>> Changes in v3:
>>>>     - change version field from u16 to u8
>>>>     - introduce size field in the rpmsg_virtio_config structure
>>>>     - check version field is set to any non-zero value.
>>>>     - check size field is not 0.
>>>>     - Remove field for private config, as not needed for now.
>>>>     - add documentation of rpmsg_virtio_config structure
>>>>
>>>>    drivers/rpmsg/virtio_rpmsg_bus.c   | 129 +++++++++++++++++++++++
>>>> +-----
>>>>    include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
>>>>    2 files changed, 160 insertions(+), 19 deletions(-)
>>>>    create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>>
>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>>> virtio_rpmsg_bus.c
>>>> index 99df1ae07055..a59925f870a4 100644
>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> @@ -15,11 +15,13 @@
>>>>    #include <linux/idr.h>
>>>>    #include <linux/jiffies.h>
>>>>    #include <linux/kernel.h>
>>>> +#include <linux/log2.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/mutex.h>
>>>>    #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 +41,8 @@
>>>>     * @tx_bufs:    kernel address of tx buffers
>>>>     * @num_rx_buf: total number of rx buffers
>>>>     * @num_tx_buf: total number of tx buffers
>>>> - * @buf_size:   size of one rx or tx buffer
>>>> + * @rx_buf_size: size of one rx buffer
>>>> + * @tx_buf_size: size of one tx buffer
>>>>     * @last_tx_buf: index of last tx buffer used
>>>>     * @bufs_dma:    dma base addr of the buffers
>>>>     * @tx_lock:    protects svq and tx_bufs, to allow concurrent
>>>> senders.
>>>> @@ -59,7 +62,8 @@ struct virtproc_info {
>>>>        void *rx_bufs, *tx_bufs;
>>>>        unsigned int num_rx_buf;
>>>>        unsigned int num_tx_buf;
>>>> -    unsigned int buf_size;
>>>> +    unsigned int rx_buf_size;
>>>> +    unsigned int tx_buf_size;
>>>>        int last_tx_buf;
>>>>        dma_addr_t bufs_dma;
>>>>        struct mutex tx_lock;
>>>> @@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info
>>>> *vrp)
>>>>          /* either pick the next unused tx buffer */
>>>>        if (vrp->last_tx_buf < vrp->num_tx_buf)
>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>>>        /* or recycle a used one */
>>>>        else
>>>>            ret = virtqueue_get_buf(vrp->svq, &len);
>>>> @@ -514,7 +515,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 +648,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 +674,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 +707,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);
>>>> @@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device
>>>> *vdev)
>>>>        struct virtproc_info *vrp;
>>>>        struct virtio_rpmsg_channel *vch = NULL;
>>>>        struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
>>>> +    u16 rpmsg_buf_align = 0;
>>>>        void *bufs_va;
>>>>        int err = 0, i;
>>>>        size_t total_buf_space;
>>>>        bool notify;
>>>> +    u8 version;
>>>> +    u16 size;
>>>>          vrp = kzalloc_obj(*vrp);
>>>>        if (!vrp)
>>>> @@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>        else
>>>>            vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>>>    -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>>> +    /*
>>>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure
>>>> buf
>>>> +     * size from virtio device config space from the resource table.
>>>> +     * If the feature is not supported, then assign default buf size.
>>>> +     */
>>>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 version, &version);
>>>> +
>>>> +        /* for now we support only v1 */
>>>> +        if (version != RPMSG_VDEV_CONFIG_V1) {
>>>> +            dev_err(&vdev->dev,
>>>> +                "unsupported vdev config version %u\n", version);
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        /* size of the config space must match */
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 size, &size);
>>>> +        if (size != sizeof(struct virtio_rpmsg_config)) {
>>>> +            dev_err(&vdev->dev, "invalid size of vdev config %u\n",
>>>> +                size);
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>>    -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>>>>> buf_size;
>>>> +        /*
>>>> +         * Optional alignment applied to each buffer size and to
>>>> the TX
>>>> +         * buffer base address (e.g. to align buffers on a cache
>>>> line).
>>>> +         * It must be a power of two; zero means no extra alignment.
>>>> +         */
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 rpmsg_buf_align, &rpmsg_buf_align);
>>>> +        if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
>>>> +            dev_err(&vdev->dev,
>>>> +                "bad vdev config: rpmsg_buf_align %u is not a power
>>>> of two\n",
>>>> +                rpmsg_buf_align);
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        /* note: tx and rx are defined from remote view */
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 txbuf_size, &vrp->rx_buf_size);
>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>> +                 rxbuf_size, &vrp->tx_buf_size);
>>>> +
>>>> +        /* The buffers must hold at least the rpmsg header */
>>>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>>>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>>>> +            dev_err(&vdev->dev,
>>>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>>>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        /*
>>>> +         * The buffer size must be aligned to the provided
>>>> alignment for
>>>> +         * so that the start address of tx bufs can be aligned.
>>>> +         */
>>>
>>> 'tx' to remove as  it also concerns Rx buffers
>>>
>>
>> Ack.
>>
>>>
>>> What about removing this check to manage alignment during buffer
>>> allocation?
>>>
>>> For example, if the alignment is on a 64-bit address and the tx_buffer
>>> and rx_buffer sizes are 40 bytes, 48 bytes can be allocated in memory
>>> for each buffer, and the virtio descriptor can be filled with aligned
>>> addresses.
>>>
>>> In other words, the rpmsg_buf_align field contains the alignment
>>> constraint from the remote processor. If the Linux kernel wants to
>>> impose another alignment constraint, it must test or update
>>> rpmsg_buf_align, but it must not impose alignment on the buffer size.
>>>
>>>
>>
>> This part I don't understand. `rpmsg_buf_align` is alignment for only
>> single buffer size. The linux kernel is checking that single rx buf size
>> and tx buf size is aligned with `rpmsg_buf_align` as firmware has
>> claimed.
>>
>> For reference the openamp-system-reference PR:
>> https://github.com/OpenAMP/openamp-system-reference/pull/106/changes
>>
>>     .vdev_config = {
>>         .version = 1,
>>         .reserved = 0,
>>         .size = (uint16_t)(sizeof(struct rpmsg_virtio_config) -
>> sizeof(bool)),
>>         .alignment = RPMSG_BUF_ALIGN,
>>         .reserved1 = 0,
>>         /* Tx for host */
>>         .h2r_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
>>         /* Rx for host */
>>         .r2h_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
>>     },
>>
>> IIUC, The linux kernel is not really supposed to modify
>> `rpmsg_buf_align`. It only uses it to check that firmware has assigned
>> correct size of single rx and tx buffer.
>>
>>
>> When the linux kernel uses dma_alloc_coherent() API it aligns total
>> buffer size with page size. That is different than single tx buf size
>> and single rx buf size. The total buf size alignment to page size is
>> irrelevant to `rpmsg_buf_align` field.
>>
>> Please let me know if I am missing something or didn't understand your
>> comment. I prefer that `rpmsg_buf_align` should be only modified by the
>> firmware and not the linux kernel.
> 
> 
> Sorry it was unclear, let try to reexplain my suggestion:
> 
> Two alignment constraints can apply:
> - The remote processor can require an alignment through
>   vdev_config::alignment.
> - The main processor, which runs Linux or another operating system (OS),
>   can require a different alignment, for example, for cache alignment.
> In current Linux implementation no constraint in Linux.
> nevertheless  I would be in favor of taking into account such future
> constraint without imposing constraint on the buffer sizes.

Is this ever going to be ture? Is it ever possible that Linux and remote
has different cache alignment? IIUC, both will be using same cache and
so same alignment will be applicable. That is why only signle alignment
is required.

> Based on that in short term the local 'rpmsg_buf_align' would still
> computed
> only from vdev_config::alignment (not update of vdev_config::alignment).
> 
> virtio_cread(vdev, struct virtio_rpmsg_config,
>                  rpmsg_buf_align, &rpmsg_buf_align);
> 
> Then you could use use ALIGN() helper:
> 
> unsigned int rx_buf_align_size = ALIGN(vrp->rx_buf_size,
>                        rpmsg_buf_align);
> unsigned int tx_buf_align_size = ALIGN(vrp->tx_buf_size,
>                        rpmsg_buf_align);
> 

This is where I have different opinion. Instead of Linux using ALIGN()
macro, can we expect that firmware must assign the aligned buffer size
with vdev_config::rpmsg_buf_align? And so Linux will fail if the buffer
size is not aligned already from the firmware side. That is why I had
introduced checks instead of doing alignment by linux.

> total_buf_space = (vrp->num_rx_buf * rx_buf_align_size) +
>           (vrp->num_tx_buf * tx_buf_align_size);
> 
> vrp->tx_bufs = bufs_va + vrp->num_rx_buf * rx_buf_align_size;
> 
> Apply the same rule to cpu_addr in the vring descriptor:
> 
> void *cpu_addr = vrp->rx_bufs + i * rx_buf_align_size;
> 
> rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
> 
> With this approach, the buffer addresses remain aligned
> independently of vdev_config::Rxbuf_size and vdev_config::txbuf_size.
> Don't hesitate if it is still not clear!

How they remain aligned independent of tx/rx_buf_size? tx_bufs address
is still calculated based on rx_buf_align_size, so its alignment still
depends on rx_buf_align_size which is derived using
vdev_config::rpmsg_buf_align.

I think we are trying to achive the same thing, but implementation is
differnt. We just need to decide where the alignment should be done?

Either on the linux side? Or in the firmware resource table?

I prefer that the firmware should already provide aligned buffer size,
and Linux should only check it. If alignment is not done, then simply
fail with error. That way, firmware also knows the correct size of the
buffer. If Linux does the alignment, then the firmware is not aware of
the correct size that is used by the linux.

I am open to move the alignment operation to the linux side with the
reasonable justification.

Thank You,
Tanmay

>>
>>
>>>> +        if (rpmsg_buf_align &&
>>>> +            (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
>>>> +             !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
>>>> +            dev_err(&vdev->dev,
>>>> +                "bad vdev config: buf sizes (rx %u, tx %u) not
>>>> aligned to %u\n",
>>>> +                vrp->rx_buf_size, vrp->tx_buf_size,
>>>> +                rpmsg_buf_align);
>>>> +            err = -EINVAL;
>>>> +            goto vqs_del;
>>>> +        }
>>>> +
>>>> +        dev_dbg(&vdev->dev,
>>>> +            "vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz =
>>>> 0x%x\n",
>>>> +            version, rpmsg_buf_align, 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->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,
>>>> @@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device
>>>> *vdev)
>>>>        /* first part of the buffers is dedicated for RX */
>>>>        vrp->rx_bufs = bufs_va;
>>>>    -    /* and second part is dedicated for TX */
>>>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>>> +    /*
>>>> +     * Here buf_va is aligned to a page. Also rx buf size is aligned
>>>> with
>>>> +     * cache line alignment provided by the firmware, so tx buf's
>>>> start
>>>> +     * address is guranteed to be aligned with the alignment
>>>> provided by
>>>> +     * the firmware.
>>>> +     */
>>>> +    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 +1055,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 +1082,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..7e14da68fd17
>>>> --- /dev/null
>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>> @@ -0,0 +1,50 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Copyright (C) Pinecone Inc. 2019
>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>>> + * Copyright (C) Advanced Micro Devices, Inc. 2026
>>>> + */
>>>> +
>>>> +#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 */
>>>> +
>>>> +/* Version of struct virtio_rpmsg_config understood by this driver */
>>>> +#define RPMSG_VDEV_CONFIG_V1    1
>>>> +
>>>> +/**
>>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>>>> + *
>>>> + * @version:    version of this structure, currently
>>>> %RPMSG_VDEV_CONFIG_V1.
>>>> + * @reserved:    reserved for padding, must be zero.
>>>> + * @size:    size of this structure in bytes.
>>>> + * @rpmsg_buf_align:    required alignment in bytes for each buffer.
>>>> Must be a
>>>> + *        power of two so that both the buffer sizes and the TX buffer
>>>> + *        base address can be aligned (e.g. to a cache line).
>>>> + * @reserved1:    reserved for padding, must be zero. Keeps the
>>>> following 32-bit
>>>> + *        fields naturally aligned.
>>>> + * @txbuf_size:    Tx buf size from remote's view. For Linux this is
>>>> rx buf size.
>>>> + * @rxbuf_size:    Rx buf size from remote's view. For Linux this is
>>>> tx buf size.
>>>> + *
>>>> + * This is the configuration structure shared by the device and the
>>>> driver,
>>>> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are laid
>>>> out so
>>>> + * the structure is naturally 32-bit aligned.
>>>> + */
>>>> +struct virtio_rpmsg_config {
>>>> +    u8 version;
>>>> +    u8 reserved;
>>>
>>> Why about defining the version type to u16 to avoid the reserved field?
>>>
>>>> +    __virtio16 size;
>>>> +    __virtio16 rpmsg_buf_align;
>>>> +    __virtio16 reserved1;
>>>
>>> Seems useless if __packed prevents the compiler from inserting extra
>>> padding
>>> bytes between fields,
>>>
>>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>>> +    __virtio32 txbuf_size;
>>>> +    __virtio32 rxbuf_size;
>>>> +} __packed;
>>>
>>> proposal
>>>
>>> +struct virtio_rpmsg_config {
>>> +    __virtio16 version;
>>> +    __virtio16 size;
>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>> +    __virtio32 txbuf_size;
>>> +    __virtio32 rxbuf_size;
>>> +    __virtio16 rpmsg_buf_align;
>>> +} __packed;
>>> +
>>>
>>
>> I am okay with the above proposal with minor difference:
>>
>> My proposal:
>>
>> +struct virtio_rpmsg_config {
>> +    u8 version;
>> +    __virtio16 size;
>> +    __virtio16 rpmsg_buf_align;
>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>> +    __virtio32 txbuf_size;
>> +    __virtio32 rxbuf_size;
>> +} __packed;
>>
>> I just want to keep version field 8-bit, as we will probably never use
>> upper byte of that field if we use 16-bit. Rest is okay. If the
>> strucutre is packed then reserved bytes are not needed.
>>
>> Please let me know your view.
> 
> No strong opinion on that. In the end, this structure is read only one
> time.
> If it is acceptable to Mathieu, it is acceptable to me.
> 
> Thanks,
> Arnaud
> 
>>
>> Thanks,
>> Tanmay
>>
>>
>>> Regards,
>>> Arnaud
>>>
>>>> +
>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>
>>
> 

Re: [PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Arnaud POULIQUEN]

On 6/18/26 18:31, Shah, Tanmay wrote:
> 
> 
> On 6/18/2026 3:32 AM, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/17/26 19:41, Shah, Tanmay wrote:
>>>
>>>
>>> On 6/17/2026 4:15 AM, Arnaud POULIQUEN wrote:
>>>> Hi Tanmay,
>>>>
>>>> On 6/15/26 22:20, Tanmay Shah wrote:
>>>>> 512 bytes isn't always suitable for all case, let firmware
>>>>> maker decide the best value from resource table.
>>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>>>
>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>> ---
>>>>>
>>>>> Changes in v4: squash to virtio rpmsg config patch
>>>>>      - Introduce new patch to modify rpmsg.rst documentation
>>>>>      - check version is always 1.
>>>>>      - check size field is same as size of struct virtio_rpmsg_config
>>>>>      - introduce alignment field
>>>>>      - check alignment field is power of 2
>>>>>      - check tx and rx buf size is aligned with alignment passed in the
>>>>>        structure
>>>>>
>>>>> Changes in v3:
>>>>>      - change version field from u16 to u8
>>>>>      - introduce size field in the rpmsg_virtio_config structure
>>>>>      - check version field is set to any non-zero value.
>>>>>      - check size field is not 0.
>>>>>      - Remove field for private config, as not needed for now.
>>>>>      - add documentation of rpmsg_virtio_config structure
>>>>>
>>>>>     drivers/rpmsg/virtio_rpmsg_bus.c   | 129 +++++++++++++++++++++++
>>>>> +-----
>>>>>     include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
>>>>>     2 files changed, 160 insertions(+), 19 deletions(-)
>>>>>     create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>>>
>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>>>> virtio_rpmsg_bus.c
>>>>> index 99df1ae07055..a59925f870a4 100644
>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> @@ -15,11 +15,13 @@
>>>>>     #include <linux/idr.h>
>>>>>     #include <linux/jiffies.h>
>>>>>     #include <linux/kernel.h>
>>>>> +#include <linux/log2.h>
>>>>>     #include <linux/module.h>
>>>>>     #include <linux/mutex.h>
>>>>>     #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 +41,8 @@
>>>>>      * @tx_bufs:    kernel address of tx buffers
>>>>>      * @num_rx_buf: total number of rx buffers
>>>>>      * @num_tx_buf: total number of tx buffers
>>>>> - * @buf_size:   size of one rx or tx buffer
>>>>> + * @rx_buf_size: size of one rx buffer
>>>>> + * @tx_buf_size: size of one tx buffer
>>>>>      * @last_tx_buf: index of last tx buffer used
>>>>>      * @bufs_dma:    dma base addr of the buffers
>>>>>      * @tx_lock:    protects svq and tx_bufs, to allow concurrent
>>>>> senders.
>>>>> @@ -59,7 +62,8 @@ struct virtproc_info {
>>>>>         void *rx_bufs, *tx_bufs;
>>>>>         unsigned int num_rx_buf;
>>>>>         unsigned int num_tx_buf;
>>>>> -    unsigned int buf_size;
>>>>> +    unsigned int rx_buf_size;
>>>>> +    unsigned int tx_buf_size;
>>>>>         int last_tx_buf;
>>>>>         dma_addr_t bufs_dma;
>>>>>         struct mutex tx_lock;
>>>>> @@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info
>>>>> *vrp)
>>>>>           /* either pick the next unused tx buffer */
>>>>>         if (vrp->last_tx_buf < vrp->num_tx_buf)
>>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>>>>         /* or recycle a used one */
>>>>>         else
>>>>>             ret = virtqueue_get_buf(vrp->svq, &len);
>>>>> @@ -514,7 +515,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 +648,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 +674,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 +707,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);
>>>>> @@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device
>>>>> *vdev)
>>>>>         struct virtproc_info *vrp;
>>>>>         struct virtio_rpmsg_channel *vch = NULL;
>>>>>         struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
>>>>> +    u16 rpmsg_buf_align = 0;
>>>>>         void *bufs_va;
>>>>>         int err = 0, i;
>>>>>         size_t total_buf_space;
>>>>>         bool notify;
>>>>> +    u8 version;
>>>>> +    u16 size;
>>>>>           vrp = kzalloc_obj(*vrp);
>>>>>         if (!vrp)
>>>>> @@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>         else
>>>>>             vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>>>>     -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>>>> +    /*
>>>>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure
>>>>> buf
>>>>> +     * size from virtio device config space from the resource table.
>>>>> +     * If the feature is not supported, then assign default buf size.
>>>>> +     */
>>>>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 version, &version);
>>>>> +
>>>>> +        /* for now we support only v1 */
>>>>> +        if (version != RPMSG_VDEV_CONFIG_V1) {
>>>>> +            dev_err(&vdev->dev,
>>>>> +                "unsupported vdev config version %u\n", version);
>>>>> +            err = -EINVAL;
>>>>> +            goto vqs_del;
>>>>> +        }
>>>>> +
>>>>> +        /* size of the config space must match */
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 size, &size);
>>>>> +        if (size != sizeof(struct virtio_rpmsg_config)) {
>>>>> +            dev_err(&vdev->dev, "invalid size of vdev config %u\n",
>>>>> +                size);
>>>>> +            err = -EINVAL;
>>>>> +            goto vqs_del;
>>>>> +        }
>>>>>     -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>>>>>> buf_size;
>>>>> +        /*
>>>>> +         * Optional alignment applied to each buffer size and to
>>>>> the TX
>>>>> +         * buffer base address (e.g. to align buffers on a cache
>>>>> line).
>>>>> +         * It must be a power of two; zero means no extra alignment.
>>>>> +         */
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 rpmsg_buf_align, &rpmsg_buf_align);
>>>>> +        if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
>>>>> +            dev_err(&vdev->dev,
>>>>> +                "bad vdev config: rpmsg_buf_align %u is not a power
>>>>> of two\n",
>>>>> +                rpmsg_buf_align);
>>>>> +            err = -EINVAL;
>>>>> +            goto vqs_del;
>>>>> +        }
>>>>> +
>>>>> +        /* note: tx and rx are defined from remote view */
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 txbuf_size, &vrp->rx_buf_size);
>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                 rxbuf_size, &vrp->tx_buf_size);
>>>>> +
>>>>> +        /* The buffers must hold at least the rpmsg header */
>>>>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>>>>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>>>>> +            dev_err(&vdev->dev,
>>>>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>>>>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>>>>> +            err = -EINVAL;
>>>>> +            goto vqs_del;
>>>>> +        }
>>>>> +
>>>>> +        /*
>>>>> +         * The buffer size must be aligned to the provided
>>>>> alignment for
>>>>> +         * so that the start address of tx bufs can be aligned.
>>>>> +         */
>>>>
>>>> 'tx' to remove as  it also concerns Rx buffers
>>>>
>>>
>>> Ack.
>>>
>>>>
>>>> What about removing this check to manage alignment during buffer
>>>> allocation?
>>>>
>>>> For example, if the alignment is on a 64-bit address and the tx_buffer
>>>> and rx_buffer sizes are 40 bytes, 48 bytes can be allocated in memory
>>>> for each buffer, and the virtio descriptor can be filled with aligned
>>>> addresses.
>>>>
>>>> In other words, the rpmsg_buf_align field contains the alignment
>>>> constraint from the remote processor. If the Linux kernel wants to
>>>> impose another alignment constraint, it must test or update
>>>> rpmsg_buf_align, but it must not impose alignment on the buffer size.
>>>>
>>>>
>>>
>>> This part I don't understand. `rpmsg_buf_align` is alignment for only
>>> single buffer size. The linux kernel is checking that single rx buf size
>>> and tx buf size is aligned with `rpmsg_buf_align` as firmware has
>>> claimed.
>>>
>>> For reference the openamp-system-reference PR:
>>> https://github.com/OpenAMP/openamp-system-reference/pull/106/changes
>>>
>>>      .vdev_config = {
>>>          .version = 1,
>>>          .reserved = 0,
>>>          .size = (uint16_t)(sizeof(struct rpmsg_virtio_config) -
>>> sizeof(bool)),
>>>          .alignment = RPMSG_BUF_ALIGN,
>>>          .reserved1 = 0,
>>>          /* Tx for host */
>>>          .h2r_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
>>>          /* Rx for host */
>>>          .r2h_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
>>>      },
>>>
>>> IIUC, The linux kernel is not really supposed to modify
>>> `rpmsg_buf_align`. It only uses it to check that firmware has assigned
>>> correct size of single rx and tx buffer.
>>>
>>>
>>> When the linux kernel uses dma_alloc_coherent() API it aligns total
>>> buffer size with page size. That is different than single tx buf size
>>> and single rx buf size. The total buf size alignment to page size is
>>> irrelevant to `rpmsg_buf_align` field.
>>>
>>> Please let me know if I am missing something or didn't understand your
>>> comment. I prefer that `rpmsg_buf_align` should be only modified by the
>>> firmware and not the linux kernel.
>>
>>
>> Sorry it was unclear, let try to reexplain my suggestion:
>>
>> Two alignment constraints can apply:
>> - The remote processor can require an alignment through
>>    vdev_config::alignment.
>> - The main processor, which runs Linux or another operating system (OS),
>>    can require a different alignment, for example, for cache alignment.
>> In current Linux implementation no constraint in Linux.
>> nevertheless  I would be in favor of taking into account such future
>> constraint without imposing constraint on the buffer sizes.
> 
> Is this ever going to be ture? Is it ever possible that Linux and remote
> has different cache alignment? IIUC, both will be using same cache and
> so same alignment will be applicable. That is why only signle alignment
> is required.

Some remote processors, for example, some Arm Cortex-M33, do not 
integrate cache. Even if cache exists, cache can be enabled on one 
processor, but not on the other.

> 
>> Based on that in short term the local 'rpmsg_buf_align' would still
>> computed
>> only from vdev_config::alignment (not update of vdev_config::alignment).
>>
>> virtio_cread(vdev, struct virtio_rpmsg_config,
>>                   rpmsg_buf_align, &rpmsg_buf_align);
>>
>> Then you could use use ALIGN() helper:
>>
>> unsigned int rx_buf_align_size = ALIGN(vrp->rx_buf_size,
>>                         rpmsg_buf_align);
>> unsigned int tx_buf_align_size = ALIGN(vrp->tx_buf_size,
>>                         rpmsg_buf_align);
>>
> 
> This is where I have different opinion. Instead of Linux using ALIGN()
> macro, can we expect that firmware must assign the aligned buffer size
> with vdev_config::rpmsg_buf_align? And so Linux will fail if the buffer
> size is not aligned already from the firmware side. That is why I had
> introduced checks instead of doing alignment by linux.
> 
>> total_buf_space = (vrp->num_rx_buf * rx_buf_align_size) +
>>            (vrp->num_tx_buf * tx_buf_align_size);
>>
>> vrp->tx_bufs = bufs_va + vrp->num_rx_buf * rx_buf_align_size;
>>
>> Apply the same rule to cpu_addr in the vring descriptor:
>>
>> void *cpu_addr = vrp->rx_bufs + i * rx_buf_align_size;
>>
>> rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
>>
>> With this approach, the buffer addresses remain aligned
>> independently of vdev_config::Rxbuf_size and vdev_config::txbuf_size.
>> Don't hesitate if it is still not clear!
> 
> How they remain aligned independent of tx/rx_buf_size? tx_bufs address
> is still calculated based on rx_buf_align_size, so its alignment still
> depends on rx_buf_align_size which is derived using
> vdev_config::rpmsg_buf_align.>
> I think we are trying to achive the same thing, but implementation is
> differnt. We just need to decide where the alignment should be done?
> 
> Either on the linux side? Or in the firmware resource table?
> 
> I prefer that the firmware should already provide aligned buffer size,
> and Linux should only check it. If alignment is not done, then simply
> fail with error. That way, firmware also knows the correct size of the
> buffer. If Linux does the alignment, then the firmware is not aware of
> the correct size that is used by the linux.
> 
> I am open to move the alignment operation to the linux side with the
> reasonable justification.

That remains a suggestion. My main concern with the implementation is
that RPMsg size should depend only on the max playlod size needed, not
also on the memory alignment.

If this constraint is kept, it must be imposed on all other non-Linux
solutions. Otherwise, the remote implementation depends on the main
processor implementation.

 From my POV, It would be preferable not to impose such constraint when
possible.

Thanks,
Arnaud

> 
> Thank You,
> Tanmay
> 
>>>
>>>
>>>>> +        if (rpmsg_buf_align &&
>>>>> +            (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
>>>>> +             !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
>>>>> +            dev_err(&vdev->dev,
>>>>> +                "bad vdev config: buf sizes (rx %u, tx %u) not
>>>>> aligned to %u\n",
>>>>> +                vrp->rx_buf_size, vrp->tx_buf_size,
>>>>> +                rpmsg_buf_align);
>>>>> +            err = -EINVAL;
>>>>> +            goto vqs_del;
>>>>> +        }
>>>>> +
>>>>> +        dev_dbg(&vdev->dev,
>>>>> +            "vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz =
>>>>> 0x%x\n",
>>>>> +            version, rpmsg_buf_align, 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->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,
>>>>> @@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device
>>>>> *vdev)
>>>>>         /* first part of the buffers is dedicated for RX */
>>>>>         vrp->rx_bufs = bufs_va;
>>>>>     -    /* and second part is dedicated for TX */
>>>>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>>>> +    /*
>>>>> +     * Here buf_va is aligned to a page. Also rx buf size is aligned
>>>>> with
>>>>> +     * cache line alignment provided by the firmware, so tx buf's
>>>>> start
>>>>> +     * address is guranteed to be aligned with the alignment
>>>>> provided by
>>>>> +     * the firmware.
>>>>> +     */
>>>>> +    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 +1055,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 +1082,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..7e14da68fd17
>>>>> --- /dev/null
>>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>>> @@ -0,0 +1,50 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/*
>>>>> + * Copyright (C) Pinecone Inc. 2019
>>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>>>> + * Copyright (C) Advanced Micro Devices, Inc. 2026
>>>>> + */
>>>>> +
>>>>> +#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 */
>>>>> +
>>>>> +/* Version of struct virtio_rpmsg_config understood by this driver */
>>>>> +#define RPMSG_VDEV_CONFIG_V1    1
>>>>> +
>>>>> +/**
>>>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>>>>> + *
>>>>> + * @version:    version of this structure, currently
>>>>> %RPMSG_VDEV_CONFIG_V1.
>>>>> + * @reserved:    reserved for padding, must be zero.
>>>>> + * @size:    size of this structure in bytes.
>>>>> + * @rpmsg_buf_align:    required alignment in bytes for each buffer.
>>>>> Must be a
>>>>> + *        power of two so that both the buffer sizes and the TX buffer
>>>>> + *        base address can be aligned (e.g. to a cache line).
>>>>> + * @reserved1:    reserved for padding, must be zero. Keeps the
>>>>> following 32-bit
>>>>> + *        fields naturally aligned.
>>>>> + * @txbuf_size:    Tx buf size from remote's view. For Linux this is
>>>>> rx buf size.
>>>>> + * @rxbuf_size:    Rx buf size from remote's view. For Linux this is
>>>>> tx buf size.
>>>>> + *
>>>>> + * This is the configuration structure shared by the device and the
>>>>> driver,
>>>>> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are laid
>>>>> out so
>>>>> + * the structure is naturally 32-bit aligned.
>>>>> + */
>>>>> +struct virtio_rpmsg_config {
>>>>> +    u8 version;
>>>>> +    u8 reserved;
>>>>
>>>> Why about defining the version type to u16 to avoid the reserved field?
>>>>
>>>>> +    __virtio16 size;
>>>>> +    __virtio16 rpmsg_buf_align;
>>>>> +    __virtio16 reserved1;
>>>>
>>>> Seems useless if __packed prevents the compiler from inserting extra
>>>> padding
>>>> bytes between fields,
>>>>
>>>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>>>> +    __virtio32 txbuf_size;
>>>>> +    __virtio32 rxbuf_size;
>>>>> +} __packed;
>>>>
>>>> proposal
>>>>
>>>> +struct virtio_rpmsg_config {
>>>> +    __virtio16 version;
>>>> +    __virtio16 size;
>>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>>> +    __virtio32 txbuf_size;
>>>> +    __virtio32 rxbuf_size;
>>>> +    __virtio16 rpmsg_buf_align;
>>>> +} __packed;
>>>> +
>>>>
>>>
>>> I am okay with the above proposal with minor difference:
>>>
>>> My proposal:
>>>
>>> +struct virtio_rpmsg_config {
>>> +    u8 version;
>>> +    __virtio16 size;
>>> +    __virtio16 rpmsg_buf_align;
>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>> +    __virtio32 txbuf_size;
>>> +    __virtio32 rxbuf_size;
>>> +} __packed;
>>>
>>> I just want to keep version field 8-bit, as we will probably never use
>>> upper byte of that field if we use 16-bit. Rest is okay. If the
>>> strucutre is packed then reserved bytes are not needed.
>>>
>>> Please let me know your view.
>>
>> No strong opinion on that. In the end, this structure is read only one
>> time.
>> If it is acceptable to Mathieu, it is acceptable to me.
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Tanmay
>>>
>>>
>>>> Regards,
>>>> Arnaud
>>>>
>>>>> +
>>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>>
>>>
>>
> 

Re: [PATCH v4 3/5] rpmsg: virtio_rpmsg_bus: get buffer size from config space

[Shah, Tanmay]

On 6/19/2026 2:45 AM, Arnaud POULIQUEN wrote:
> 
> 
> On 6/18/26 18:31, Shah, Tanmay wrote:
>>
>>
>> On 6/18/2026 3:32 AM, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 6/17/26 19:41, Shah, Tanmay wrote:
>>>>
>>>>
>>>> On 6/17/2026 4:15 AM, Arnaud POULIQUEN wrote:
>>>>> Hi Tanmay,
>>>>>
>>>>> On 6/15/26 22:20, Tanmay Shah wrote:
>>>>>> 512 bytes isn't always suitable for all case, let firmware
>>>>>> maker decide the best value from resource table.
>>>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>>>>
>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v4: squash to virtio rpmsg config patch
>>>>>>      - Introduce new patch to modify rpmsg.rst documentation
>>>>>>      - check version is always 1.
>>>>>>      - check size field is same as size of struct virtio_rpmsg_config
>>>>>>      - introduce alignment field
>>>>>>      - check alignment field is power of 2
>>>>>>      - check tx and rx buf size is aligned with alignment passed
>>>>>> in the
>>>>>>        structure
>>>>>>
>>>>>> Changes in v3:
>>>>>>      - change version field from u16 to u8
>>>>>>      - introduce size field in the rpmsg_virtio_config structure
>>>>>>      - check version field is set to any non-zero value.
>>>>>>      - check size field is not 0.
>>>>>>      - Remove field for private config, as not needed for now.
>>>>>>      - add documentation of rpmsg_virtio_config structure
>>>>>>
>>>>>>     drivers/rpmsg/virtio_rpmsg_bus.c   | 129 +++++++++++++++++++++++
>>>>>> +-----
>>>>>>     include/linux/rpmsg/virtio_rpmsg.h |  50 +++++++++++
>>>>>>     2 files changed, 160 insertions(+), 19 deletions(-)
>>>>>>     create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>>>>
>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>>>>> virtio_rpmsg_bus.c
>>>>>> index 99df1ae07055..a59925f870a4 100644
>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>> @@ -15,11 +15,13 @@
>>>>>>     #include <linux/idr.h>
>>>>>>     #include <linux/jiffies.h>
>>>>>>     #include <linux/kernel.h>
>>>>>> +#include <linux/log2.h>
>>>>>>     #include <linux/module.h>
>>>>>>     #include <linux/mutex.h>
>>>>>>     #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 +41,8 @@
>>>>>>      * @tx_bufs:    kernel address of tx buffers
>>>>>>      * @num_rx_buf: total number of rx buffers
>>>>>>      * @num_tx_buf: total number of tx buffers
>>>>>> - * @buf_size:   size of one rx or tx buffer
>>>>>> + * @rx_buf_size: size of one rx buffer
>>>>>> + * @tx_buf_size: size of one tx buffer
>>>>>>      * @last_tx_buf: index of last tx buffer used
>>>>>>      * @bufs_dma:    dma base addr of the buffers
>>>>>>      * @tx_lock:    protects svq and tx_bufs, to allow concurrent
>>>>>> senders.
>>>>>> @@ -59,7 +62,8 @@ struct virtproc_info {
>>>>>>         void *rx_bufs, *tx_bufs;
>>>>>>         unsigned int num_rx_buf;
>>>>>>         unsigned int num_tx_buf;
>>>>>> -    unsigned int buf_size;
>>>>>> +    unsigned int rx_buf_size;
>>>>>> +    unsigned int tx_buf_size;
>>>>>>         int last_tx_buf;
>>>>>>         dma_addr_t bufs_dma;
>>>>>>         struct mutex tx_lock;
>>>>>> @@ -68,9 +72,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 +129,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 +445,7 @@ static void *get_a_tx_buf(struct virtproc_info
>>>>>> *vrp)
>>>>>>           /* either pick the next unused tx buffer */
>>>>>>         if (vrp->last_tx_buf < vrp->num_tx_buf)
>>>>>> -        ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
>>>>>> +        ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
>>>>>>         /* or recycle a used one */
>>>>>>         else
>>>>>>             ret = virtqueue_get_buf(vrp->svq, &len);
>>>>>> @@ -514,7 +515,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 +648,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 +674,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 +707,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);
>>>>>> @@ -820,10 +821,13 @@ static int rpmsg_probe(struct virtio_device
>>>>>> *vdev)
>>>>>>         struct virtproc_info *vrp;
>>>>>>         struct virtio_rpmsg_channel *vch = NULL;
>>>>>>         struct rpmsg_device *rpdev_ns, *rpdev_ctrl;
>>>>>> +    u16 rpmsg_buf_align = 0;
>>>>>>         void *bufs_va;
>>>>>>         int err = 0, i;
>>>>>>         size_t total_buf_space;
>>>>>>         bool notify;
>>>>>> +    u8 version;
>>>>>> +    u16 size;
>>>>>>           vrp = kzalloc_obj(*vrp);
>>>>>>         if (!vrp)
>>>>>> @@ -855,9 +859,90 @@ static int rpmsg_probe(struct virtio_device
>>>>>> *vdev)
>>>>>>         else
>>>>>>             vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>>>>>     -    vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>>>>> +    /*
>>>>>> +     * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure
>>>>>> buf
>>>>>> +     * size from virtio device config space from the resource table.
>>>>>> +     * If the feature is not supported, then assign default buf
>>>>>> size.
>>>>>> +     */
>>>>>> +    if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>>> +                 version, &version);
>>>>>> +
>>>>>> +        /* for now we support only v1 */
>>>>>> +        if (version != RPMSG_VDEV_CONFIG_V1) {
>>>>>> +            dev_err(&vdev->dev,
>>>>>> +                "unsupported vdev config version %u\n", version);
>>>>>> +            err = -EINVAL;
>>>>>> +            goto vqs_del;
>>>>>> +        }
>>>>>> +
>>>>>> +        /* size of the config space must match */
>>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>>> +                 size, &size);
>>>>>> +        if (size != sizeof(struct virtio_rpmsg_config)) {
>>>>>> +            dev_err(&vdev->dev, "invalid size of vdev config %u\n",
>>>>>> +                size);
>>>>>> +            err = -EINVAL;
>>>>>> +            goto vqs_del;
>>>>>> +        }
>>>>>>     -    total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
>>>>>>> buf_size;
>>>>>> +        /*
>>>>>> +         * Optional alignment applied to each buffer size and to
>>>>>> the TX
>>>>>> +         * buffer base address (e.g. to align buffers on a cache
>>>>>> line).
>>>>>> +         * It must be a power of two; zero means no extra alignment.
>>>>>> +         */
>>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>>> +                 rpmsg_buf_align, &rpmsg_buf_align);
>>>>>> +        if (rpmsg_buf_align && !is_power_of_2(rpmsg_buf_align)) {
>>>>>> +            dev_err(&vdev->dev,
>>>>>> +                "bad vdev config: rpmsg_buf_align %u is not a power
>>>>>> of two\n",
>>>>>> +                rpmsg_buf_align);
>>>>>> +            err = -EINVAL;
>>>>>> +            goto vqs_del;
>>>>>> +        }
>>>>>> +
>>>>>> +        /* note: tx and rx are defined from remote view */
>>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>>> +                 txbuf_size, &vrp->rx_buf_size);
>>>>>> +        virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>>> +                 rxbuf_size, &vrp->tx_buf_size);
>>>>>> +
>>>>>> +        /* The buffers must hold at least the rpmsg header */
>>>>>> +        if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
>>>>>> +            vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
>>>>>> +            dev_err(&vdev->dev,
>>>>>> +                "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
>>>>>> +                vrp->rx_buf_size, vrp->tx_buf_size);
>>>>>> +            err = -EINVAL;
>>>>>> +            goto vqs_del;
>>>>>> +        }
>>>>>> +
>>>>>> +        /*
>>>>>> +         * The buffer size must be aligned to the provided
>>>>>> alignment for
>>>>>> +         * so that the start address of tx bufs can be aligned.
>>>>>> +         */
>>>>>
>>>>> 'tx' to remove as  it also concerns Rx buffers
>>>>>
>>>>
>>>> Ack.
>>>>
>>>>>
>>>>> What about removing this check to manage alignment during buffer
>>>>> allocation?
>>>>>
>>>>> For example, if the alignment is on a 64-bit address and the tx_buffer
>>>>> and rx_buffer sizes are 40 bytes, 48 bytes can be allocated in memory
>>>>> for each buffer, and the virtio descriptor can be filled with aligned
>>>>> addresses.
>>>>>
>>>>> In other words, the rpmsg_buf_align field contains the alignment
>>>>> constraint from the remote processor. If the Linux kernel wants to
>>>>> impose another alignment constraint, it must test or update
>>>>> rpmsg_buf_align, but it must not impose alignment on the buffer size.
>>>>>
>>>>>
>>>>
>>>> This part I don't understand. `rpmsg_buf_align` is alignment for only
>>>> single buffer size. The linux kernel is checking that single rx buf
>>>> size
>>>> and tx buf size is aligned with `rpmsg_buf_align` as firmware has
>>>> claimed.
>>>>
>>>> For reference the openamp-system-reference PR:
>>>> https://github.com/OpenAMP/openamp-system-reference/pull/106/changes
>>>>
>>>>      .vdev_config = {
>>>>          .version = 1,
>>>>          .reserved = 0,
>>>>          .size = (uint16_t)(sizeof(struct rpmsg_virtio_config) -
>>>> sizeof(bool)),
>>>>          .alignment = RPMSG_BUF_ALIGN,
>>>>          .reserved1 = 0,
>>>>          /* Tx for host */
>>>>          .h2r_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
>>>>          /* Rx for host */
>>>>          .r2h_buf_size = metal_align_up(4096, RPMSG_BUF_ALIGN),
>>>>      },
>>>>
>>>> IIUC, The linux kernel is not really supposed to modify
>>>> `rpmsg_buf_align`. It only uses it to check that firmware has assigned
>>>> correct size of single rx and tx buffer.
>>>>
>>>>
>>>> When the linux kernel uses dma_alloc_coherent() API it aligns total
>>>> buffer size with page size. That is different than single tx buf size
>>>> and single rx buf size. The total buf size alignment to page size is
>>>> irrelevant to `rpmsg_buf_align` field.
>>>>
>>>> Please let me know if I am missing something or didn't understand your
>>>> comment. I prefer that `rpmsg_buf_align` should be only modified by the
>>>> firmware and not the linux kernel.
>>>
>>>
>>> Sorry it was unclear, let try to reexplain my suggestion:
>>>
>>> Two alignment constraints can apply:
>>> - The remote processor can require an alignment through
>>>    vdev_config::alignment.
>>> - The main processor, which runs Linux or another operating system (OS),
>>>    can require a different alignment, for example, for cache alignment.
>>> In current Linux implementation no constraint in Linux.
>>> nevertheless  I would be in favor of taking into account such future
>>> constraint without imposing constraint on the buffer sizes.
>>
>> Is this ever going to be ture? Is it ever possible that Linux and remote
>> has different cache alignment? IIUC, both will be using same cache and
>> so same alignment will be applicable. That is why only signle alignment
>> is required.
> 
> Some remote processors, for example, some Arm Cortex-M33, do not
> integrate cache. Even if cache exists, cache can be enabled on one
> processor, but not on the other.
> 

Okay, how about introducing two alignment in that case?
vdev_config::rpmsg_buf_align_remote, and vdev_config::rpmsg_buf_align_host ?

If remote doesn't have cache, then remote alignment will be 0, and the
*_host alignment can be applied. The rsc_table can provide both, and the
*_host will take priority over *_remote.


>>
>>> Based on that in short term the local 'rpmsg_buf_align' would still
>>> computed
>>> only from vdev_config::alignment (not update of vdev_config::alignment).
>>>
>>> virtio_cread(vdev, struct virtio_rpmsg_config,
>>>                   rpmsg_buf_align, &rpmsg_buf_align);
>>>
>>> Then you could use use ALIGN() helper:
>>>
>>> unsigned int rx_buf_align_size = ALIGN(vrp->rx_buf_size,
>>>                         rpmsg_buf_align);
>>> unsigned int tx_buf_align_size = ALIGN(vrp->tx_buf_size,
>>>                         rpmsg_buf_align);
>>>
>>
>> This is where I have different opinion. Instead of Linux using ALIGN()
>> macro, can we expect that firmware must assign the aligned buffer size
>> with vdev_config::rpmsg_buf_align? And so Linux will fail if the buffer
>> size is not aligned already from the firmware side. That is why I had
>> introduced checks instead of doing alignment by linux.
>>
>>> total_buf_space = (vrp->num_rx_buf * rx_buf_align_size) +
>>>            (vrp->num_tx_buf * tx_buf_align_size);
>>>
>>> vrp->tx_bufs = bufs_va + vrp->num_rx_buf * rx_buf_align_size;
>>>
>>> Apply the same rule to cpu_addr in the vring descriptor:
>>>
>>> void *cpu_addr = vrp->rx_bufs + i * rx_buf_align_size;
>>>
>>> rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
>>>
>>> With this approach, the buffer addresses remain aligned
>>> independently of vdev_config::Rxbuf_size and vdev_config::txbuf_size.
>>> Don't hesitate if it is still not clear!
>>
>> How they remain aligned independent of tx/rx_buf_size? tx_bufs address
>> is still calculated based on rx_buf_align_size, so its alignment still
>> depends on rx_buf_align_size which is derived using
>> vdev_config::rpmsg_buf_align.>
>> I think we are trying to achive the same thing, but implementation is
>> differnt. We just need to decide where the alignment should be done?
>>
>> Either on the linux side? Or in the firmware resource table?
>>
>> I prefer that the firmware should already provide aligned buffer size,
>> and Linux should only check it. If alignment is not done, then simply
>> fail with error. That way, firmware also knows the correct size of the
>> buffer. If Linux does the alignment, then the firmware is not aware of
>> the correct size that is used by the linux.
>>
>> I am open to move the alignment operation to the linux side with the
>> reasonable justification.
> 
> That remains a suggestion. My main concern with the implementation is
> that RPMsg size should depend only on the max playlod size needed, not
> also on the memory alignment.

Okay, I think this is a good reason to apply alignment on the linux
side. If I understand correctly, the rpmsg buffer size will be used as
it is from the rsc table, but vdev_config::alignment will be used only
to decide the start address of the next buffer. If that is the
intention, then I agree, and I will refactor the patch accordingly.

> 
> If this constraint is kept, it must be imposed on all other non-Linux
> solutions. Otherwise, the remote implementation depends on the main
> processor implementation.
> 
> From my POV, It would be preferable not to impose such constraint when
> possible.
> 

Okay.

> Thanks,
> Arnaud
> 
>>
>> Thank You,
>> Tanmay
>>
>>>>
>>>>
>>>>>> +        if (rpmsg_buf_align &&
>>>>>> +            (!IS_ALIGNED(vrp->rx_buf_size, rpmsg_buf_align) ||
>>>>>> +             !IS_ALIGNED(vrp->tx_buf_size, rpmsg_buf_align))) {
>>>>>> +            dev_err(&vdev->dev,
>>>>>> +                "bad vdev config: buf sizes (rx %u, tx %u) not
>>>>>> aligned to %u\n",
>>>>>> +                vrp->rx_buf_size, vrp->tx_buf_size,
>>>>>> +                rpmsg_buf_align);
>>>>>> +            err = -EINVAL;
>>>>>> +            goto vqs_del;
>>>>>> +        }
>>>>>> +
>>>>>> +        dev_dbg(&vdev->dev,
>>>>>> +            "vdev config: ver=%u, align=0x%x, rx sz = 0x%x, tx sz =
>>>>>> 0x%x\n",
>>>>>> +            version, rpmsg_buf_align, 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->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,
>>>>>> @@ -874,15 +959,20 @@ static int rpmsg_probe(struct virtio_device
>>>>>> *vdev)
>>>>>>         /* first part of the buffers is dedicated for RX */
>>>>>>         vrp->rx_bufs = bufs_va;
>>>>>>     -    /* and second part is dedicated for TX */
>>>>>> -    vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>>>>> +    /*
>>>>>> +     * Here buf_va is aligned to a page. Also rx buf size is aligned
>>>>>> with
>>>>>> +     * cache line alignment provided by the firmware, so tx buf's
>>>>>> start
>>>>>> +     * address is guranteed to be aligned with the alignment
>>>>>> provided by
>>>>>> +     * the firmware.
>>>>>> +     */
>>>>>> +    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 +1055,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 +1082,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..7e14da68fd17
>>>>>> --- /dev/null
>>>>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
>>>>>> @@ -0,0 +1,50 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>>> +/*
>>>>>> + * Copyright (C) Pinecone Inc. 2019
>>>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>>>>> + * Copyright (C) Advanced Micro Devices, Inc. 2026
>>>>>> + */
>>>>>> +
>>>>>> +#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 */
>>>>>> +
>>>>>> +/* Version of struct virtio_rpmsg_config understood by this
>>>>>> driver */
>>>>>> +#define RPMSG_VDEV_CONFIG_V1    1
>>>>>> +
>>>>>> +/**
>>>>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
>>>>>> + *
>>>>>> + * @version:    version of this structure, currently
>>>>>> %RPMSG_VDEV_CONFIG_V1.
>>>>>> + * @reserved:    reserved for padding, must be zero.
>>>>>> + * @size:    size of this structure in bytes.
>>>>>> + * @rpmsg_buf_align:    required alignment in bytes for each buffer.
>>>>>> Must be a
>>>>>> + *        power of two so that both the buffer sizes and the TX
>>>>>> buffer
>>>>>> + *        base address can be aligned (e.g. to a cache line).
>>>>>> + * @reserved1:    reserved for padding, must be zero. Keeps the
>>>>>> following 32-bit
>>>>>> + *        fields naturally aligned.
>>>>>> + * @txbuf_size:    Tx buf size from remote's view. For Linux this is
>>>>>> rx buf size.
>>>>>> + * @rxbuf_size:    Rx buf size from remote's view. For Linux this is
>>>>>> tx buf size.
>>>>>> + *
>>>>>> + * This is the configuration structure shared by the device and the
>>>>>> driver,
>>>>>> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are
>>>>>> laid
>>>>>> out so
>>>>>> + * the structure is naturally 32-bit aligned.
>>>>>> + */
>>>>>> +struct virtio_rpmsg_config {
>>>>>> +    u8 version;
>>>>>> +    u8 reserved;
>>>>>
>>>>> Why about defining the version type to u16 to avoid the reserved
>>>>> field?
>>>>>
>>>>>> +    __virtio16 size;
>>>>>> +    __virtio16 rpmsg_buf_align;
>>>>>> +    __virtio16 reserved1;
>>>>>
>>>>> Seems useless if __packed prevents the compiler from inserting extra
>>>>> padding
>>>>> bytes between fields,
>>>>>
>>>>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>>>>> +    __virtio32 txbuf_size;
>>>>>> +    __virtio32 rxbuf_size;
>>>>>> +} __packed;
>>>>>
>>>>> proposal
>>>>>
>>>>> +struct virtio_rpmsg_config {
>>>>> +    __virtio16 version;
>>>>> +    __virtio16 size;
>>>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>>>> +    __virtio32 txbuf_size;
>>>>> +    __virtio32 rxbuf_size;
>>>>> +    __virtio16 rpmsg_buf_align;
>>>>> +} __packed;
>>>>> +
>>>>>
>>>>
>>>> I am okay with the above proposal with minor difference:
>>>>
>>>> My proposal:
>>>>
>>>> +struct virtio_rpmsg_config {
>>>> +    u8 version;
>>>> +    __virtio16 size;
>>>> +    __virtio16 rpmsg_buf_align;
>>>> +    /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */
>>>> +    __virtio32 txbuf_size;
>>>> +    __virtio32 rxbuf_size;
>>>> +} __packed;
>>>>
>>>> I just want to keep version field 8-bit, as we will probably never use
>>>> upper byte of that field if we use 16-bit. Rest is okay. If the
>>>> strucutre is packed then reserved bytes are not needed.
>>>>
>>>> Please let me know your view.
>>>
>>> No strong opinion on that. In the end, this structure is read only one
>>> time.
>>> If it is acceptable to Mathieu, it is acceptable to me.
>>>
>>> Thanks,
>>> Arnaud
>>>
>>>>
>>>> Thanks,
>>>> Tanmay
>>>>
>>>>
>>>>> Regards,
>>>>> Arnaud
>>>>>
>>>>>> +
>>>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>>>
>>>>
>>>
>>
>