Re: [PATCH vhost v6 3/6] virtio: find_vqs: pass struct instead of multi parameters

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 28 Mar 2024 12:26:46 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Wed, Mar 27, 2024 at 5:57 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > Now, we pass multi parameters to find_vqs. These parameters
> > may work for transport or work for vring.
> >
> > And find_vqs has multi implements in many places:
> >
> >  arch/um/drivers/virtio_uml.c
> >  drivers/platform/mellanox/mlxbf-tmfifo.c
> >  drivers/remoteproc/remoteproc_virtio.c
> >  drivers/s390/virtio/virtio_ccw.c
> >  drivers/virtio/virtio_mmio.c
> >  drivers/virtio/virtio_pci_legacy.c
> >  drivers/virtio/virtio_pci_modern.c
> >  drivers/virtio/virtio_vdpa.c
> >
> > Every time, we try to add a new parameter, that is difficult.
> > We must change every find_vqs implement.
> >
> > One the other side, if we want to pass a parameter to vring,
> > we must change the call path from transport to vring.
> > Too many functions need to be changed.
> >
> > So it is time to refactor the find_vqs. We pass a structure
> > cfg to find_vqs(), that will be passed to vring by transport.
> >
> > Because the vp_modern_create_avq() use the "const char *names[]",
> > and the virtio_uml.c changes the name in the subsequent commit, so
> > change the "names" inside the virtio_vq_config from "const char *const
> > *names" to "const char **names".
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > Acked-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >  arch/um/drivers/virtio_uml.c             | 22 +++----
> >  drivers/platform/mellanox/mlxbf-tmfifo.c | 13 ++--
> >  drivers/remoteproc/remoteproc_virtio.c   | 25 ++++----
> >  drivers/s390/virtio/virtio_ccw.c         | 28 ++++-----
> >  drivers/virtio/virtio_mmio.c             | 26 ++++----
> >  drivers/virtio/virtio_pci_common.c       | 57 ++++++++----------
> >  drivers/virtio/virtio_pci_common.h       |  9 +--
> >  drivers/virtio/virtio_pci_legacy.c       | 11 ++--
> >  drivers/virtio/virtio_pci_modern.c       | 32 ++++++----
> >  drivers/virtio/virtio_vdpa.c             | 33 +++++-----
> >  include/linux/virtio_config.h            | 76 ++++++++++++++++++------
> >  11 files changed, 175 insertions(+), 157 deletions(-)
> >
> > diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> > index 773f9fc4d582..adc619362cc0 100644
> > --- a/arch/um/drivers/virtio_uml.c
> > +++ b/arch/um/drivers/virtio_uml.c
> > @@ -937,8 +937,8 @@ static int vu_setup_vq_call_fd(struct virtio_uml_device *vu_dev,
> >  }
> >
> >  static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
> > -                                    unsigned index, vq_callback_t *callback,
> > -                                    const char *name, bool ctx)
> > +                                    unsigned index,
> > +                                    struct virtio_vq_config *cfg)
> >  {
> >         struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
> >         struct platform_device *pdev = vu_dev->pdev;
> > @@ -953,10 +953,12 @@ static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
> >                 goto error_kzalloc;
> >         }
> >         snprintf(info->name, sizeof(info->name), "%s.%d-%s", pdev->name,
> > -                pdev->id, name);
> > +                pdev->id, cfg->names[index]);
> >
> >         vq = vring_create_virtqueue(index, num, PAGE_SIZE, vdev, true, true,
> > -                                   ctx, vu_notify, callback, info->name);
> > +                                   cfg->ctx ? cfg->ctx[index] : false,
> > +                                   vu_notify,
> > +                                   cfg->callbacks[index], info->name);
> >         if (!vq) {
> >                 rc = -ENOMEM;
> >                 goto error_create;
> > @@ -1013,12 +1015,11 @@ static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
> >         return ERR_PTR(rc);
> >  }
> >
> > -static int vu_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > -                      struct virtqueue *vqs[], vq_callback_t *callbacks[],
> > -                      const char * const names[], const bool *ctx,
> > -                      struct irq_affinity *desc)
> > +static int vu_find_vqs(struct virtio_device *vdev, struct virtio_vq_config *cfg)
> >  {
> >         struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
> > +       struct virtqueue **vqs = cfg->vqs;
> > +       unsigned int nvqs = cfg->nvqs;
> >         struct virtqueue *vq;
> >         int i, rc;
> >
> > @@ -1031,13 +1032,12 @@ static int vu_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> >                 return rc;
> >
> >         for (i = 0; i < nvqs; ++i) {
> > -               if (!names[i]) {
> > +               if (!cfg->names[i]) {
> >                         rc = -EINVAL;
> >                         goto error_setup;
> >                 }
> >
> > -               vqs[i] = vu_setup_vq(vdev, i, callbacks[i], names[i],
> > -                                    ctx ? ctx[i] : false);
> > +               vqs[i] = vu_setup_vq(vdev, i, cfg);
> >                 if (IS_ERR(vqs[i])) {
> >                         rc = PTR_ERR(vqs[i]);
> >                         goto error_setup;
> > diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > index b8d1e32e97eb..4252388f52a2 100644
> > --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> > +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > @@ -1056,15 +1056,12 @@ static void mlxbf_tmfifo_virtio_del_vqs(struct virtio_device *vdev)
> >
> >  /* Create and initialize the virtual queues. */
> >  static int mlxbf_tmfifo_virtio_find_vqs(struct virtio_device *vdev,
> > -                                       unsigned int nvqs,
> > -                                       struct virtqueue *vqs[],
> > -                                       vq_callback_t *callbacks[],
> > -                                       const char * const names[],
> > -                                       const bool *ctx,
> > -                                       struct irq_affinity *desc)
> > +                                       struct virtio_vq_config *cfg)
> >  {
> >         struct mlxbf_tmfifo_vdev *tm_vdev = mlxbf_vdev_to_tmfifo(vdev);
> > +       struct virtqueue **vqs = cfg->vqs;
> >         struct mlxbf_tmfifo_vring *vring;
> > +       unsigned int nvqs = cfg->nvqs;
> >         struct virtqueue *vq;
> >         int i, ret, size;
> >
> > @@ -1072,7 +1069,7 @@ static int mlxbf_tmfifo_virtio_find_vqs(struct virtio_device *vdev,
> >                 return -EINVAL;
> >
> >         for (i = 0; i < nvqs; ++i) {
> > -               if (!names[i]) {
> > +               if (!cfg->names[i]) {
> >                         ret = -EINVAL;
> >                         goto error;
> >                 }
> > @@ -1084,7 +1081,7 @@ static int mlxbf_tmfifo_virtio_find_vqs(struct virtio_device *vdev,
> >                 vq = vring_new_virtqueue(i, vring->num, vring->align, vdev,
> >                                          false, false, vring->va,
> >                                          mlxbf_tmfifo_virtio_notify,
> > -                                        callbacks[i], names[i]);
> > +                                        cfg->callbacks[i], cfg->names[i]);
> >                 if (!vq) {
> >                         dev_err(&vdev->dev, "vring_new_virtqueue failed\n");
> >                         ret = -ENOMEM;
> > diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> > index 8fb5118b6953..489fea1d41c0 100644
> > --- a/drivers/remoteproc/remoteproc_virtio.c
> > +++ b/drivers/remoteproc/remoteproc_virtio.c
> > @@ -102,8 +102,7 @@ EXPORT_SYMBOL(rproc_vq_interrupt);
> >
> >  static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
> >                                     unsigned int id,
> > -                                   void (*callback)(struct virtqueue *vq),
> > -                                   const char *name, bool ctx)
> > +                                   struct virtio_vq_config *cfg)
> >  {
> >         struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> >         struct rproc *rproc = vdev_to_rproc(vdev);
> > @@ -140,10 +139,12 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
> >          * Create the new vq, and tell virtio we're not interested in
> >          * the 'weak' smp barriers, since we're talking with a real device.
> >          */
> > -       vq = vring_new_virtqueue(id, num, rvring->align, vdev, false, ctx,
> > -                                addr, rproc_virtio_notify, callback, name);
> > +       vq = vring_new_virtqueue(id, num, rvring->align, vdev, false,
> > +                                cfg->ctx ? cfg->ctx[id] : false,
> > +                                addr, rproc_virtio_notify, cfg->callbacks[id],
> > +                                cfg->names[id]);
> >         if (!vq) {
> > -               dev_err(dev, "vring_new_virtqueue %s failed\n", name);
> > +               dev_err(dev, "vring_new_virtqueue %s failed\n", cfg->names[id]);
> >                 rproc_free_vring(rvring);
> >                 return ERR_PTR(-ENOMEM);
> >         }
> > @@ -177,23 +178,19 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
> >         __rproc_virtio_del_vqs(vdev);
> >  }
> >
> > -static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> > -                                struct virtqueue *vqs[],
> > -                                vq_callback_t *callbacks[],
> > -                                const char * const names[],
> > -                                const bool * ctx,
> > -                                struct irq_affinity *desc)
> > +static int rproc_virtio_find_vqs(struct virtio_device *vdev, struct virtio_vq_config *cfg)
> >  {
> > +       struct virtqueue **vqs = cfg->vqs;
> > +       unsigned int nvqs = cfg->nvqs;
> >         int i, ret;
> >
> >         for (i = 0; i < nvqs; ++i) {
> > -               if (!names[i]) {
> > +               if (!cfg->names[i]) {
> >                         ret = -EINVAL;
> >                         goto error;
> >                 }
> >
> > -               vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i],
> > -                                   ctx ? ctx[i] : false);
> > +               vqs[i] = rp_find_vq(vdev, i, cfg);
> >                 if (IS_ERR(vqs[i])) {
> >                         ret = PTR_ERR(vqs[i]);
> >                         goto error;
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index 508154705554..3c78122f00f5 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -499,9 +499,8 @@ static void virtio_ccw_del_vqs(struct virtio_device *vdev)
> >  }
> >
> >  static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> > -                                            int i, vq_callback_t *callback,
> > -                                            const char *name, bool ctx,
> > -                                            struct ccw1 *ccw)
> > +                                            int i, struct ccw1 *ccw,
> > +                                            struct virtio_vq_config *cfg)
> >  {
> >         struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> >         bool (*notify)(struct virtqueue *vq);
> > @@ -538,8 +537,11 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> >         }
> >         may_reduce = vcdev->revision > 0;
> >         vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN,
> > -                                   vdev, true, may_reduce, ctx,
> > -                                   notify, callback, name);
> > +                                   vdev, true, may_reduce,
> > +                                   cfg->ctx ? cfg->ctx[i] : false,
> > +                                   notify,
> > +                                   cfg->callbacks[i],
> > +                                   cfg->names[i]);
> >
> >         if (!vq) {
> >                 /* For now, we fail if we can't get the requested size. */
> > @@ -650,15 +652,13 @@ static int virtio_ccw_register_adapter_ind(struct virtio_ccw_device *vcdev,
> >         return ret;
> >  }
> >
> > -static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> > -                              struct virtqueue *vqs[],
> > -                              vq_callback_t *callbacks[],
> > -                              const char * const names[],
> > -                              const bool *ctx,
> > -                              struct irq_affinity *desc)
> > +static int virtio_ccw_find_vqs(struct virtio_device *vdev,
> > +                              struct virtio_vq_config *cfg)
> >  {
> >         struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> > +       struct virtqueue **vqs = cfg->vqs;
> >         unsigned long *indicatorp = NULL;
> > +       unsigned int nvqs = cfg->nvqs;
> >         int ret, i;
> >         struct ccw1 *ccw;
> >
> > @@ -667,14 +667,12 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> >                 return -ENOMEM;
> >
> >         for (i = 0; i < nvqs; ++i) {
> > -               if (!names[i]) {
> > +               if (!cfg->names[i]) {
> >                         ret = -EINVAL;
> >                         goto out;
> >                 }
> >
> > -               vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i],
> > -                                            names[i], ctx ? ctx[i] : false,
> > -                                            ccw);
> > +               vqs[i] = virtio_ccw_setup_vq(vdev, i, ccw, cfg);
> >                 if (IS_ERR(vqs[i])) {
> >                         ret = PTR_ERR(vqs[i]);
> >                         vqs[i] = NULL;
> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > index 82ee4a288728..7f0fdc3f51cb 100644
> > --- a/drivers/virtio/virtio_mmio.c
> > +++ b/drivers/virtio/virtio_mmio.c
> > @@ -370,8 +370,7 @@ static void vm_synchronize_cbs(struct virtio_device *vdev)
> >  }
> >
> >  static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int index,
> > -                                 void (*callback)(struct virtqueue *vq),
> > -                                 const char *name, bool ctx)
> > +                                    struct virtio_vq_config *cfg)
> >  {
> >         struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> >         bool (*notify)(struct virtqueue *vq);
> > @@ -386,9 +385,6 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
> >         else
> >                 notify = vm_notify;
> >
> > -       if (!name)
> > -               return NULL;
>
> Nit: This seems to belong to patch 2.

YES.

Will fix in next version.

Thanks.


>
> Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
>
> Thanks
>





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux