On Tue, Jun 09, 2026 at 12:33:47PM -0500, Shah, Tanmay wrote:
>
>
> On 6/5/2026 3:25 AM, Arnaud POULIQUEN wrote:
> > Hi Tanmay,
> >
> > On 6/4/26 22:31, Shah, Tanmay wrote:
> >> Thank You for the reviews, please find my comments below:
> >>
> >> On 6/2/2026 3:25 AM, Arnaud POULIQUEN wrote:
> >>> Hi Tanmay,
> >>>
> >>> On 5/29/26 18:43, Tanmay Shah wrote:
> >>>> 512 bytes isn't always suitable for all case, let firmware
> >>>> maker decide the best value from resource table.
> >>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> >>>>
> >>>> Signed-off-by: Tanmay Shah <[email protected]>
> >>>> ---
> >>>>
> >>>> Changes in v3:
> >>>> - change version field from u16 to u8
> >>>> - introduce size field in the rpmsg_virtio_config structure
> >>>> - check version field is set to any non-zero value.
> >>>> - check size field is not 0.
> >>>> - Remove field for private config, as not needed for now.
> >>>> - add documentation of rpmsg_virtio_config structure
> >>>>
> >>>> drivers/rpmsg/virtio_rpmsg_bus.c | 90 +++++++++++++++++++++++
> >>>> +------
> >>>> include/linux/rpmsg/virtio_rpmsg.h | 34 +++++++++++
> >>>> 2 files changed, 106 insertions(+), 18 deletions(-)
> >>>> create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
> >>>>
> >>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/
> >>>> virtio_rpmsg_bus.c
> >>>> index 99df1ae07055..f1ab8e792f3d 100644
> >>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>> @@ -20,6 +20,7 @@
> >>>> #include <linux/rpmsg.h>
> >>>> #include <linux/rpmsg/byteorder.h>
> >>>> #include <linux/rpmsg/ns.h>
> >>>> +#include <linux/rpmsg/virtio_rpmsg.h>
> >>>> #include <linux/scatterlist.h>
> >>>> #include <linux/slab.h>
> >>>> #include <linux/sched.h>
> >>>> @@ -39,7 +40,8 @@
> >>>> * @tx_bufs: kernel address of tx buffers
> >>>> * @num_rx_buf: total number of rx buffers
> >>>> * @num_tx_buf: total number of tx buffers
> >>>> - * @buf_size: size of one rx or tx buffer
> >>>> + * @rx_buf_size: size of one rx buffer
> >>>> + * @tx_buf_size: size of one tx buffer
> >>>> * @last_tx_buf: index of last tx buffer used
> >>>> * @bufs_dma: dma base addr of the buffers
> >>>> * @tx_lock: protects svq and tx_bufs, to allow concurrent
> >>>> senders.
> >>>> @@ -59,7 +61,8 @@ struct virtproc_info {
> >>>> void *rx_bufs, *tx_bufs;
> >>>> unsigned int num_rx_buf;
> >>>> unsigned int num_tx_buf;
> >>>> - unsigned int buf_size;
> >>>> + unsigned int rx_buf_size;
> >>>> + unsigned int tx_buf_size;
> >>>> int last_tx_buf;
> >>>> dma_addr_t bufs_dma;
> >>>> struct mutex tx_lock;
> >>>> @@ -68,9 +71,6 @@ struct virtproc_info {
> >>>> wait_queue_head_t sendq;
> >>>> };
> >>>> -/* The feature bitmap for virtio rpmsg */
> >>>> -#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
> >>>> notifications */
> >>>> -
> >>>> /**
> >>>> * struct rpmsg_hdr - common header for all rpmsg messages
> >>>> * @src: source address
> >>>> @@ -128,7 +128,9 @@ struct virtio_rpmsg_channel {
> >>>> * processor.
> >>>> */
> >>>> #define MAX_RPMSG_NUM_BUFS (256)
> >>>> -#define MAX_RPMSG_BUF_SIZE (512)
> >>>> +#define DEFAULT_RPMSG_BUF_SIZE (512)
> >>>> +
> >>>> +#define RPMSG_VDEV_CONFIG_VER 1
> >>>
> >>> I would rename it
> >>>
> >>> #define RPMSG_VDEV_CONFIG_V1 1
> >>
> >> We might not need this define at all. I should have removed it in this
> >> patch. Please see below [1].
> >>
> >>>
> >>>> /*
> >>>> * Local addresses are dynamically allocated on-demand.
> >>>> @@ -444,7 +446,7 @@ static void *get_a_tx_buf(struct virtproc_info
> >>>> *vrp)
> >>>> /* either pick the next unused tx buffer */
> >>>> if (vrp->last_tx_buf < vrp->num_tx_buf)
> >>>> - ret = vrp->tx_bufs + vrp->buf_size * vrp->last_tx_buf++;
> >>>> + ret = vrp->tx_bufs + vrp->tx_buf_size * vrp->last_tx_buf++;
> >>>> /* or recycle a used one */
> >>>> else
> >>>> ret = virtqueue_get_buf(vrp->svq, &len);
> >>>> @@ -514,7 +516,7 @@ static int rpmsg_send_offchannel_raw(struct
> >>>> rpmsg_device *rpdev,
> >>>> * messaging), or to improve the buffer allocator, to support
> >>>> * variable-length buffer sizes.
> >>>> */
> >>>> - if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> >>>> + if (len > vrp->tx_buf_size - sizeof(struct rpmsg_hdr)) {
> >>>> dev_err(dev, "message is too big (%d)\n", len);
> >>>> return -EMSGSIZE;
> >>>> }
> >>>> @@ -647,7 +649,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct
> >>>> rpmsg_endpoint *ept)
> >>>> struct rpmsg_device *rpdev = ept->rpdev;
> >>>> struct virtio_rpmsg_channel *vch =
> >>>> to_virtio_rpmsg_channel(rpdev);
> >>>> - return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
> >>>> + return vch->vrp->tx_buf_size - sizeof(struct rpmsg_hdr);
> >>>> }
> >>>> static int rpmsg_recv_single(struct virtproc_info *vrp, struct
> >>>> device *dev,
> >>>> @@ -673,7 +675,7 @@ static int rpmsg_recv_single(struct virtproc_info
> >>>> *vrp, struct device *dev,
> >>>> * We currently use fixed-sized buffers, so trivially sanitize
> >>>> * the reported payload length.
> >>>> */
> >>>> - if (len > vrp->buf_size ||
> >>>> + if (len > vrp->rx_buf_size ||
> >>>> msg_len > (len - sizeof(struct rpmsg_hdr))) {
> >>>> dev_warn(dev, "inbound msg too big: (%d, %d)\n", len,
> >>>> msg_len);
> >>>> return -EINVAL;
> >>>> @@ -706,7 +708,7 @@ static int rpmsg_recv_single(struct virtproc_info
> >>>> *vrp, struct device *dev,
> >>>> dev_warn_ratelimited(dev, "msg received with no
> >>>> recipient\n");
> >>>> /* publish the real size of the buffer */
> >>>> - rpmsg_sg_init(&sg, msg, vrp->buf_size);
> >>>> + rpmsg_sg_init(&sg, msg, vrp->rx_buf_size);
> >>>> /* add the buffer back to the remote processor's virtqueue */
> >>>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> >>>> @@ -824,6 +826,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>> int err = 0, i;
> >>>> size_t total_buf_space;
> >>>> bool notify;
> >>>> + u8 version;
> >>>> + u16 size;
> >>>> vrp = kzalloc_obj(*vrp);
> >>>> if (!vrp)
> >>>> @@ -855,9 +859,58 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>> else
> >>>> vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
> >>>> - vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> >>>> + /*
> >>>> + * If VIRTIO_RPMSG_F_BUFSZ feature is supported, then configure
> >>>> buf
> >>>> + * size from virtio device config space from the resource table.
> >>>> + * If the feature is not supported, then assign default buf size.
> >>>> + */
> >>>
> >>> Seems to me that It would be nice to document the config space in
> >>> rpmsg.rst
> >>>
> >>
> >> Ack.
> >>
> >>>> + if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> >>>> + virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>> + version, &version);
> >>>> + if (version == 0) {
> >>>
> >>> if (version != RPMSG_VDEV_CONFIG_V1) {
> >>>
> >>
> >> [1] Here we have to allow any non-zero version to be valid, and make
> >> sure any future version is always backward compatible.
> >>
> >> For example, if we need v2 of the structure, then that should be
> >> compatible with v1. So, old kernel keeps working with the new firmware
> >> with limited functionality supported by the kernel. And new kernel keep
> >> working with the old firmware, with the limited functionality supported
> >> by the firmware.
> >>
> >> That is just my view. I am open to more ideas, thank you.
> >
> > My concern is that you allow any version here, while we define only a V1.
> > If we need a V2 we will probably have to modify this part of the code.
> >
>
> So that means, the firmware that has v2, will never work with the old
> kernel that supports only v1. I would like to design in a way, where
> firmware has v2, but it is back compatible with v1. New firmware can
> still work with old kernel with only functionality that was supported in
> v1.
>
> Mathieu do you have any opinion on this? I don't know if there is any
> standard accepted design pattern for this type of compatibility.
>
>
> use | Linux | firmware |
> case | kernel | version | Note |
> | | | |
> 1 | v1 | v1 | works |
> 2 | v2 | v1 | old firmware features work.
> 3 | v1 | v2 | new firmware features won't work.
Scenario #3 is expected and desired. There is no point in trying to guess what
features a new firmware revision, i.e v2, will have.
I agree with Arnaud, right now we aim to support v1 and nothing else. We can
provision to support newer versions at the time they become available.
>
> In use-case 3, Do we want to fail completely and say old Linux will not
> support new firmware at all ?
>
> Thanks,
> Tanmay
>
>
> >>
> >>>> + dev_err(&vdev->dev, "invalid version of vdev config\n");
> >>>> + err = -EINVAL;
> >>>> + goto vqs_del;
> >>>> + }
> >>>> +
> >>>> + /*
> >>>> + * The size field is not used for the remoteproc virtio
> >>>> transport,
> >>>> + * but kept for any future transport to use
> >>>> + */
> >>>
> >>> I suggest to not mention the virtio transport, indeed we should decouple
> >>> the virtio device from the virtio transport.
> >>>
> >>
> >> Ack.
> >>
> >>>> + virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>> + size, &size);
> >>>> + if (size == 0) {
> >>>
> >>> if (size != sizeof(virtio_rpmsg_config)) {
> >>>
> >>
> >> So, I think sizeof(virtio_rpmsg_config) on the remote side may not be
> >> the same as in the linux kernel. Remote side can have its private
> >> variables which might not needed on the linux side:
> >>
> >> For example, the open-amp library defines the structure differently than
> >> linux:
> >> https://github.com/OpenAMP/open-amp/
> >> blob/23d4c5d7a5c5dd08b74d4ba828243988592337cb/lib/include/openamp/
> >> rpmsg_virtio.h#L70
> >>
> >> There is 'split_shpool' extra variable which is not needed by the linux.
> >
> > The 'split_shpool' setting is currently an internal OpenAMP configuration
> > and is not part of a configuration space. If there is a need to split
> > memory regions for RX and TX buffers, the implementation should use another
> > method, perhaps with DA and PA addresses that specify the RX and TX buffer
> > locations, as for the vrings in the resource table.
> > I would suggest to not address this in version 1.
> >
> > From my perspective, the configuration space is a common structure that
> > the device and the driver share. In that case, checking the size makes
> > sense.
>
> Ack.
>
> > Thanks,
> > Arnaud
> >
> >>
> >> That is why restriction on the size isn't needed IMHO.
> >>
> >>>> + dev_err(&vdev->dev, "invalid size of vdev config\n");
> >>>> + err = -EINVAL;
> >>>> + goto vqs_del;
> >>>> + }
> >>>> +
> >>>> + /* note: tx and rx are defined from remote view */
> >>>> + virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>> + txbuf_size, &vrp->rx_buf_size);
> >>>> + virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>> + rxbuf_size, &vrp->tx_buf_size);
> >>>
> >>> I wonder if we should not impose a size aligned on 64-bits
> >>>
> >>
> >> I think you mean size should be aligned to 64-bits.
> >> Ack for that.
> >>
> >>>> +
> >>>> + /* The buffers must hold at least the rpmsg header */
> >>>> + if (vrp->rx_buf_size < sizeof(struct rpmsg_hdr) ||
> >>>> + vrp->tx_buf_size < sizeof(struct rpmsg_hdr)) {
> >>>> + dev_err(&vdev->dev,
> >>>> + "bad vdev config: rx buf sz = %u, tx buf sz = %u\n",
> >>>> + vrp->rx_buf_size, vrp->tx_buf_size);
> >>>> + err = -EINVAL;
> >>>> + goto vqs_del;
> >>>> + }
> >>>> +
> >>>> + dev_dbg(&vdev->dev,
> >>>> + "vdev config: version=%d, rx buf sz = 0x%x, tx buf sz =
> >>>> 0x%x\n",
> >>>> + version, vrp->rx_buf_size, vrp->tx_buf_size);
> >>>> + } else {
> >>>> + vrp->rx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
> >>>> + vrp->tx_buf_size = DEFAULT_RPMSG_BUF_SIZE;
> >>>> + }
> >>>> - total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp-
> >>>>> buf_size;
> >>>> + total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
> >>>> + (vrp->num_tx_buf * vrp->tx_buf_size);
> >>>> /* allocate coherent memory for the buffers */
> >>>> bufs_va = dma_alloc_coherent(vdev->dev.parent,
> >>>> @@ -875,14 +928,14 @@ static int rpmsg_probe(struct virtio_device
> >>>> *vdev)
> >>>> vrp->rx_bufs = bufs_va;
> >>>> /* and second part is dedicated for TX */
> >>>> - vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
> >>>> + vrp->tx_bufs = bufs_va + (vrp->num_rx_buf * vrp->rx_buf_size);
> >>>
> >>>
> >>> We should have a cache or 64-bit alignement here. or perhaps such
> >>> constraints should be specified in the config space?
> >>>
> >>
> >> I prefer to specify in the config space.
> >>
> >>>> /* set up the receive buffers */
> >>>> for (i = 0; i < vrp->num_rx_buf; i++) {
> >>>> struct scatterlist sg;
> >>>> - void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
> >>>> + void *cpu_addr = vrp->rx_bufs + i * vrp->rx_buf_size;
> >>>> - rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> >>>> + rpmsg_sg_init(&sg, cpu_addr, vrp->rx_buf_size);
> >>>> err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> >>>> GFP_KERNEL);
> >>>> @@ -965,8 +1018,8 @@ static int rpmsg_remove_device(struct device
> >>>> *dev, void *data)
> >>>> static void rpmsg_remove(struct virtio_device *vdev)
> >>>> {
> >>>> struct virtproc_info *vrp = vdev->priv;
> >>>> - unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
> >>>> - size_t total_buf_space = num_bufs * vrp->buf_size;
> >>>> + size_t total_buf_space = (vrp->num_rx_buf * vrp->rx_buf_size) +
> >>>> + (vrp->num_tx_buf * vrp->tx_buf_size);
> >>>> int ret;
> >>>> virtio_reset_device(vdev);
> >>>> @@ -992,6 +1045,7 @@ static struct virtio_device_id id_table[] = {
> >>>> static unsigned int features[] = {
> >>>> VIRTIO_RPMSG_F_NS,
> >>>> + VIRTIO_RPMSG_F_BUFSZ,
> >>>> };
> >>>> static struct virtio_driver virtio_ipc_driver = {
> >>>> diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/
> >>>> virtio_rpmsg.h
> >>>> new file mode 100644
> >>>> index 000000000000..77a530514d86
> >>>> --- /dev/null
> >>>> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> >>>> @@ -0,0 +1,34 @@
> >>>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>> +/*
> >>>> + * Copyright (C) Pinecone Inc. 2019
> >>>> + * Copyright (C) Xiang Xiao <[email protected]>
> >>>> + * Copyright (C) Advanced Micro Devices, Inc.
> >>>
> >>> No year specified for AMD copyright?
> >>
> >> Ack, will fix.
> >>
> >>>
> >>>> + */
> >>>> +
> >>>> +#ifndef _LINUX_VIRTIO_RPMSG_H
> >>>> +#define _LINUX_VIRTIO_RPMSG_H
> >>>> +
> >>>> +#include <linux/types.h>
> >>>> +#include <linux/virtio_types.h>
> >>>> +
> >>>> +/* The feature bitmap for virtio rpmsg */
> >>>> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service
> >>>> notifications */
> >>>> +#define VIRTIO_RPMSG_F_BUFSZ 1 /* RP get buffer size from config
> >>>> space */
> >>>> +
> >>>> +/**
> >>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
> >>>> + *
> >>>> + * @version: version of this structure. current version is 1.
> >>>> + * @size: size of this structure. unused for the remoteproc virtio
> >>>> backend.
> >>>
> >>> reference to remoteproc to remove
> >>>
> >>
> >> Ack.
> >>
> >>>> + * @txbuf_size: Tx buf size from remote's view. For Linux this is rx
> >>>> buf size.
> >>>> + * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx
> >>>> buf size.
> >>>> + */
> >>>> +struct virtio_rpmsg_config {
> >>>> + u8 version;
> >>>> + __virtio16 size;
> >>>> + /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>> + __virtio32 txbuf_size;
> >>>> + __virtio32 rxbuf_size;
> >>>> +} __packed;
> >>>
> >>>
> >>> As mentioned above
> >>> - The size should be be a multiple of four to ensure 64-bit alignment.
>
> Here, do you mean to document that size should be a multiple of 4 ? How
> to enforce the alignment to 4? We can only document right?
>
> Thanks,
> Tanmay
>
> >>> - I would add an alignment field to align the address of the TX buffers
> >>> to the cache line.
> >>>
> >>
> >> Ok, I will change and test.
> >>
> >>> Thanks,
> >>> Arnaud
> >>>
> >>>
> >>>> +
> >>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> >>>
> >>
> >
>
