On Wed, 31 Jan 2024 17:12:34 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Tue, Jan 30, 2024 at 7:42 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:
> >
> > But every time,
> > 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.
> >
> > And squish the parameters from transport to a structure.
>
> The patch did more than what is described here, it also switch to use
> a structure for vring_create_virtqueue() etc.
>
> Is it better to split?
Sure.
>
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > ---
> > arch/um/drivers/virtio_uml.c | 29 +++++++-----
> > drivers/platform/mellanox/mlxbf-tmfifo.c | 14 +++---
> > drivers/remoteproc/remoteproc_virtio.c | 28 ++++++-----
> > drivers/s390/virtio/virtio_ccw.c | 33 ++++++-------
> > drivers/virtio/virtio_mmio.c | 30 ++++++------
> > drivers/virtio/virtio_pci_common.c | 59 +++++++++++-------------
> > drivers/virtio/virtio_pci_common.h | 9 +---
> > drivers/virtio/virtio_pci_legacy.c | 16 ++++---
> > drivers/virtio/virtio_pci_modern.c | 24 +++++-----
> > drivers/virtio/virtio_ring.c | 59 ++++++++++--------------
> > drivers/virtio/virtio_vdpa.c | 33 +++++++------
> > include/linux/virtio_config.h | 51 ++++++++++++++++----
> > include/linux/virtio_ring.h | 40 ++++++----------
> > 13 files changed, 217 insertions(+), 208 deletions(-)
> >
> > diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> > index 8adca2000e51..161bac67e454 100644
> > --- a/arch/um/drivers/virtio_uml.c
> > +++ b/arch/um/drivers/virtio_uml.c
> > @@ -937,11 +937,12 @@ 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;
> > + struct transport_vq_config tp_cfg = {};
>
> Nit: what did "tp" short for?
tp: transport
Any better?
>
> > struct virtio_uml_vq_info *info;
> > struct virtqueue *vq;
> > int num = MAX_SUPPORTED_QUEUE_SIZE;
> > @@ -953,10 +954,15 @@ 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[cfg->cfg_idx]);
> >
> > - dev_err(dev, "vring_new_virtqueue %s failed\n", name);
[...]
> > - if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > - return vring_create_virtqueue_packed(index, num, vring_align,
> > - vdev, weak_barriers, may_reduce_num,
> > - context, notify, callback, name, vdev->dev.parent);
> > + dma_dev = tp_cfg->dma_dev;
> > + if (!dma_dev)
> > + dma_dev = vdev->dev.parent;
>
> Nit: This seems suboptimal than using "?:" ?
YES
>
> >
> > - return vring_create_virtqueue_split(index, num, vring_align,
> > - vdev, weak_barriers, may_reduce_num,
> > - context, notify, callback, name, vdev->dev.parent);
> > -}
[...]
> > err = PTR_ERR(vqs[i]);
> > goto err_setup_vq;
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 2b3438de2c4d..e2c72e125dae 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -94,6 +94,20 @@ typedef void vq_callback_t(struct virtqueue *);
> > * If disable_vq_and_reset is set, then enable_vq_after_reset must also be
> > * set.
> > */
> > +
> > +struct virtio_vq_config {
> > + unsigned int nvqs;
> > +
> > + /* the vq index may not eq to the cfg index of the other array items */
>
> What does this mean?
When we read from the names/ctx/callbacks array, we can use the vq index,
because some names maybe null, the vq index may not equal to the array index.
We must save a cfg idx for the names/ctx/callbacks array.
for (i = 0; i < nvqs; ++i) {
if (!cfg->names[i]) {
vqs[i] = NULL;
continue;
}
cfg->cfg_idx = i;
vqs[i] = vp_setup_vq(vdev, queue_idx++, cfg, VIRTIO_MSI_NO_VECTOR);
if (IS_ERR(vqs[i])) {
err = PTR_ERR(vqs[i]);
goto out_del_vqs;
}
}
notice "i" and "queue_idx"
Thanks.
>
> > + unsigned int cfg_idx;
> > +
> > + struct virtqueue **vqs;
> > + vq_callback_t **callbacks;
> > + const char *const *names;
> > + const bool *ctx;
> > + struct irq_affinity *desc;
> > +};
> > +
> > struct virtio_config_ops {
> > void (*get)(struct virtio_device *vdev, unsigned offset,
> > void *buf, unsigned len);
[...]
> > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > index 9b33df741b63..0de46ed17cc0 100644
> > --- a/include/linux/virtio_ring.h
> > +++ b/include/linux/virtio_ring.h
> > @@ -5,6 +5,7 @@
> > #include <asm/barrier.h>
> > #include <linux/irqreturn.h>
> > #include <uapi/linux/virtio_ring.h>
> > +#include <linux/virtio_config.h>
> >
> > /*
> > * Barriers in virtio are tricky. Non-SMP virtio guests can't assume
> > @@ -60,38 +61,25 @@ struct virtio_device;
> > struct virtqueue;
> > struct device;
> >
> > +struct transport_vq_config {
>
> To reduce the confusion, let's rename this as "vq_transport_config"
OK
Thanks.
>
> Thanks
>
