| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251203142540-mutt-send-email-mst@kernel.org>
Date: Wed, 3 Dec 2025 14:35:56 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Tanmay Shah <tanmay.shah@....com>
Cc: Zhongqiu Han <zhongqiu.han@....qualcomm.com>, andersson@...nel.org,
mathieu.poirier@...aro.org, jasowang@...hat.com,
xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
linux-kernel@...r.kernel.org, linux-remoteproc@...r.kernel.org,
virtualization@...ts.linux.dev, xiaoxiang@...omi.com,
Xiang Xiao <xiaoxiang781216@...il.com>,
arnaud.pouliquen@...s.st.com
Subject: Re: [RFC PATCH 2/2] rpmsg: virtio_rpmsg_bus: get buffer size from
config space
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@...il.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@...omi.com>
> > > Signed-off-by: Tanmay Shah <tanmay.shah@....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@...econe.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 */
> >
> >
Powered by blists - more mailing lists