On Mon, 4 Mar 2024, Xuan Zhuo 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.
>
> Signed-off-by: Xuan Zhuo <xuanz...@linux.alibaba.com>
> ---
> arch/um/drivers/virtio_uml.c | 23 ++++-----
> drivers/platform/mellanox/mlxbf-tmfifo.c | 13 ++----
> drivers/remoteproc/remoteproc_virtio.c | 28 ++++++-----
> drivers/s390/virtio/virtio_ccw.c | 29 ++++++------
> drivers/virtio/virtio_mmio.c | 26 +++++------
> drivers/virtio/virtio_pci_common.c | 59 +++++++++++-------------
> drivers/virtio/virtio_pci_common.h | 9 +---
> drivers/virtio/virtio_pci_legacy.c | 11 +++--
> drivers/virtio/virtio_pci_modern.c | 33 +++++++------
> drivers/virtio/virtio_vdpa.c | 36 +++++++--------
> include/linux/virtio_config.h | 51 ++++++++++++++++----
> 11 files changed, 172 insertions(+), 146 deletions(-)
>
> diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> index 8adca2000e51..c13dfeeb90c4 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[cfg->cfg_idx]);
>
> vq = vring_create_virtqueue(index, num, PAGE_SIZE, vdev, true, true,
> - ctx, vu_notify, callback, info->name);
> + cfg->ctx ? cfg->ctx[cfg->cfg_idx] : false,
Based on the commit message, I don't understand why this transformation
was made. It's perhaps some artifact of moving things around but please
state it in the commit message because this isn't 1:1 transformation
which would be just ctx -> cfg->ctx
> + vu_notify,
> + cfg->callbacks[cfg->cfg_idx], info->name);
> if (!vq) {
> rc = -ENOMEM;
> goto error_create;
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index b655fccaf773..a9ae03904dcf 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -172,9 +172,7 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> }
>
> static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned
> int index,
> - void (*callback)(struct virtqueue *vq),
> - const char *name,
> - bool ctx,
> + struct virtio_vq_config *cfg,
> u16 msix_vec)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> @@ -186,13 +184,13 @@ static struct virtqueue *vp_setup_vq(struct
> virtio_device *vdev, unsigned int in
> if (!info)
> return ERR_PTR(-ENOMEM);
>
> - vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
> + vq = vp_dev->setup_vq(vp_dev, info, index, cfg,
> msix_vec);
Should now easily fit to one line.
> @@ -126,10 +124,7 @@ bool vp_notify(struct virtqueue *vq);
> /* the config->del_vqs() implementation */
> void vp_del_vqs(struct virtio_device *vdev);
> /* the config->find_vqs() implementation */
> -int vp_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);
> +int vp_find_vqs(struct virtio_device *vdev, struct virtio_vq_config *cfg);
Without knowing better, do you expect cfg is mutated inside vp_find_vqs()?
If not, mark it as const.
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index da9b271b54db..1df8634d1258 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -96,6 +96,20 @@ typedef void vq_callback_t(struct virtqueue *);
> * @create_avq: create admin virtqueue resource.
> * @destroy_avq: destroy admin virtqueue resource.
> */
> +
> +struct virtio_vq_config {
> + unsigned int nvqs;
> +
> + /* the vq index may not eq to the cfg index of the other array items */
Can you try to make this comment clearer, as is I don't understand what it
means. E.g. what is "the other array"? not eq = not equal ?
> + unsigned int cfg_idx;
> +
> + struct virtqueue **vqs;
> + vq_callback_t **callbacks;
> + const char *const *names;
> + const bool *ctx;
> + struct irq_affinity *desc;
> +};
The placement of the struct is wrong. Now the documentation of struct
virtio_config_ops is above your struct!?!
Please also document the members of the newly added struct with kerneldoc.
> +
> struct virtio_config_ops {
> void (*get)(struct virtio_device *vdev, unsigned offset,
> void *buf, unsigned len);
> @@ -105,10 +119,7 @@ struct virtio_config_ops {
> u8 (*get_status)(struct virtio_device *vdev);
> void (*set_status)(struct virtio_device *vdev, u8 status);
> void (*reset)(struct virtio_device *vdev);
> - int (*find_vqs)(struct virtio_device *, unsigned nvqs,
> - struct virtqueue *vqs[], vq_callback_t *callbacks[],
> - const char * const names[], const bool *ctx,
> - struct irq_affinity *desc);
> + int (*find_vqs)(struct virtio_device *vdev, struct virtio_vq_config
> *cfg);
> void (*del_vqs)(struct virtio_device *);
> void (*synchronize_cbs)(struct virtio_device *);
> u64 (*get_features)(struct virtio_device *vdev);
--
i.
