From: Xiang Xiao <xiaoxiang781216@gmail.com>
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: Xiang Xiao <xiaoxiang@xiaomi.com>
Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
include/linux/virtio_rpmsg.h | 24 +++++++++++
2 files changed, 74 insertions(+), 18 deletions(-)
create mode 100644 include/linux/virtio_rpmsg.h
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index cc26dfcc3e29..03dd5535880a 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -25,6 +25,7 @@
#include <linux/sched.h>
#include <linux/virtio.h>
#include <linux/virtio_ids.h>
+#include <linux/virtio_rpmsg.h>
#include <linux/virtio_config.h>
#include <linux/wait.h>
@@ -39,7 +40,8 @@
* @sbufs: kernel address of tx buffers
* @num_rbufs: total number of buffers for rx
* @num_sbufs: total number of buffers for tx
- * @buf_size: size of one rx or tx buffer
+ * @rbuf_size: size of one rx buffer
+ * @sbuf_size: size of one tx buffer
* @last_sbuf: index of last tx buffer used
* @bufs_dma: dma base addr of the buffers
* @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders.
@@ -60,7 +62,8 @@ struct virtproc_info {
void *rbufs, *sbufs;
unsigned int num_rbufs;
unsigned int num_sbufs;
- unsigned int buf_size;
+ unsigned int rbuf_size;
+ unsigned int sbuf_size;
int last_sbuf;
dma_addr_t bufs_dma;
struct mutex tx_lock;
@@ -70,9 +73,6 @@ struct virtproc_info {
atomic_t sleepers;
};
-/* 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
@@ -130,7 +130,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.
@@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
/* either pick the next unused tx buffer */
if (vrp->last_sbuf < vrp->num_sbufs)
- ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
+ ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
/* or recycle a used one */
else
ret = virtqueue_get_buf(vrp->svq, &len);
@@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
dev_err(dev, "message is too big (%d)\n", len);
return -EMSGSIZE;
}
@@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
}
static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
@@ -706,7 +706,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->rbuf_size ||
msg_len > (len - sizeof(struct rpmsg_hdr))) {
dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
return -EINVAL;
@@ -739,7 +739,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->rbuf_size);
/* add the buffer back to the remote processor's virtqueue */
err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
@@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
else
vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
- vrp->buf_size = MAX_RPMSG_BUF_SIZE;
+ /*
+ * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
+ * size from virtio device config space from the resource table.
+ * If the feature is not supported, then assign default buf size.
+ */
+ if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
+ /* note: virtio_rpmsg_config is defined from remote view */
+ virtio_cread(vdev, struct virtio_rpmsg_config,
+ txbuf_size, &vrp->rbuf_size);
+ virtio_cread(vdev, struct virtio_rpmsg_config,
+ rxbuf_size, &vrp->sbuf_size);
+
+ /* The buffers must hold rpmsg header atleast */
+ if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
+ vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
+ dev_err(&vdev->dev,
+ "vdev config: rx buf sz = %d, tx buf sz = %d\n",
+ vrp->rbuf_size, vrp->sbuf_size);
+ err = -EINVAL;
+ goto vqs_del;
+ }
+
+ dev_dbg(&vdev->dev,
+ "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
+ vrp->rbuf_size, vrp->sbuf_size);
+ } else {
+ vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
+ vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
+ }
- total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
+ total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
+ (vrp->num_sbufs * vrp->sbuf_size);
+ total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
/* allocate coherent memory for the buffers */
bufs_va = dma_alloc_coherent(vdev->dev.parent,
@@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
vrp->rbufs = bufs_va;
/* and second part is dedicated for TX */
- vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
+ vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
/* set up the receive buffers */
for (i = 0; i < vrp->num_rbufs; i++) {
struct scatterlist sg;
- void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
+ void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
- rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
+ rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
GFP_KERNEL);
@@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
- size_t total_buf_space = num_bufs * vrp->buf_size;
+ size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
+ (vrp->num_sbufs * vrp->sbuf_size);
int ret;
virtio_reset_device(vdev);
@@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device *vdev)
vdev->config->del_vqs(vrp->vdev);
+ total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
dma_free_coherent(vdev->dev.parent, total_buf_space,
vrp->rbufs, vrp->bufs_dma);
@@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
new file mode 100644
index 000000000000..6406bc505383
--- /dev/null
+++ b/include/linux/virtio_rpmsg.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) Pinecone Inc. 2019
+ * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
+ */
+
+#ifndef _LINUX_VIRTIO_RPMSG_H
+#define _LINUX_VIRTIO_RPMSG_H
+
+#include <linux/types.h>
+
+/* The feature bitmap for virtio rpmsg */
+#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
+#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
+
+struct virtio_rpmsg_config {
+ /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
+ __u32 txbuf_size;
+ __u32 rxbuf_size;
+ __u32 reserved[14]; /* Reserve for the future use */
+ /* Put the customize config here */
+} __attribute__((packed));
+
+#endif /* _LINUX_VIRTIO_RPMSG_H */
--
2.34.1On 11/15/2025 2:46 AM, Tanmay Shah wrote:
> From: Xiang Xiao <xiaoxiang781216@gmail.com>
>
> 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: Xiang Xiao <xiaoxiang@xiaomi.com>
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
> include/linux/virtio_rpmsg.h | 24 +++++++++++
> 2 files changed, 74 insertions(+), 18 deletions(-)
> create mode 100644 include/linux/virtio_rpmsg.h
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index cc26dfcc3e29..03dd5535880a 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -25,6 +25,7 @@
> #include <linux/sched.h>
> #include <linux/virtio.h>
> #include <linux/virtio_ids.h>
> +#include <linux/virtio_rpmsg.h>
> #include <linux/virtio_config.h>
> #include <linux/wait.h>
>
> @@ -39,7 +40,8 @@
> * @sbufs: kernel address of tx buffers
> * @num_rbufs: total number of buffers for rx
> * @num_sbufs: total number of buffers for tx
> - * @buf_size: size of one rx or tx buffer
> + * @rbuf_size: size of one rx buffer
> + * @sbuf_size: size of one tx buffer
> * @last_sbuf: index of last tx buffer used
> * @bufs_dma: dma base addr of the buffers
> * @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders.
> @@ -60,7 +62,8 @@ struct virtproc_info {
> void *rbufs, *sbufs;
> unsigned int num_rbufs;
> unsigned int num_sbufs;
> - unsigned int buf_size;
> + unsigned int rbuf_size;
> + unsigned int sbuf_size;
> int last_sbuf;
> dma_addr_t bufs_dma;
> struct mutex tx_lock;
> @@ -70,9 +73,6 @@ struct virtproc_info {
> atomic_t sleepers;
> };
>
> -/* 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
> @@ -130,7 +130,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.
> @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>
> /* either pick the next unused tx buffer */
> if (vrp->last_sbuf < vrp->num_sbufs)
> - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> /* or recycle a used one */
> else
> ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
> dev_err(dev, "message is too big (%d)\n", len);
> return -EMSGSIZE;
> }
> @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
> }
>
> static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> @@ -706,7 +706,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->rbuf_size ||
> msg_len > (len - sizeof(struct rpmsg_hdr))) {
> dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
> return -EINVAL;
> @@ -739,7 +739,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->rbuf_size);
>
> /* add the buffer back to the remote processor's virtqueue */
> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
> else
> vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>
> - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> + /*
> + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
> + * size from virtio device config space from the resource table.
> + * If the feature is not supported, then assign default buf size.
> + */
> + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> + /* note: virtio_rpmsg_config is defined from remote view */
> + virtio_cread(vdev, struct virtio_rpmsg_config,
> + txbuf_size, &vrp->rbuf_size);
> + virtio_cread(vdev, struct virtio_rpmsg_config,
> + rxbuf_size, &vrp->sbuf_size);
> +
> + /* The buffers must hold rpmsg header atleast */
> + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
> + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
Hello Tanmay,
May I know if the omission of = here is to accommodate the ping/pong/ack
scenarios? mtu will 0
> + dev_err(&vdev->dev,
> + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
> + vrp->rbuf_size, vrp->sbuf_size);
> + err = -EINVAL;
> + goto vqs_del;
> + }
> +
> + dev_dbg(&vdev->dev,
> + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
> + vrp->rbuf_size, vrp->sbuf_size);
> + } else {
> + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> + }
>
> - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
> + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> + (vrp->num_sbufs * vrp->sbuf_size);
> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>
> /* allocate coherent memory for the buffers */
> bufs_va = dma_alloc_coherent(vdev->dev.parent,
> @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
> vrp->rbufs = bufs_va;
>
> /* and second part is dedicated for TX */
> - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
> + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
>
> /* set up the receive buffers */
> for (i = 0; i < vrp->num_rbufs; i++) {
> struct scatterlist sg;
> - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>
> - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>
> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> GFP_KERNEL);
> @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
> - size_t total_buf_space = num_bufs * vrp->buf_size;
> + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> + (vrp->num_sbufs * vrp->sbuf_size);
> int ret;
>
> virtio_reset_device(vdev);
> @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device *vdev)
>
> vdev->config->del_vqs(vrp->vdev);
>
> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
> dma_free_coherent(vdev->dev.parent, total_buf_space,
> vrp->rbufs, vrp->bufs_dma);
>
> @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
> new file mode 100644
> index 000000000000..6406bc505383
> --- /dev/null
> +++ b/include/linux/virtio_rpmsg.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
Echo Arnaud's comments. If it is intended for UAPI, please keep it in
include/uapi/linux
> +/*
> + * Copyright (C) Pinecone Inc. 2019
> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> + */
> +
> +#ifndef _LINUX_VIRTIO_RPMSG_H
> +#define _LINUX_VIRTIO_RPMSG_H
> +
> +#include <linux/types.h>
> +
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
May I know why skip bit 1?
> +
> +struct virtio_rpmsg_config {
> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> + __u32 txbuf_size;
> + __u32 rxbuf_size;
> + __u32 reserved[14]; /* Reserve for the future use */
Should we use __virtio32 instead of __u32 to avoid endianness issues?
> + /* Put the customize config here */
> +} __attribute__((packed));
> +
> +#endif /* _LINUX_VIRTIO_RPMSG_H */
--
Thx and BRs,
Zhongqiu Han
Hello,
Thanks for your reviews. Please find the response below.
On 11/22/25 6:05 AM, Zhongqiu Han wrote:
> On 11/15/2025 2:46 AM, Tanmay Shah wrote:
>> From: Xiang Xiao <xiaoxiang781216@gmail.com>
>>
>> 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: Xiang Xiao <xiaoxiang@xiaomi.com>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>> drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
>> include/linux/virtio_rpmsg.h | 24 +++++++++++
>> 2 files changed, 74 insertions(+), 18 deletions(-)
>> create mode 100644 include/linux/virtio_rpmsg.h
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>> virtio_rpmsg_bus.c
>> index cc26dfcc3e29..03dd5535880a 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -25,6 +25,7 @@
>> #include <linux/sched.h>
>> #include <linux/virtio.h>
>> #include <linux/virtio_ids.h>
>> +#include <linux/virtio_rpmsg.h>
>> #include <linux/virtio_config.h>
>> #include <linux/wait.h>
>> @@ -39,7 +40,8 @@
>> * @sbufs: kernel address of tx buffers
>> * @num_rbufs: total number of buffers for rx
>> * @num_sbufs: total number of buffers for tx
>> - * @buf_size: size of one rx or tx buffer
>> + * @rbuf_size: size of one rx buffer
>> + * @sbuf_size: size of one tx buffer
>> * @last_sbuf: index of last tx buffer used
>> * @bufs_dma: dma base addr of the buffers
>> * @tx_lock: protects svq, sbufs and sleepers, to allow
>> concurrent senders.
>> @@ -60,7 +62,8 @@ struct virtproc_info {
>> void *rbufs, *sbufs;
>> unsigned int num_rbufs;
>> unsigned int num_sbufs;
>> - unsigned int buf_size;
>> + unsigned int rbuf_size;
>> + unsigned int sbuf_size;
>> int last_sbuf;
>> dma_addr_t bufs_dma;
>> struct mutex tx_lock;
>> @@ -70,9 +73,6 @@ struct virtproc_info {
>> atomic_t sleepers;
>> };
>> -/* 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
>> @@ -130,7 +130,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.
>> @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>> /* either pick the next unused tx buffer */
>> if (vrp->last_sbuf < vrp->num_sbufs)
>> - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>> + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>> /* or recycle a used one */
>> else
>> ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
>> dev_err(dev, "message is too big (%d)\n", len);
>> return -EMSGSIZE;
>> }
>> @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
>> }
>> static int rpmsg_recv_single(struct virtproc_info *vrp, struct
>> device *dev,
>> @@ -706,7 +706,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->rbuf_size ||
>> msg_len > (len - sizeof(struct rpmsg_hdr))) {
>> dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
>> return -EINVAL;
>> @@ -739,7 +739,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->rbuf_size);
>> /* add the buffer back to the remote processor's virtqueue */
>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>> @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> else
>> vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>> - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>> + /*
>> + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>> + * size from virtio device config space from the resource table.
>> + * If the feature is not supported, then assign default buf size.
>> + */
>> + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>> + /* note: virtio_rpmsg_config is defined from remote view */
>> + virtio_cread(vdev, struct virtio_rpmsg_config,
>> + txbuf_size, &vrp->rbuf_size);
>> + virtio_cread(vdev, struct virtio_rpmsg_config,
>> + rxbuf_size, &vrp->sbuf_size);
>> +
>> + /* The buffers must hold rpmsg header atleast */
>> + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
>> + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
>
>
> Hello Tanmay,
>
> May I know if the omission of = here is to accommodate the ping/pong/ack
> scenarios? mtu will 0
>
>
Yes. At minimum RPMsg header is needed to ping the correct endpoint. We
don't need to have any payload attached to the packet. MTU will be
sizeof rpmsg_hdr I think.
>> + dev_err(&vdev->dev,
>> + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
>> + vrp->rbuf_size, vrp->sbuf_size);
>> + err = -EINVAL;
>> + goto vqs_del;
>> + }
>> +
>> + dev_dbg(&vdev->dev,
>> + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
>> + vrp->rbuf_size, vrp->sbuf_size);
>> + } else {
>> + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
>> + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
>> + }
>> - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
>> + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
>> + (vrp->num_sbufs * vrp->sbuf_size);
>> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>> /* allocate coherent memory for the buffers */
>> bufs_va = dma_alloc_coherent(vdev->dev.parent,
>> @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> vrp->rbufs = bufs_va;
>> /* and second part is dedicated for TX */
>> - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
>> + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
>> /* set up the receive buffers */
>> for (i = 0; i < vrp->num_rbufs; i++) {
>> struct scatterlist sg;
>> - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>> + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>> - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>> + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>> GFP_KERNEL);
>> @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
>> - size_t total_buf_space = num_bufs * vrp->buf_size;
>> + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
>> + (vrp->num_sbufs * vrp->sbuf_size);
>> int ret;
>> virtio_reset_device(vdev);
>> @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device
>> *vdev)
>> vdev->config->del_vqs(vrp->vdev);
>> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>> dma_free_coherent(vdev->dev.parent, total_buf_space,
>> vrp->rbufs, vrp->bufs_dma);
>> @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
>> new file mode 100644
>> index 000000000000..6406bc505383
>> --- /dev/null
>> +++ b/include/linux/virtio_rpmsg.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>
>
> Echo Arnaud's comments. If it is intended for UAPI, please keep it in
> include/uapi/linux
>
>
It's not intended for UAPI. I need to fix the license. I will check
other virtio headers in the same directory and fix the license accordingly.
>> +/*
>> + * Copyright (C) Pinecone Inc. 2019
>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>> + */
>> +
>> +#ifndef _LINUX_VIRTIO_RPMSG_H
>> +#define _LINUX_VIRTIO_RPMSG_H
>> +
>> +#include <linux/types.h>
>> +
>> +/* The feature bitmap for virtio rpmsg */
>> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
>> notifications */
>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config
>> space */
>
> May I know why skip bit 1?
>
>
Thanks, that's a good question. I keept id 2 unmodified from the
original series. I don't know why ID 2 was chosen in the original
series. I will have to discuss this with the linux remoteproc/rpmsg
maintainers and choose the correct ID.
I don't see any problem choosing ID 1, but for some reason if ID 1 was
assigned and deprecated (I don't think that is the case) then only we
should use ID 2.
Arnaud, Mathieu, Bjorn any input here?
>> +
>> +struct virtio_rpmsg_config {
>> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>> + __u32 txbuf_size;
>> + __u32 rxbuf_size;
>> + __u32 reserved[14]; /* Reserve for the future use */
>
> Should we use __virtio32 instead of __u32 to avoid endianness issues?
>
>
Sure, if that is the standard in other virtio headers I will modify it.
Thanks,
Tanmay
>> + /* Put the customize config here */
>> +} __attribute__((packed));
>> +
>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>
>
Hi,
On 12/3/25 19:12, Tanmay Shah wrote:
> Hello,
>
> Thanks for your reviews. Please find the response below.
>
> On 11/22/25 6:05 AM, Zhongqiu Han wrote:
>> On 11/15/2025 2:46 AM, Tanmay Shah wrote:
>>> From: Xiang Xiao <xiaoxiang781216@gmail.com>
>>>
>>> 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: Xiang Xiao <xiaoxiang@xiaomi.com>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>> drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
>>> include/linux/virtio_rpmsg.h | 24 +++++++++++
>>> 2 files changed, 74 insertions(+), 18 deletions(-)
>>> create mode 100644 include/linux/virtio_rpmsg.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>>> virtio_rpmsg_bus.c
>>> index cc26dfcc3e29..03dd5535880a 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/sched.h>
>>> #include <linux/virtio.h>
>>> #include <linux/virtio_ids.h>
>>> +#include <linux/virtio_rpmsg.h>
>>> #include <linux/virtio_config.h>
>>> #include <linux/wait.h>
>>> @@ -39,7 +40,8 @@
>>> * @sbufs: kernel address of tx buffers
>>> * @num_rbufs: total number of buffers for rx
>>> * @num_sbufs: total number of buffers for tx
>>> - * @buf_size: size of one rx or tx buffer
>>> + * @rbuf_size: size of one rx buffer
>>> + * @sbuf_size: size of one tx buffer
>>> * @last_sbuf: index of last tx buffer used
>>> * @bufs_dma: dma base addr of the buffers
>>> * @tx_lock: protects svq, sbufs and sleepers, to allow
>>> concurrent senders.
>>> @@ -60,7 +62,8 @@ struct virtproc_info {
>>> void *rbufs, *sbufs;
>>> unsigned int num_rbufs;
>>> unsigned int num_sbufs;
>>> - unsigned int buf_size;
>>> + unsigned int rbuf_size;
>>> + unsigned int sbuf_size;
>>> int last_sbuf;
>>> dma_addr_t bufs_dma;
>>> struct mutex tx_lock;
>>> @@ -70,9 +73,6 @@ struct virtproc_info {
>>> atomic_t sleepers;
>>> };
>>> -/* 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
>>> @@ -130,7 +130,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.
>>> @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>> /* either pick the next unused tx buffer */
>>> if (vrp->last_sbuf < vrp->num_sbufs)
>>> - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>>> + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>>> /* or recycle a used one */
>>> else
>>> ret = virtqueue_get_buf(vrp->svq, &len);
>>> @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
>>> dev_err(dev, "message is too big (%d)\n", len);
>>> return -EMSGSIZE;
>>> }
>>> @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
>>> }
>>> static int rpmsg_recv_single(struct virtproc_info *vrp, struct
>>> device *dev,
>>> @@ -706,7 +706,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->rbuf_size ||
>>> msg_len > (len - sizeof(struct rpmsg_hdr))) {
>>> dev_warn(dev, "inbound msg too big: (%d, %d)\n", len,
>>> msg_len);
>>> return -EINVAL;
>>> @@ -739,7 +739,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->rbuf_size);
>>> /* add the buffer back to the remote processor's virtqueue */
>>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>> @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> else
>>> vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>>> - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>> + /*
>>> + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>>> + * size from virtio device config space from the resource table.
>>> + * If the feature is not supported, then assign default buf size.
>>> + */
>>> + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>> + /* note: virtio_rpmsg_config is defined from remote view */
>>> + virtio_cread(vdev, struct virtio_rpmsg_config,
>>> + txbuf_size, &vrp->rbuf_size);
>>> + virtio_cread(vdev, struct virtio_rpmsg_config,
>>> + rxbuf_size, &vrp->sbuf_size);
>>> +
>>> + /* The buffers must hold rpmsg header atleast */
>>> + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
>>> + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
>>
>>
>> Hello Tanmay,
>>
>> May I know if the omission of = here is to accommodate the ping/pong/ack
>> scenarios? mtu will 0
>>
>>
>
> Yes. At minimum RPMsg header is needed to ping the correct endpoint. We
> don't need to have any payload attached to the packet. MTU will be
> sizeof rpmsg_hdr I think.
>
>>> + dev_err(&vdev->dev,
>>> + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
>>> + vrp->rbuf_size, vrp->sbuf_size);
>>> + err = -EINVAL;
>>> + goto vqs_del;
>>> + }
>>> +
>>> + dev_dbg(&vdev->dev,
>>> + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
>>> + vrp->rbuf_size, vrp->sbuf_size);
>>> + } else {
>>> + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
>>> + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
>>> + }
>>> - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp-
>>> >buf_size;
>>> + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
>>> + (vrp->num_sbufs * vrp->sbuf_size);
>>> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>>> /* allocate coherent memory for the buffers */
>>> bufs_va = dma_alloc_coherent(vdev->dev.parent,
>>> @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> vrp->rbufs = bufs_va;
>>> /* and second part is dedicated for TX */
>>> - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
>>> + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
>>> /* set up the receive buffers */
>>> for (i = 0; i < vrp->num_rbufs; i++) {
>>> struct scatterlist sg;
>>> - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>> + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>>> - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>> + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>> GFP_KERNEL);
>>> @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
>>> - size_t total_buf_space = num_bufs * vrp->buf_size;
>>> + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
>>> + (vrp->num_sbufs * vrp->sbuf_size);
>>> int ret;
>>> virtio_reset_device(vdev);
>>> @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device
>>> *vdev)
>>> vdev->config->del_vqs(vrp->vdev);
>>> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>>> dma_free_coherent(vdev->dev.parent, total_buf_space,
>>> vrp->rbufs, vrp->bufs_dma);
>>> @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
>>> new file mode 100644
>>> index 000000000000..6406bc505383
>>> --- /dev/null
>>> +++ b/include/linux/virtio_rpmsg.h
>>> @@ -0,0 +1,24 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>
>>
>> Echo Arnaud's comments. If it is intended for UAPI, please keep it in
>> include/uapi/linux
>>
>>
>
> It's not intended for UAPI. I need to fix the license. I will check
> other virtio headers in the same directory and fix the license accordingly.
>
>>> +/*
>>> + * Copyright (C) Pinecone Inc. 2019
>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>> + */
>>> +
>>> +#ifndef _LINUX_VIRTIO_RPMSG_H
>>> +#define _LINUX_VIRTIO_RPMSG_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +/* The feature bitmap for virtio rpmsg */
>>> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
>>> notifications */
>>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config
>>> space */
>>
>> May I know why skip bit 1?
>>
>>
>
> Thanks, that's a good question. I keept id 2 unmodified from the
> original series. I don't know why ID 2 was chosen in the original
> series. I will have to discuss this with the linux remoteproc/rpmsg
> maintainers and choose the correct ID.
>
> I don't see any problem choosing ID 1, but for some reason if ID 1 was
> assigned and deprecated (I don't think that is the case) then only we
> should use ID 2.
The ID 1 was proposed in an openamp PR [1]. If we
enter VIRTIO_RPMSG_F_BUFSZ first it makes sense to set its ID to 1.
[1]https://github.com/OpenAMP/open-amp/pull/160/commits/d4a13128f94e46180285c05a20da78fdca54f7d7
Regards,
Arnaud
>
>
> Arnaud, Mathieu, Bjorn any input here?
>
>>> +
>>> +struct virtio_rpmsg_config {
>>> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>> + __u32 txbuf_size;
>>> + __u32 rxbuf_size;
>>> + __u32 reserved[14]; /* Reserve for the future use */
>>
>> Should we use __virtio32 instead of __u32 to avoid endianness issues?
>>
>>
>
> Sure, if that is the standard in other virtio headers I will modify it.
>
> Thanks,
> Tanmay
>
>>> + /* Put the customize config here */
>>> +} __attribute__((packed));
>>> +
>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>
>>
>
On Fri, Dec 05, 2025 at 05:01:17PM +0100, Arnaud POULIQUEN wrote:
> Hi,
>
> On 12/3/25 19:12, Tanmay Shah wrote:
> > Hello,
> >
> > Thanks for your reviews. Please find the response below.
> >
> > On 11/22/25 6:05 AM, Zhongqiu Han wrote:
> > > On 11/15/2025 2:46 AM, Tanmay Shah wrote:
> > > > From: Xiang Xiao <xiaoxiang781216@gmail.com>
> > > >
> > > > 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: Xiang Xiao <xiaoxiang@xiaomi.com>
> > > > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> > > > ---
> > > > drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
> > > > include/linux/virtio_rpmsg.h | 24 +++++++++++
> > > > 2 files changed, 74 insertions(+), 18 deletions(-)
> > > > create mode 100644 include/linux/virtio_rpmsg.h
> > > >
> > > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
> > > > virtio_rpmsg_bus.c
> > > > index cc26dfcc3e29..03dd5535880a 100644
> > > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > > @@ -25,6 +25,7 @@
> > > > #include <linux/sched.h>
> > > > #include <linux/virtio.h>
> > > > #include <linux/virtio_ids.h>
> > > > +#include <linux/virtio_rpmsg.h>
> > > > #include <linux/virtio_config.h>
> > > > #include <linux/wait.h>
> > > > @@ -39,7 +40,8 @@
> > > > * @sbufs: kernel address of tx buffers
> > > > * @num_rbufs: total number of buffers for rx
> > > > * @num_sbufs: total number of buffers for tx
> > > > - * @buf_size: size of one rx or tx buffer
> > > > + * @rbuf_size: size of one rx buffer
> > > > + * @sbuf_size: size of one tx buffer
> > > > * @last_sbuf: index of last tx buffer used
> > > > * @bufs_dma: dma base addr of the buffers
> > > > * @tx_lock: protects svq, sbufs and sleepers, to allow
> > > > concurrent senders.
> > > > @@ -60,7 +62,8 @@ struct virtproc_info {
> > > > void *rbufs, *sbufs;
> > > > unsigned int num_rbufs;
> > > > unsigned int num_sbufs;
> > > > - unsigned int buf_size;
> > > > + unsigned int rbuf_size;
> > > > + unsigned int sbuf_size;
> > > > int last_sbuf;
> > > > dma_addr_t bufs_dma;
> > > > struct mutex tx_lock;
> > > > @@ -70,9 +73,6 @@ struct virtproc_info {
> > > > atomic_t sleepers;
> > > > };
> > > > -/* 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
> > > > @@ -130,7 +130,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.
> > > > @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> > > > /* either pick the next unused tx buffer */
> > > > if (vrp->last_sbuf < vrp->num_sbufs)
> > > > - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> > > > + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> > > > /* or recycle a used one */
> > > > else
> > > > ret = virtqueue_get_buf(vrp->svq, &len);
> > > > @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
> > > > dev_err(dev, "message is too big (%d)\n", len);
> > > > return -EMSGSIZE;
> > > > }
> > > > @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
> > > > }
> > > > static int rpmsg_recv_single(struct virtproc_info *vrp, struct
> > > > device *dev,
> > > > @@ -706,7 +706,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->rbuf_size ||
> > > > msg_len > (len - sizeof(struct rpmsg_hdr))) {
> > > > dev_warn(dev, "inbound msg too big: (%d, %d)\n", len,
> > > > msg_len);
> > > > return -EINVAL;
> > > > @@ -739,7 +739,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->rbuf_size);
> > > > /* add the buffer back to the remote processor's virtqueue */
> > > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> > > > @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > > else
> > > > vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
> > > > - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> > > > + /*
> > > > + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
> > > > + * size from virtio device config space from the resource table.
> > > > + * If the feature is not supported, then assign default buf size.
> > > > + */
> > > > + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> > > > + /* note: virtio_rpmsg_config is defined from remote view */
> > > > + virtio_cread(vdev, struct virtio_rpmsg_config,
> > > > + txbuf_size, &vrp->rbuf_size);
> > > > + virtio_cread(vdev, struct virtio_rpmsg_config,
> > > > + rxbuf_size, &vrp->sbuf_size);
> > > > +
> > > > + /* The buffers must hold rpmsg header atleast */
> > > > + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
> > > > + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
> > >
> > >
> > > Hello Tanmay,
> > >
> > > May I know if the omission of = here is to accommodate the ping/pong/ack
> > > scenarios? mtu will 0
> > >
> > >
> >
> > Yes. At minimum RPMsg header is needed to ping the correct endpoint. We
> > don't need to have any payload attached to the packet. MTU will be
> > sizeof rpmsg_hdr I think.
> >
> > > > + dev_err(&vdev->dev,
> > > > + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
> > > > + vrp->rbuf_size, vrp->sbuf_size);
> > > > + err = -EINVAL;
> > > > + goto vqs_del;
> > > > + }
> > > > +
> > > > + dev_dbg(&vdev->dev,
> > > > + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
> > > > + vrp->rbuf_size, vrp->sbuf_size);
> > > > + } else {
> > > > + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> > > > + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> > > > + }
> > > > - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp-
> > > > >buf_size;
> > > > + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> > > > + (vrp->num_sbufs * vrp->sbuf_size);
> > > > + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
> > > > /* allocate coherent memory for the buffers */
> > > > bufs_va = dma_alloc_coherent(vdev->dev.parent,
> > > > @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > > vrp->rbufs = bufs_va;
> > > > /* and second part is dedicated for TX */
> > > > - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
> > > > + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
> > > > /* set up the receive buffers */
> > > > for (i = 0; i < vrp->num_rbufs; i++) {
> > > > struct scatterlist sg;
> > > > - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> > > > + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
> > > > - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> > > > + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
> > > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> > > > GFP_KERNEL);
> > > > @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
> > > > - size_t total_buf_space = num_bufs * vrp->buf_size;
> > > > + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> > > > + (vrp->num_sbufs * vrp->sbuf_size);
> > > > int ret;
> > > > virtio_reset_device(vdev);
> > > > @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct
> > > > virtio_device *vdev)
> > > > vdev->config->del_vqs(vrp->vdev);
> > > > + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
> > > > dma_free_coherent(vdev->dev.parent, total_buf_space,
> > > > vrp->rbufs, vrp->bufs_dma);
> > > > @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
> > > > new file mode 100644
> > > > index 000000000000..6406bc505383
> > > > --- /dev/null
> > > > +++ b/include/linux/virtio_rpmsg.h
> > > > @@ -0,0 +1,24 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > >
> > >
> > > Echo Arnaud's comments. If it is intended for UAPI, please keep it in
> > > include/uapi/linux
> > >
> > >
> >
> > It's not intended for UAPI. I need to fix the license. I will check
> > other virtio headers in the same directory and fix the license
> > accordingly.
> >
> > > > +/*
> > > > + * Copyright (C) Pinecone Inc. 2019
> > > > + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> > > > + */
> > > > +
> > > > +#ifndef _LINUX_VIRTIO_RPMSG_H
> > > > +#define _LINUX_VIRTIO_RPMSG_H
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +/* The feature bitmap for virtio rpmsg */
> > > > +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
> > > > notifications */
> > > > +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from
> > > > config space */
> > >
> > > May I know why skip bit 1?
> > >
> > >
> >
> > Thanks, that's a good question. I keept id 2 unmodified from the
> > original series. I don't know why ID 2 was chosen in the original
> > series. I will have to discuss this with the linux remoteproc/rpmsg
> > maintainers and choose the correct ID.
> >
> > I don't see any problem choosing ID 1, but for some reason if ID 1 was
> > assigned and deprecated (I don't think that is the case) then only we
> > should use ID 2.
>
>
> The ID 1 was proposed in an openamp PR [1]. If we
> enter VIRTIO_RPMSG_F_BUFSZ first it makes sense to set its ID to 1.
>
I agree.
> [1]https://github.com/OpenAMP/open-amp/pull/160/commits/d4a13128f94e46180285c05a20da78fdca54f7d7
>
>
> Regards,
> Arnaud
>
> >
> >
> > Arnaud, Mathieu, Bjorn any input here?
> >
> > > > +
> > > > +struct virtio_rpmsg_config {
> > > > + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> > > > + __u32 txbuf_size;
> > > > + __u32 rxbuf_size;
> > > > + __u32 reserved[14]; /* Reserve for the future use */
> > >
> > > Should we use __virtio32 instead of __u32 to avoid endianness issues?
> > >
> > >
> >
> > Sure, if that is the standard in other virtio headers I will modify it.
> >
> > Thanks,
> > Tanmay
> >
> > > > + /* Put the customize config here */
> > > > +} __attribute__((packed));
> > > > +
> > > > +#endif /* _LINUX_VIRTIO_RPMSG_H */
> > >
> > >
> >
>
On Wed, Dec 03, 2025 at 12:12:46PM -0600, Tanmay Shah wrote:
> Hello,
>
> Thanks for your reviews. Please find the response below.
>
> On 11/22/25 6:05 AM, Zhongqiu Han wrote:
> > On 11/15/2025 2:46 AM, Tanmay Shah wrote:
> > > From: Xiang Xiao <xiaoxiang781216@gmail.com>
> > >
> > > 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: Xiang Xiao <xiaoxiang@xiaomi.com>
> > > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> > > ---
> > > drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
> > > include/linux/virtio_rpmsg.h | 24 +++++++++++
> > > 2 files changed, 74 insertions(+), 18 deletions(-)
> > > create mode 100644 include/linux/virtio_rpmsg.h
> > >
> > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
> > > virtio_rpmsg_bus.c
> > > index cc26dfcc3e29..03dd5535880a 100644
> > > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/sched.h>
> > > #include <linux/virtio.h>
> > > #include <linux/virtio_ids.h>
> > > +#include <linux/virtio_rpmsg.h>
> > > #include <linux/virtio_config.h>
> > > #include <linux/wait.h>
> > > @@ -39,7 +40,8 @@
> > > * @sbufs: kernel address of tx buffers
> > > * @num_rbufs: total number of buffers for rx
> > > * @num_sbufs: total number of buffers for tx
> > > - * @buf_size: size of one rx or tx buffer
> > > + * @rbuf_size: size of one rx buffer
> > > + * @sbuf_size: size of one tx buffer
> > > * @last_sbuf: index of last tx buffer used
> > > * @bufs_dma: dma base addr of the buffers
> > > * @tx_lock: protects svq, sbufs and sleepers, to allow
> > > concurrent senders.
> > > @@ -60,7 +62,8 @@ struct virtproc_info {
> > > void *rbufs, *sbufs;
> > > unsigned int num_rbufs;
> > > unsigned int num_sbufs;
> > > - unsigned int buf_size;
> > > + unsigned int rbuf_size;
> > > + unsigned int sbuf_size;
> > > int last_sbuf;
> > > dma_addr_t bufs_dma;
> > > struct mutex tx_lock;
> > > @@ -70,9 +73,6 @@ struct virtproc_info {
> > > atomic_t sleepers;
> > > };
> > > -/* 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
> > > @@ -130,7 +130,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.
> > > @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> > > /* either pick the next unused tx buffer */
> > > if (vrp->last_sbuf < vrp->num_sbufs)
> > > - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> > > + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> > > /* or recycle a used one */
> > > else
> > > ret = virtqueue_get_buf(vrp->svq, &len);
> > > @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
> > > dev_err(dev, "message is too big (%d)\n", len);
> > > return -EMSGSIZE;
> > > }
> > > @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
> > > }
> > > static int rpmsg_recv_single(struct virtproc_info *vrp, struct
> > > device *dev,
> > > @@ -706,7 +706,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->rbuf_size ||
> > > msg_len > (len - sizeof(struct rpmsg_hdr))) {
> > > dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
> > > return -EINVAL;
> > > @@ -739,7 +739,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->rbuf_size);
> > > /* add the buffer back to the remote processor's virtqueue */
> > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> > > @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > else
> > > vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
> > > - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> > > + /*
> > > + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
> > > + * size from virtio device config space from the resource table.
> > > + * If the feature is not supported, then assign default buf size.
> > > + */
> > > + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> > > + /* note: virtio_rpmsg_config is defined from remote view */
> > > + virtio_cread(vdev, struct virtio_rpmsg_config,
> > > + txbuf_size, &vrp->rbuf_size);
> > > + virtio_cread(vdev, struct virtio_rpmsg_config,
> > > + rxbuf_size, &vrp->sbuf_size);
> > > +
> > > + /* The buffers must hold rpmsg header atleast */
> > > + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
> > > + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
> >
> >
> > Hello Tanmay,
> >
> > May I know if the omission of = here is to accommodate the ping/pong/ack
> > scenarios? mtu will 0
> >
> >
>
> Yes. At minimum RPMsg header is needed to ping the correct endpoint. We
> don't need to have any payload attached to the packet. MTU will be sizeof
> rpmsg_hdr I think.
>
> > > + dev_err(&vdev->dev,
> > > + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
> > > + vrp->rbuf_size, vrp->sbuf_size);
> > > + err = -EINVAL;
> > > + goto vqs_del;
> > > + }
> > > +
> > > + dev_dbg(&vdev->dev,
> > > + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
> > > + vrp->rbuf_size, vrp->sbuf_size);
> > > + } else {
> > > + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> > > + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> > > + }
> > > - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
> > > + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> > > + (vrp->num_sbufs * vrp->sbuf_size);
> > > + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
> > > /* allocate coherent memory for the buffers */
> > > bufs_va = dma_alloc_coherent(vdev->dev.parent,
> > > @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > > vrp->rbufs = bufs_va;
> > > /* and second part is dedicated for TX */
> > > - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
> > > + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
> > > /* set up the receive buffers */
> > > for (i = 0; i < vrp->num_rbufs; i++) {
> > > struct scatterlist sg;
> > > - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> > > + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
> > > - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> > > + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
> > > err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> > > GFP_KERNEL);
> > > @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
> > > - size_t total_buf_space = num_bufs * vrp->buf_size;
> > > + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> > > + (vrp->num_sbufs * vrp->sbuf_size);
> > > int ret;
> > > virtio_reset_device(vdev);
> > > @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device
> > > *vdev)
> > > vdev->config->del_vqs(vrp->vdev);
> > > + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
> > > dma_free_coherent(vdev->dev.parent, total_buf_space,
> > > vrp->rbufs, vrp->bufs_dma);
> > > @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
> > > new file mode 100644
> > > index 000000000000..6406bc505383
> > > --- /dev/null
> > > +++ b/include/linux/virtio_rpmsg.h
> > > @@ -0,0 +1,24 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >
> >
> > Echo Arnaud's comments. If it is intended for UAPI, please keep it in
> > include/uapi/linux
> >
> >
>
> It's not intended for UAPI. I need to fix the license. I will check other
> virtio headers in the same directory and fix the license accordingly.
>
> > > +/*
> > > + * Copyright (C) Pinecone Inc. 2019
> > > + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> > > + */
> > > +
> > > +#ifndef _LINUX_VIRTIO_RPMSG_H
> > > +#define _LINUX_VIRTIO_RPMSG_H
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/* The feature bitmap for virtio rpmsg */
> > > +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
> > > notifications */
> > > +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config
> > > space */
> >
> > May I know why skip bit 1?
> >
> >
>
> Thanks, that's a good question. I keept id 2 unmodified from the original
> series. I don't know why ID 2 was chosen in the original series. I will have
> to discuss this with the linux remoteproc/rpmsg maintainers and choose the
> correct ID.
>
> I don't see any problem choosing ID 1, but for some reason if ID 1 was
> assigned and deprecated (I don't think that is the case) then only we should
> use ID 2.
>
>
> Arnaud, Mathieu, Bjorn any input here?
>
> > > +
> > > +struct virtio_rpmsg_config {
> > > + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> > > + __u32 txbuf_size;
> > > + __u32 rxbuf_size;
> > > + __u32 reserved[14]; /* Reserve for the future use */
> >
> > Should we use __virtio32 instead of __u32 to avoid endianness issues?
> >
> >
>
> Sure, if that is the standard in other virtio headers I will modify it.
>
> Thanks,
> Tanmay
rpmsg is still not standardized, sadly. It's really time it was.
Modern virtio devices use __le32.
Accordingly, accessed with virtio_cread_le
__virtioXX and virtio_cread are for legacy compatible parts of config space.
Does rpmsg want to be modern or keep using legacy? I donnu.
Ideally it should finally be documented and at that point we
definitely will want to switch to __le32.
For now, run sparse to make sure you don't introduce new endian-ness
issues.
> > > + /* Put the customize config here */
> > > +} __attribute__((packed));
> > > +
> > > +#endif /* _LINUX_VIRTIO_RPMSG_H */
> >
> >
On 12/3/25 1:35 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 03, 2025 at 12:12:46PM -0600, Tanmay Shah wrote:
>> Hello,
>>
>> Thanks for your reviews. Please find the response below.
>>
>> On 11/22/25 6:05 AM, Zhongqiu Han wrote:
>>> On 11/15/2025 2:46 AM, Tanmay Shah wrote:
>>>> From: Xiang Xiao <xiaoxiang781216@gmail.com>
>>>>
[...]
>>
>> Thanks, that's a good question. I keept id 2 unmodified from the original
>> series. I don't know why ID 2 was chosen in the original series. I will have
>> to discuss this with the linux remoteproc/rpmsg maintainers and choose the
>> correct ID.
>>
>> I don't see any problem choosing ID 1, but for some reason if ID 1 was
>> assigned and deprecated (I don't think that is the case) then only we should
>> use ID 2.
>>
>>
>> Arnaud, Mathieu, Bjorn any input here?
>>
>>>> +
>>>> +struct virtio_rpmsg_config {
>>>> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>> + __u32 txbuf_size;
>>>> + __u32 rxbuf_size;
>>>> + __u32 reserved[14]; /* Reserve for the future use */
>>>
>>> Should we use __virtio32 instead of __u32 to avoid endianness issues?
>>>
>>>
>>
>> Sure, if that is the standard in other virtio headers I will modify it.
>>
>> Thanks,
>> Tanmay
>
> rpmsg is still not standardized, sadly. It's really time it was.
>
>
>
> Modern virtio devices use __le32.
> Accordingly, accessed with virtio_cread_le
>
>
> __virtioXX and virtio_cread are for legacy compatible parts of config space.
>
As of now, I am using virtio_cread so I think it is legacy compatible.
>
> Does rpmsg want to be modern or keep using legacy? I donnu.
>
I don't know either. This we have to discuss with the maintainers and
other vendors.
> Ideally it should finally be documented and at that point we
> definitely will want to switch to __le32.
>
>
> For now, run sparse to make sure you don't introduce new endian-ness
> issues.
Ack.
>
>
>>>> + /* Put the customize config here */
>>>> +} __attribute__((packed));
>>>> +
>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>>>
>>>
>
On Thu, Dec 04, 2025 at 10:55:10AM -0600, Tanmay Shah wrote:
>
>
> On 12/3/25 1:35 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 03, 2025 at 12:12:46PM -0600, Tanmay Shah wrote:
> > > Hello,
> > >
> > > Thanks for your reviews. Please find the response below.
> > >
> > > On 11/22/25 6:05 AM, Zhongqiu Han wrote:
> > > > On 11/15/2025 2:46 AM, Tanmay Shah wrote:
> > > > > From: Xiang Xiao <xiaoxiang781216@gmail.com>
> > > > >
>
> [...]
>
> > >
> > > Thanks, that's a good question. I keept id 2 unmodified from the original
> > > series. I don't know why ID 2 was chosen in the original series. I will have
> > > to discuss this with the linux remoteproc/rpmsg maintainers and choose the
> > > correct ID.
> > >
> > > I don't see any problem choosing ID 1, but for some reason if ID 1 was
> > > assigned and deprecated (I don't think that is the case) then only we should
> > > use ID 2.
> > >
> > >
> > > Arnaud, Mathieu, Bjorn any input here?
> > >
> > > > > +
> > > > > +struct virtio_rpmsg_config {
> > > > > + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> > > > > + __u32 txbuf_size;
> > > > > + __u32 rxbuf_size;
> > > > > + __u32 reserved[14]; /* Reserve for the future use */
> > > >
> > > > Should we use __virtio32 instead of __u32 to avoid endianness issues?
> > > >
> > > >
> > >
> > > Sure, if that is the standard in other virtio headers I will modify it.
> > >
> > > Thanks,
> > > Tanmay
> >
> > rpmsg is still not standardized, sadly. It's really time it was.
> >
> >
> >
> > Modern virtio devices use __le32.
> > Accordingly, accessed with virtio_cread_le
> >
> >
> > __virtioXX and virtio_cread are for legacy compatible parts of config space.
> >
>
> As of now, I am using virtio_cread so I think it is legacy compatible.
>
> >
> > Does rpmsg want to be modern or keep using legacy? I donnu.
> >
>
> I don't know either. This we have to discuss with the maintainers and other
> vendors.
>
I would certainly like to see a modernization effort, but it needs to be
backward compatible. Probably outside the scope of this set though.
>
> > Ideally it should finally be documented and at that point we
> > definitely will want to switch to __le32.
> >
> >
> > For now, run sparse to make sure you don't introduce new endian-ness
> > issues.
>
> Ack.
>
> >
> >
>
>
>
> > > > > + /* Put the customize config here */
> > > > > +} __attribute__((packed));
> > > > > +
> > > > > +#endif /* _LINUX_VIRTIO_RPMSG_H */
> > > >
> > > >
> >
>
On 11/14/25 19:46, Tanmay Shah wrote:
> From: Xiang Xiao <xiaoxiang781216@gmail.com>
>
> 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: Xiang Xiao <xiaoxiang@xiaomi.com>
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
> include/linux/virtio_rpmsg.h | 24 +++++++++++
> 2 files changed, 74 insertions(+), 18 deletions(-)
> create mode 100644 include/linux/virtio_rpmsg.h
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index cc26dfcc3e29..03dd5535880a 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -25,6 +25,7 @@
> #include <linux/sched.h>
> #include <linux/virtio.h>
> #include <linux/virtio_ids.h>
> +#include <linux/virtio_rpmsg.h>
> #include <linux/virtio_config.h>
> #include <linux/wait.h>
>
> @@ -39,7 +40,8 @@
> * @sbufs: kernel address of tx buffers
> * @num_rbufs: total number of buffers for rx
> * @num_sbufs: total number of buffers for tx
> - * @buf_size: size of one rx or tx buffer
> + * @rbuf_size: size of one rx buffer
> + * @sbuf_size: size of one tx buffer
> * @last_sbuf: index of last tx buffer used
> * @bufs_dma: dma base addr of the buffers
> * @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders.
> @@ -60,7 +62,8 @@ struct virtproc_info {
> void *rbufs, *sbufs;
> unsigned int num_rbufs;
> unsigned int num_sbufs;
> - unsigned int buf_size;
> + unsigned int rbuf_size;
> + unsigned int sbuf_size;
> int last_sbuf;
> dma_addr_t bufs_dma;
> struct mutex tx_lock;
> @@ -70,9 +73,6 @@ struct virtproc_info {
> atomic_t sleepers;
> };
>
> -/* 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
> @@ -130,7 +130,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.
> @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>
> /* either pick the next unused tx buffer */
> if (vrp->last_sbuf < vrp->num_sbufs)
> - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> /* or recycle a used one */
> else
> ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
> dev_err(dev, "message is too big (%d)\n", len);
> return -EMSGSIZE;
> }
> @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
> }
>
> static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> @@ -706,7 +706,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->rbuf_size ||
> msg_len > (len - sizeof(struct rpmsg_hdr))) {
> dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
> return -EINVAL;
> @@ -739,7 +739,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->rbuf_size);
>
> /* add the buffer back to the remote processor's virtqueue */
> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
> else
> vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>
> - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> + /*
> + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
> + * size from virtio device config space from the resource table.
> + * If the feature is not supported, then assign default buf size.
> + */
> + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> + /* note: virtio_rpmsg_config is defined from remote view */
> + virtio_cread(vdev, struct virtio_rpmsg_config,
> + txbuf_size, &vrp->rbuf_size);
> + virtio_cread(vdev, struct virtio_rpmsg_config,
> + rxbuf_size, &vrp->sbuf_size);
> +
> + /* The buffers must hold rpmsg header atleast */
typo: /* The buffers must hold at least the rpmsg header */
> + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
> + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
> + dev_err(&vdev->dev,
> + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
message could be more explicit: s/"vdev config:/"bad vdev config:/
> + vrp->rbuf_size, vrp->sbuf_size);
> + err = -EINVAL;
> + goto vqs_del;
> + }
> +
> + dev_dbg(&vdev->dev,
> + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
> + vrp->rbuf_size, vrp->sbuf_size);
> + } else {
> + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
> + }
>
> - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
> + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> + (vrp->num_sbufs * vrp->sbuf_size);
> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
Needed? The allocator does not manage the page alignment?
>
> /* allocate coherent memory for the buffers */
> bufs_va = dma_alloc_coherent(vdev->dev.parent,
> @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
> vrp->rbufs = bufs_va;
>
> /* and second part is dedicated for TX */
> - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
> + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
>
> /* set up the receive buffers */
> for (i = 0; i < vrp->num_rbufs; i++) {
> struct scatterlist sg;
> - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>
> - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>
> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> GFP_KERNEL);
> @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
> - size_t total_buf_space = num_bufs * vrp->buf_size;
> + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
> + (vrp->num_sbufs * vrp->sbuf_size);
> int ret;
>
> virtio_reset_device(vdev);
> @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device *vdev)
>
> vdev->config->del_vqs(vrp->vdev);
>
> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
> dma_free_coherent(vdev->dev.parent, total_buf_space,
> vrp->rbufs, vrp->bufs_dma);
>
> @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
> new file mode 100644
> index 000000000000..6406bc505383
> --- /dev/null
> +++ b/include/linux/virtio_rpmsg.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
This licensing seems reserved for the UAPIs
> +/*
> + * Copyright (C) Pinecone Inc. 2019
> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> + */
> +
> +#ifndef _LINUX_VIRTIO_RPMSG_H
> +#define _LINUX_VIRTIO_RPMSG_H
> +
> +#include <linux/types.h>
> +
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
> +
> +struct virtio_rpmsg_config {
> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> + __u32 txbuf_size;
> + __u32 rxbuf_size;
> + __u32 reserved[14]; /* Reserve for the future use */
> + /* Put the customize config here */
> +} __attribute__((packed));
I am still convinced that adding a version field would help with future
improvements, even though a feature bit field can be used to handle new
configurations. Indeed, how can we address the need for more space
beyond the 14 reserved words?
Thanks,
Arnaud
> +
> +#endif /* _LINUX_VIRTIO_RPMSG_H */
On 11/21/25 3:44 AM, Arnaud POULIQUEN wrote:
>
>
> On 11/14/25 19:46, Tanmay Shah wrote:
>> From: Xiang Xiao <xiaoxiang781216@gmail.com>
>>
>> 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: Xiang Xiao <xiaoxiang@xiaomi.com>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>> drivers/rpmsg/virtio_rpmsg_bus.c | 68 +++++++++++++++++++++++---------
>> include/linux/virtio_rpmsg.h | 24 +++++++++++
>> 2 files changed, 74 insertions(+), 18 deletions(-)
>> create mode 100644 include/linux/virtio_rpmsg.h
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
>> virtio_rpmsg_bus.c
>> index cc26dfcc3e29..03dd5535880a 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -25,6 +25,7 @@
>> #include <linux/sched.h>
>> #include <linux/virtio.h>
>> #include <linux/virtio_ids.h>
>> +#include <linux/virtio_rpmsg.h>
>> #include <linux/virtio_config.h>
>> #include <linux/wait.h>
>> @@ -39,7 +40,8 @@
>> * @sbufs: kernel address of tx buffers
>> * @num_rbufs: total number of buffers for rx
>> * @num_sbufs: total number of buffers for tx
>> - * @buf_size: size of one rx or tx buffer
>> + * @rbuf_size: size of one rx buffer
>> + * @sbuf_size: size of one tx buffer
>> * @last_sbuf: index of last tx buffer used
>> * @bufs_dma: dma base addr of the buffers
>> * @tx_lock: protects svq, sbufs and sleepers, to allow
>> concurrent senders.
>> @@ -60,7 +62,8 @@ struct virtproc_info {
>> void *rbufs, *sbufs;
>> unsigned int num_rbufs;
>> unsigned int num_sbufs;
>> - unsigned int buf_size;
>> + unsigned int rbuf_size;
>> + unsigned int sbuf_size;
>> int last_sbuf;
>> dma_addr_t bufs_dma;
>> struct mutex tx_lock;
>> @@ -70,9 +73,6 @@ struct virtproc_info {
>> atomic_t sleepers;
>> };
>> -/* 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
>> @@ -130,7 +130,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.
>> @@ -443,7 +443,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>> /* either pick the next unused tx buffer */
>> if (vrp->last_sbuf < vrp->num_sbufs)
>> - ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>> + ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>> /* or recycle a used one */
>> else
>> ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -569,7 +569,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->sbuf_size - sizeof(struct rpmsg_hdr)) {
>> dev_err(dev, "message is too big (%d)\n", len);
>> return -EMSGSIZE;
>> }
>> @@ -680,7 +680,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->sbuf_size - sizeof(struct rpmsg_hdr);
>> }
>> static int rpmsg_recv_single(struct virtproc_info *vrp, struct
>> device *dev,
>> @@ -706,7 +706,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->rbuf_size ||
>> msg_len > (len - sizeof(struct rpmsg_hdr))) {
>> dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
>> return -EINVAL;
>> @@ -739,7 +739,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->rbuf_size);
>> /* add the buffer back to the remote processor's virtqueue */
>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>> @@ -888,9 +888,39 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> else
>> vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>> - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>> + /*
>> + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure buf
>> + * size from virtio device config space from the resource table.
>> + * If the feature is not supported, then assign default buf size.
>> + */
>> + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>> + /* note: virtio_rpmsg_config is defined from remote view */
>> + virtio_cread(vdev, struct virtio_rpmsg_config,
>> + txbuf_size, &vrp->rbuf_size);
>> + virtio_cread(vdev, struct virtio_rpmsg_config,
>> + rxbuf_size, &vrp->sbuf_size);
>> +
>> + /* The buffers must hold rpmsg header atleast */
>
> typo: /* The buffers must hold at least the rpmsg header */
Ack.
>> + if (vrp->rbuf_size < sizeof(struct rpmsg_hdr) ||
>> + vrp->sbuf_size < sizeof(struct rpmsg_hdr)) {
>> + dev_err(&vdev->dev,
>> + "vdev config: rx buf sz = %d, tx buf sz = %d\n",
>
> message could be more explicit: s/"vdev config:/"bad vdev config:/
>
Ack.
>> + vrp->rbuf_size, vrp->sbuf_size);
>> + err = -EINVAL;
>> + goto vqs_del;
>> + }
>> +
>> + dev_dbg(&vdev->dev,
>> + "vdev config: rx buf sz = 0x%x, tx buf sz = 0x%x\n",
>> + vrp->rbuf_size, vrp->sbuf_size);
>> + } else {
>> + vrp->rbuf_size = DEFAULT_RPMSG_BUF_SIZE;
>> + vrp->sbuf_size = DEFAULT_RPMSG_BUF_SIZE;
>> + }
>> - total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
>> + total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
>> + (vrp->num_sbufs * vrp->sbuf_size);
>> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>
> Needed? The allocator does not manage the page alignment?
>
Ack.
Not needed. Now as you pointed, I think it's better to leave the
alignment to the allocator. This will be fixed.
>> /* allocate coherent memory for the buffers */
>> bufs_va = dma_alloc_coherent(vdev->dev.parent,
>> @@ -908,14 +938,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>> vrp->rbufs = bufs_va;
>> /* and second part is dedicated for TX */
>> - vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
>> + vrp->sbufs = bufs_va + (vrp->num_rbufs * vrp->rbuf_size);
>> /* set up the receive buffers */
>> for (i = 0; i < vrp->num_rbufs; i++) {
>> struct scatterlist sg;
>> - void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>> + void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>> - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>> + rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>> GFP_KERNEL);
>> @@ -1001,8 +1031,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_rbufs + vrp->num_sbufs;
>> - size_t total_buf_space = num_bufs * vrp->buf_size;
>> + size_t total_buf_space = (vrp->num_rbufs * vrp->rbuf_size) +
>> + (vrp->num_sbufs * vrp->sbuf_size);
>> int ret;
>> virtio_reset_device(vdev);
>> @@ -1015,6 +1045,7 @@ static void rpmsg_remove(struct virtio_device
>> *vdev)
>> vdev->config->del_vqs(vrp->vdev);
>> + total_buf_space = ALIGN(total_buf_space, PAGE_SIZE);
>> dma_free_coherent(vdev->dev.parent, total_buf_space,
>> vrp->rbufs, vrp->bufs_dma);
>> @@ -1028,6 +1059,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/virtio_rpmsg.h b/include/linux/virtio_rpmsg.h
>> new file mode 100644
>> index 000000000000..6406bc505383
>> --- /dev/null
>> +++ b/include/linux/virtio_rpmsg.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>
> This licensing seems reserved for the UAPIs
>
Ack. I will check other headers in the same directory and update license
accordingly.
>> +/*
>> + * Copyright (C) Pinecone Inc. 2019
>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>> + */
>> +
>> +#ifndef _LINUX_VIRTIO_RPMSG_H
>> +#define _LINUX_VIRTIO_RPMSG_H
>> +
>> +#include <linux/types.h>
>> +
>> +/* The feature bitmap for virtio rpmsg */
>> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
>> notifications */
>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config
>> space */
>> +
>> +struct virtio_rpmsg_config {
>> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>> + __u32 txbuf_size;
>> + __u32 rxbuf_size;
>> + __u32 reserved[14]; /* Reserve for the future use */
>> + /* Put the customize config here */
>> +} __attribute__((packed));
>
> I am still convinced that adding a version field would help with future
> improvements, even though a feature bit field can be used to handle new
> configurations. Indeed, how can we address the need for more space
> beyond the 14 reserved words?
> Thanks,
> Arnaud
>
Ack.
Okay, it's not a deal breaker for me. I will add the version field linux
side. I will add new structure in open-amp side as well accordingly.
>> +
>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
>
© 2016 - 2026 Red Hat, Inc.