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

[xiang xiao]

On Wed, Jun 5, 2019 at 4:02 PM Arnaud Pouliquen <arnaud.pouliquen@xxxxxx> wrote:
>
>
>
> On 6/5/19 4:40 AM, xiang xiao wrote:
> > On Tue, Jun 4, 2019 at 10:25 PM Arnaud Pouliquen
> > <arnaud.pouliquen@xxxxxx> wrote:
> >>
> >> Hello Xiang,
> >>
> >> On 5/9/19 3:00 PM, xiang xiao wrote:
> >>> On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@xxxxxx> wrote:
> >>>>
> >>>> Hello Xiang,
> >>>>
> >>>> Similar mechanism has been proposed by Loic 2 years ago (link to the
> >>>> series here https://lkml.org/lkml/2017/3/28/349).
> >>>>
> >>>> Did you see them? Regarding history, patches seem just on hold...
> >>>>
> >>>
> >>> Just saw this patchset, so it's common problem hit by many vendor,
> >>> rpmsg framework need to address it.:)
> >>>
> >>>> Main differences (except interesting RX/TX size split) seems that you
> >>>> - don't use the virtio_config_ops->get
> >>>
> >>> virtio_cread call virtio_config_ops->get internally, the ideal is same
> >>> for both patch, just the implementation detail is different.
> >>>
> >>>> - define a new feature VIRTIO_RPMSG_F_NS.
> >>>
> >>> I add this flag to keep the compatibility with old remote peer, and
> >>> also follow the common virito driver practice.
> >> I discussed with Loic, he is ok to go further with your patch and
> >> abandon his one. Please find some remarks below in-line
> >>>
> >>>>
> >>>> Regards
> >>>> Arnaud
> >>>>
> >>>>
> >>>> On 1/31/19 4:41 PM, Xiang Xiao 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: Xiang Xiao <xiaoxiang@xxxxxxxxxx>
> >>>>> ---
> >>>>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
> >>>>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
> >>>>>  2 files changed, 56 insertions(+), 18 deletions(-)
> >>>>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> >>>>>
> >>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>> index 59c4554..049dd97 100644
> >>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>> @@ -16,6 +16,7 @@
> >>>>>  #include <linux/virtio.h>
> >>>>>  #include <linux/virtio_ids.h>
> >>>>>  #include <linux/virtio_config.h>
> >>>>> +#include <linux/virtio_rpmsg.h>
> >>>>>  #include <linux/scatterlist.h>
> >>>>>  #include <linux/dma-mapping.h>
> >>>>>  #include <linux/slab.h>
> >>>>> @@ -38,7 +39,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
> >>>>>   * @rbufs_dma:       dma base addr of rx buffers
> >>>>>   * @sbufs_dma:       dma base addr of tx buffers
> >>>>> @@ -61,7 +63,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 rbufs_dma;
> >>>>>       dma_addr_t sbufs_dma;
> >>>>> @@ -73,9 +76,6 @@ struct virtproc_info {
> >>>>>       struct rpmsg_endpoint *ns_ept;
> >>>>>  };
> >>>>>
> >>>>> -/* 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
> >>>>> @@ -452,7 +452,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);
> >>>>> @@ -578,7 +578,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;
> >>>>>       }
> >>>>> @@ -718,7 +718,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;
> >>>>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >>>>>               dev_warn(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);
> >>>>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>       else
> >>>>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
> >>>>>
> >>>>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> >>>>> +     /* try to get buffer size from config space */
> >>>>> +     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);
> >>>>> +     }
> >>>>> +
> >>>>> +     /* use the default if resource table doesn't provide one */
> >>>>> +     if (!vrp->rbuf_size)
> >>>>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
> >> In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
> >> no more a max value
> >
> > Yes, DEFAULT_RPMSG_BUF_SIZE is more reasonable now.
> >
> >>>>> +     if (!vrp->sbuf_size)
> >>>>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
> >> Here, if the config space exists you need to update it in consequence to
> >> ensure coherency with the remote processor config.
> >
> > The update is already done in if (virtio_has_feature(vdev,
> > VIRTIO_RPMSG_F_BUFSZ)), here just handle the zero value in config
> > space which mean the remote side want to use the default value even
> > VIRTIO_RPMSG_F_BUFSZ set.
> > For example:
> > 1.remote side want to change one direction buffer size, but keep
> > another direction as default
> > 2.or remote side want to change other config options(define in the
> > furture) not the buffer size
>
> In code above i can see a virtio_cread of the config structure, but no
> writing of it...
> I mentioned the configs space in the resource table itself.
> Without an update, you must ensure that both have the same default
> value... In addition, it makes sense that the master can update the
> buffer size according to some other constraints.

Get your point, thanks.

>
> >
> >>
> >>>>>
> >>>>>       /* allocate coherent memory for the buffers */
> >>>>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> >>>>> -                                     vrp->num_rbufs * vrp->buf_size,
> >>>>> +                                     vrp->num_rbufs * vrp->rbuf_size,
> >>>>>                                       &vrp->rbufs_dma, GFP_KERNEL);
> >>>>>       if (!vrp->rbufs) {
> >>>>>               err = -ENOMEM;
> >>>>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>               vrp->rbufs, &vrp->rbufs_dma);
> >>>>>
> >>>>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> >>>>> -                                     vrp->num_sbufs * vrp->buf_size,
> >>>>> +                                     vrp->num_sbufs * vrp->sbuf_size,
> >>>>>                                       &vrp->sbufs_dma, GFP_KERNEL);
> >>>>>       if (!vrp->sbufs) {
> >>>>>               err = -ENOMEM;
> >>>>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>       /* 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);
> >>>>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>
> >>>>>  free_sbufs:
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_sbufs * vrp->buf_size,
> >>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
> >>>>>                         vrp->sbufs, vrp->sbufs_dma);
> >>>>>  free_rbufs:
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_rbufs * vrp->buf_size,
> >>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
> >>>>>                         vrp->rbufs, vrp->rbufs_dma);
> >>>>>  vqs_del:
> >>>>>       vdev->config->del_vqs(vrp->vdev);
> >>>>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
> >>>>>       vdev->config->del_vqs(vrp->vdev);
> >>>>>
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_sbufs * vrp->buf_size,
> >>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
> >>>>>                         vrp->sbufs, vrp->sbufs_dma);
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_rbufs * vrp->buf_size,
> >>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
> >>>>>                         vrp->rbufs, vrp->rbufs_dma);
> >>>>>
> >>>>>       kfree(vrp);
> >>>>> @@ -1039,6 +1052,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/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
> >>>>> new file mode 100644
> >>>>> index 0000000..24fa0dd
> >>>>> --- /dev/null
> >>>>> +++ b/include/uapi/linux/virtio_rpmsg.h
> >> Strange to define a user space API for kernel usage need. Could you
> >> elaborate?
> >
> > I just follow the practice other virtio drivers(e.g.
> > include/uapi/virtio_net.h) applied, but rpmsg driver don't need to
> > talk with the host VM software like other virtio driver, yes this
> > header file isn't really needed.
> >
> >>>>> @@ -0,0 +1,24 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >>>>> +/*
> >>>>> + * Copyright (C) Pinecone Inc. 2019
> >>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@xxxxxxxxxxxx>
> >>>>> + */
> >>>>> +
> >>>>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
> >>>>> +#define _UAPI_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 */
> >> Would be useful to document it in rpmsg.txt
> >
> > Good point, but it is better to put them into this document:
> > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> > like other vritio driver spec.
> >
> >>>>> +
> >>>>> +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));
> >>>>> +
> >> Wouldn't it be better to add an identifier and a version fields at the
> >> beginning of the structure? Idea would be to simplify a future extension
> >> In this case is VIRTIO_RPMSG_F_BUFSZ still useful?
> >>
> >
> > Yes, I consider this option before, but after review all
> > include/uapi/virtio_*.h, I found that virito driver prefer feature
> > bits than version number to handle the compability issue.
> > For example, if we need introduce more options in the furture, we need:
> > 1.Add new feature bit to notice the option exist
> > 2.Allocate the field from reserved space
> >
> >>>>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
> >>>>>
> >> --
> >> Thanks
> >> Arnaud