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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

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

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 <xiaoxiang@pinecone.net>
> >>>> + * Copyright (C) Advanced Micro Devices, Inc.
> >>>
> >>> No year specified for AMD copyright?
> >>
> >> Ack, will fix.
> >>
> >>>
> >>>> + */
> >>>> +
> >>>> +#ifndef _LINUX_VIRTIO_RPMSG_H
> >>>> +#define _LINUX_VIRTIO_RPMSG_H
> >>>> +
> >>>> +#include <linux/types.h>
> >>>> +#include <linux/virtio_types.h>
> >>>> +
> >>>> +/* The feature bitmap for virtio rpmsg */
> >>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service
> >>>> notifications */
> >>>> +#define VIRTIO_RPMSG_F_BUFSZ    1 /* RP get buffer size from config
> >>>> space */
> >>>> +
> >>>> +/**
> >>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device
> >>>> + *
> >>>> + * @version: version of this structure. current version is 1.
> >>>> + * @size:    size of this structure. unused for the remoteproc virtio
> >>>> backend.
> >>>
> >>> reference to remoteproc to remove
> >>>
> >>
> >> Ack.
> >>
> >>>> + * @txbuf_size: Tx buf size from remote's view. For Linux this is rx
> >>>> buf size.
> >>>> + * @rxbuf_size: Rx buf size from remote's view. For Linux this is tx
> >>>> buf size.
> >>>> + */
> >>>> +struct virtio_rpmsg_config {
> >>>> +    u8 version;
> >>>> +    __virtio16 size;
> >>>> +    /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>> +    __virtio32 txbuf_size;
> >>>> +    __virtio32 rxbuf_size;
> >>>> +} __packed;
> >>>
> >>>
> >>> As mentioned above
> >>> - The size should be be a multiple of four to ensure 64-bit alignment.
> 
> Here, do you mean to document that size should be a multiple of 4 ? How
> to enforce the alignment to 4? We can only document right?
> 
> Thanks,
> Tanmay
> 
> >>> - I would add an alignment field to align the address of the TX buffers
> >>> to the cache line.
> >>>
> >>
> >> Ok, I will change and test.
> >>
> >>> Thanks,
> >>> Arnaud
> >>>
> >>>
> >>>> +
> >>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */
> >>>
> >>
> > 
>