[vhost,v3,1/4] virtio: find_vqs: pass struct instead of multi parameters

Commit Message

Xuan Zhuo March 12, 2024, 2:10 a.m. UTC

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 <[e-mail]>
Acked-by: Johannes Berg <[e-mail]>
Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
---
 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       | 60 ++++++++---------
 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            | 85 ++++++++++++++++++------
 11 files changed, 196 insertions(+), 157 deletions(-)

Comments

Jason Wang March 14, 2024, 3:12 a.m. UTC | #1

On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> Acked-by: Johannes Berg <[e-mail]>
> Reviewed-by: Ilpo J=E4rvinen <[e-mail]>

The name seems broken here.

[...]

>
>  typedef void vq_callback_t(struct virtqueue *);
>
> +/**
> + * struct virtio_vq_config - configure for find_vqs()
> + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> + *     During the initialization of each vq(vring setup), we need to know which
> + *     item in the array should be used at that time. But since the item in
> + *     names can be null, which causes some item of array to be skipped, we
> + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> + *     know how to get the current configuration from the array when
> + *     initializing vq.

So this design is not good. If it is not something that the driver
needs to care about, the core needs to hide it from the API.

Thanks

Xuan Zhuo March 14, 2024, 5:58 a.m. UTC | #2

On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > Acked-by: Johannes Berg <[e-mail]>
> > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
>
> The name seems broken here.

Email APP bug.

I will fix.


>
> [...]
>
> >
> >  typedef void vq_callback_t(struct virtqueue *);
> >
> > +/**
> > + * struct virtio_vq_config - configure for find_vqs()
> > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > + *     During the initialization of each vq(vring setup), we need to know which
> > + *     item in the array should be used at that time. But since the item in
> > + *     names can be null, which causes some item of array to be skipped, we
> > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > + *     know how to get the current configuration from the array when
> > + *     initializing vq.
>
> So this design is not good. If it is not something that the driver
> needs to care about, the core needs to hide it from the API.

The driver just ignore it. That will be beneficial to the virtio core.
Otherwise, we must pass one more parameter everywhere.

Thanks.

>
> Thanks
>

Jason Wang March 15, 2024, 3:51 a.m. UTC | #3

On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
>
> On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > Acked-by: Johannes Berg <[e-mail]>
> > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> >
> > The name seems broken here.
>
> Email APP bug.
>
> I will fix.
>
>
> >
> > [...]
> >
> > >
> > >  typedef void vq_callback_t(struct virtqueue *);
> > >
> > > +/**
> > > + * struct virtio_vq_config - configure for find_vqs()
> > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > + *     During the initialization of each vq(vring setup), we need to know which
> > > + *     item in the array should be used at that time. But since the item in
> > > + *     names can be null, which causes some item of array to be skipped, we
> > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > + *     know how to get the current configuration from the array when
> > > + *     initializing vq.
> >
> > So this design is not good. If it is not something that the driver
> > needs to care about, the core needs to hide it from the API.
>
> The driver just ignore it. That will be beneficial to the virtio core.
> Otherwise, we must pass one more parameter everywhere.

I don't get here, it's an internal logic and we've already done that.

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
>

Xuan Zhuo March 15, 2024, 7:20 a.m. UTC | #4

On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> >
> > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > Acked-by: Johannes Berg <[e-mail]>
> > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > >
> > > The name seems broken here.
> >
> > Email APP bug.
> >
> > I will fix.
> >
> >
> > >
> > > [...]
> > >
> > > >
> > > >  typedef void vq_callback_t(struct virtqueue *);
> > > >
> > > > +/**
> > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > + *     item in the array should be used at that time. But since the item in
> > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > + *     know how to get the current configuration from the array when
> > > > + *     initializing vq.
> > >
> > > So this design is not good. If it is not something that the driver
> > > needs to care about, the core needs to hide it from the API.
> >
> > The driver just ignore it. That will be beneficial to the virtio core.
> > Otherwise, we must pass one more parameter everywhere.
>
> I don't get here, it's an internal logic and we've already done that.


## Then these must add one param "cfg_idx";

 struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
 					 unsigned int index,
 					 struct vq_transport_config *tp_cfg,
 					 struct virtio_vq_config *cfg,
--> 					 uint cfg_idx);

 struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
 				      unsigned int index,
 				      void *pages,
 				      struct vq_transport_config *tp_cfg,
 				      struct virtio_vq_config *cfg,
--> 					 uint cfg_idx);


## The functions inside virtio_ring also need to add a new param, such as:

 static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
						      unsigned int index,
						      struct vq_transport_config *tp_cfg,
						      struct virtio_vq_config,
--> 					              uint cfg_idx);



Thanks.




>
> Thanks
>
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> >
>

Jason Wang March 18, 2024, 4:18 a.m. UTC | #5

On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
>
> On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > >
> > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > >
> > > > The name seems broken here.
> > >
> > > Email APP bug.
> > >
> > > I will fix.
> > >
> > >
> > > >
> > > > [...]
> > > >
> > > > >
> > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > >
> > > > > +/**
> > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > + *     item in the array should be used at that time. But since the item in
> > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > + *     know how to get the current configuration from the array when
> > > > > + *     initializing vq.
> > > >
> > > > So this design is not good. If it is not something that the driver
> > > > needs to care about, the core needs to hide it from the API.
> > >
> > > The driver just ignore it. That will be beneficial to the virtio core.
> > > Otherwise, we must pass one more parameter everywhere.
> >
> > I don't get here, it's an internal logic and we've already done that.
>
>
> ## Then these must add one param "cfg_idx";
>
>  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
>                                          unsigned int index,
>                                          struct vq_transport_config *tp_cfg,
>                                          struct virtio_vq_config *cfg,
> -->                                      uint cfg_idx);
>
>  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
>                                       unsigned int index,
>                                       void *pages,
>                                       struct vq_transport_config *tp_cfg,
>                                       struct virtio_vq_config *cfg,
> -->                                      uint cfg_idx);
>
>
> ## The functions inside virtio_ring also need to add a new param, such as:
>
>  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
>                                                       unsigned int index,
>                                                       struct vq_transport_config *tp_cfg,
>                                                       struct virtio_vq_config,
> -->                                                   uint cfg_idx);
>
>
>

I guess what I'm missing is when could the index differ from cfg_idx?

Thanks

> Thanks.
>
>
>
>
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > > >
> > > > Thanks
> > > >
> > >
> >
>

Xuan Zhuo March 18, 2024, 5:59 a.m. UTC | #6

On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> >
> > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > >
> > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > >
> > > > > The name seems broken here.
> > > >
> > > > Email APP bug.
> > > >
> > > > I will fix.
> > > >
> > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > >
> > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > >
> > > > > > +/**
> > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > + *     know how to get the current configuration from the array when
> > > > > > + *     initializing vq.
> > > > >
> > > > > So this design is not good. If it is not something that the driver
> > > > > needs to care about, the core needs to hide it from the API.
> > > >
> > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > Otherwise, we must pass one more parameter everywhere.
> > >
> > > I don't get here, it's an internal logic and we've already done that.
> >
> >
> > ## Then these must add one param "cfg_idx";
> >
> >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> >                                          unsigned int index,
> >                                          struct vq_transport_config *tp_cfg,
> >                                          struct virtio_vq_config *cfg,
> > -->                                      uint cfg_idx);
> >
> >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> >                                       unsigned int index,
> >                                       void *pages,
> >                                       struct vq_transport_config *tp_cfg,
> >                                       struct virtio_vq_config *cfg,
> > -->                                      uint cfg_idx);
> >
> >
> > ## The functions inside virtio_ring also need to add a new param, such as:
> >
> >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> >                                                       unsigned int index,
> >                                                       struct vq_transport_config *tp_cfg,
> >                                                       struct virtio_vq_config,
> > -->                                                   uint cfg_idx);
> >
> >
> >
>
> I guess what I'm missing is when could the index differ from cfg_idx?


 @cfg_idx: Used by virtio core. The drivers should set this to 0.
     During the initialization of each vq(vring setup), we need to know which
     item in the array should be used at that time. But since the item in
     names can be null, which causes some item of array to be skipped, we
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     cannot use vq.index as the current id. So add a cfg_idx to let vring
     know how to get the current configuration from the array when
     initializing vq.


static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,

	................

	for (i = 0; i < nvqs; ++i) {
		if (!names[i]) {
			vqs[i] = NULL;
			continue;
		}

		if (!callbacks[i])
			msix_vec = VIRTIO_MSI_NO_VECTOR;
		else if (vp_dev->per_vq_vectors)
			msix_vec = allocated_vectors++;
		else
			msix_vec = VP_MSIX_VQ_VECTOR;
		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
				     ctx ? ctx[i] : false,
				     msix_vec);


Thanks.

>
> Thanks
>
> > Thanks.
> >
> >
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > >
> > >
> >
>

Michael S. Tsirkin March 19, 2024, 6:57 a.m. UTC | #7

On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > >
> > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > >
> > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > >
> > > > > > The name seems broken here.
> > > > >
> > > > > Email APP bug.
> > > > >
> > > > > I will fix.
> > > > >
> > > > >
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > >
> > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > >
> > > > > > > +/**
> > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > + *     initializing vq.
> > > > > >
> > > > > > So this design is not good. If it is not something that the driver
> > > > > > needs to care about, the core needs to hide it from the API.
> > > > >
> > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > Otherwise, we must pass one more parameter everywhere.
> > > >
> > > > I don't get here, it's an internal logic and we've already done that.
> > >
> > >
> > > ## Then these must add one param "cfg_idx";
> > >
> > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > >                                          unsigned int index,
> > >                                          struct vq_transport_config *tp_cfg,
> > >                                          struct virtio_vq_config *cfg,
> > > -->                                      uint cfg_idx);
> > >
> > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > >                                       unsigned int index,
> > >                                       void *pages,
> > >                                       struct vq_transport_config *tp_cfg,
> > >                                       struct virtio_vq_config *cfg,
> > > -->                                      uint cfg_idx);
> > >
> > >
> > > ## The functions inside virtio_ring also need to add a new param, such as:
> > >
> > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > >                                                       unsigned int index,
> > >                                                       struct vq_transport_config *tp_cfg,
> > >                                                       struct virtio_vq_config,
> > > -->                                                   uint cfg_idx);
> > >
> > >
> > >
> >
> > I guess what I'm missing is when could the index differ from cfg_idx?
> 
> 
>  @cfg_idx: Used by virtio core. The drivers should set this to 0.
>      During the initialization of each vq(vring setup), we need to know which
>      item in the array should be used at that time. But since the item in
>      names can be null, which causes some item of array to be skipped, we
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      cannot use vq.index as the current id. So add a cfg_idx to let vring
>      know how to get the current configuration from the array when
>      initializing vq.
> 
> 
> static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> 
> 	................
> 
> 	for (i = 0; i < nvqs; ++i) {
> 		if (!names[i]) {
> 			vqs[i] = NULL;
> 			continue;
> 		}
> 
> 		if (!callbacks[i])
> 			msix_vec = VIRTIO_MSI_NO_VECTOR;
> 		else if (vp_dev->per_vq_vectors)
> 			msix_vec = allocated_vectors++;
> 		else
> 			msix_vec = VP_MSIX_VQ_VECTOR;
> 		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> 				     ctx ? ctx[i] : false,
> 				     msix_vec);
> 
> 
> Thanks.


Jason what do you think is the way to resolve this?

> >
> > Thanks
> >
> > > Thanks.
> > >
> > >
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > >
> > > >
> > >
> >

Jason Wang March 20, 2024, 9:22 a.m. UTC | #8

On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
>
> On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > >
> > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > >
> > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > >
> > > > > > > The name seems broken here.
> > > > > >
> > > > > > Email APP bug.
> > > > > >
> > > > > > I will fix.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > >
> > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > + *     initializing vq.
> > > > > > >
> > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > >
> > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > >
> > > > > I don't get here, it's an internal logic and we've already done that.
> > > >
> > > >
> > > > ## Then these must add one param "cfg_idx";
> > > >
> > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > >                                          unsigned int index,
> > > >                                          struct vq_transport_config *tp_cfg,
> > > >                                          struct virtio_vq_config *cfg,
> > > > -->                                      uint cfg_idx);
> > > >
> > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > >                                       unsigned int index,
> > > >                                       void *pages,
> > > >                                       struct vq_transport_config *tp_cfg,
> > > >                                       struct virtio_vq_config *cfg,
> > > > -->                                      uint cfg_idx);
> > > >
> > > >
> > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > >
> > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > >                                                       unsigned int index,
> > > >                                                       struct vq_transport_config *tp_cfg,
> > > >                                                       struct virtio_vq_config,
> > > > -->                                                   uint cfg_idx);
> > > >
> > > >
> > > >
> > >
> > > I guess what I'm missing is when could the index differ from cfg_idx?
> >
> >
> >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> >      During the initialization of each vq(vring setup), we need to know which
> >      item in the array should be used at that time. But since the item in
> >      names can be null, which causes some item of array to be skipped, we
> >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> >      know how to get the current configuration from the array when
> >      initializing vq.
> >
> >
> > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> >
> >       ................
> >
> >       for (i = 0; i < nvqs; ++i) {
> >               if (!names[i]) {
> >                       vqs[i] = NULL;
> >                       continue;
> >               }
> >
> >               if (!callbacks[i])
> >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> >               else if (vp_dev->per_vq_vectors)
> >                       msix_vec = allocated_vectors++;
> >               else
> >                       msix_vec = VP_MSIX_VQ_VECTOR;
> >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> >                                    ctx ? ctx[i] : false,
> >                                    msix_vec);
> >
> >
> > Thanks.
>
>
> Jason what do you think is the way to resolve this?

I wonder which driver doesn't use a specific virtqueue in this case.

And it looks to me, introducing a per-vq-config struct might be better
then we have

virtio_vqs_config {
      unsigned int nvqs;
      struct virtio_vq_config *configs;
}

So we don't need the cfg_idx stuff.

Thanks

>
> > >
> > > Thanks
> > >
> > > > Thanks.
> > > >
> > > >
> > > >
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>

Jason Wang March 20, 2024, 9:31 a.m. UTC | #9

On Wed, Mar 20, 2024 at 5:22 PM Jason Wang <[e-mail]> wrote:
>
> On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
> >
> > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > > >
> > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > > >
> > > > > > > > The name seems broken here.
> > > > > > >
> > > > > > > Email APP bug.
> > > > > > >
> > > > > > > I will fix.
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > >
> > > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > > >
> > > > > > > > > +/**
> > > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > > + *     initializing vq.
> > > > > > > >
> > > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > > >
> > > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > > >
> > > > > > I don't get here, it's an internal logic and we've already done that.
> > > > >
> > > > >
> > > > > ## Then these must add one param "cfg_idx";
> > > > >
> > > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > > >                                          unsigned int index,
> > > > >                                          struct vq_transport_config *tp_cfg,
> > > > >                                          struct virtio_vq_config *cfg,
> > > > > -->                                      uint cfg_idx);
> > > > >
> > > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > > >                                       unsigned int index,
> > > > >                                       void *pages,
> > > > >                                       struct vq_transport_config *tp_cfg,
> > > > >                                       struct virtio_vq_config *cfg,
> > > > > -->                                      uint cfg_idx);
> > > > >
> > > > >
> > > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > > >
> > > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > > >                                                       unsigned int index,
> > > > >                                                       struct vq_transport_config *tp_cfg,
> > > > >                                                       struct virtio_vq_config,
> > > > > -->                                                   uint cfg_idx);
> > > > >
> > > > >
> > > > >
> > > >
> > > > I guess what I'm missing is when could the index differ from cfg_idx?
> > >
> > >
> > >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > >      During the initialization of each vq(vring setup), we need to know which
> > >      item in the array should be used at that time. But since the item in
> > >      names can be null, which causes some item of array to be skipped, we
> > >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> > >      know how to get the current configuration from the array when
> > >      initializing vq.
> > >
> > >
> > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> > >
> > >       ................
> > >
> > >       for (i = 0; i < nvqs; ++i) {
> > >               if (!names[i]) {
> > >                       vqs[i] = NULL;
> > >                       continue;
> > >               }
> > >
> > >               if (!callbacks[i])
> > >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> > >               else if (vp_dev->per_vq_vectors)
> > >                       msix_vec = allocated_vectors++;
> > >               else
> > >                       msix_vec = VP_MSIX_VQ_VECTOR;
> > >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> > >                                    ctx ? ctx[i] : false,
> > >                                    msix_vec);
> > >
> > >
> > > Thanks.
> >
> >
> > Jason what do you think is the way to resolve this?
>
> I wonder which driver doesn't use a specific virtqueue in this case.
>
> And it looks to me, introducing a per-vq-config struct might be better
> then we have
>
> virtio_vqs_config {
>       unsigned int nvqs;
>       struct virtio_vq_config *configs;
> }
>
> So we don't need the cfg_idx stuff.

And actually, I'm also ok to have cfg_idx internally, it's better than
having an API which has a field that the user doesn't need to care
about.

Thanks

>
> Thanks
>
> >
> > > >
> > > > Thanks
> > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> >

Xuan Zhuo March 20, 2024, 9:39 a.m. UTC | #10

On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <[e-mail]> wrote:
> On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
> >
> > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > > >
> > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > > >
> > > > > > > > The name seems broken here.
> > > > > > >
> > > > > > > Email APP bug.
> > > > > > >
> > > > > > > I will fix.
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > >
> > > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > > >
> > > > > > > > > +/**
> > > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > > + *     initializing vq.
> > > > > > > >
> > > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > > >
> > > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > > >
> > > > > > I don't get here, it's an internal logic and we've already done that.
> > > > >
> > > > >
> > > > > ## Then these must add one param "cfg_idx";
> > > > >
> > > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > > >                                          unsigned int index,
> > > > >                                          struct vq_transport_config *tp_cfg,
> > > > >                                          struct virtio_vq_config *cfg,
> > > > > -->                                      uint cfg_idx);
> > > > >
> > > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > > >                                       unsigned int index,
> > > > >                                       void *pages,
> > > > >                                       struct vq_transport_config *tp_cfg,
> > > > >                                       struct virtio_vq_config *cfg,
> > > > > -->                                      uint cfg_idx);
> > > > >
> > > > >
> > > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > > >
> > > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > > >                                                       unsigned int index,
> > > > >                                                       struct vq_transport_config *tp_cfg,
> > > > >                                                       struct virtio_vq_config,
> > > > > -->                                                   uint cfg_idx);
> > > > >
> > > > >
> > > > >
> > > >
> > > > I guess what I'm missing is when could the index differ from cfg_idx?
> > >
> > >
> > >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > >      During the initialization of each vq(vring setup), we need to know which
> > >      item in the array should be used at that time. But since the item in
> > >      names can be null, which causes some item of array to be skipped, we
> > >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> > >      know how to get the current configuration from the array when
> > >      initializing vq.
> > >
> > >
> > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> > >
> > >       ................
> > >
> > >       for (i = 0; i < nvqs; ++i) {
> > >               if (!names[i]) {
> > >                       vqs[i] = NULL;
> > >                       continue;
> > >               }
> > >
> > >               if (!callbacks[i])
> > >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> > >               else if (vp_dev->per_vq_vectors)
> > >                       msix_vec = allocated_vectors++;
> > >               else
> > >                       msix_vec = VP_MSIX_VQ_VECTOR;
> > >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> > >                                    ctx ? ctx[i] : false,
> > >                                    msix_vec);
> > >
> > >
> > > Thanks.
> >
> >
> > Jason what do you think is the way to resolve this?
>
> I wonder which driver doesn't use a specific virtqueue in this case.


commit 6457f126c888b3481fdae6f702e616cd0c79646e
Author: Michael S. Tsirkin <[e-mail]>
Date:   Wed Sep 5 21:47:45 2012 +0300

    virtio: support reserved vqs

    virtio network device multiqueue support reserves
    vq 3 for future use (useful both for future extensions and to make it
    pretty - this way receive vqs have even and transmit - odd numbers).
    Make it possible to skip initialization for
    specific vq numbers by specifying NULL for name.
    Document this usage as well as (existing) NULL callback.

    Drivers using this not coded up yet, so I simply tested
    with virtio-pci and verified that this patch does
    not break existing drivers.

    Signed-off-by: Michael S. Tsirkin <[e-mail]>
    Signed-off-by: Rusty Russell <[e-mail]>

This patch introduced this.

Could we remove this? Then we can use the vq.index directly. That will
be great.

>
> And it looks to me, introducing a per-vq-config struct might be better
> then we have
>
> virtio_vqs_config {
>       unsigned int nvqs;
>       struct virtio_vq_config *configs;
> }

YES. I prefer this. But we need to refactor every driver.

>
> So we don't need the cfg_idx stuff.

This still needs cfg_idx.

Thanks


>
> Thanks
>
> >
> > > >
> > > > Thanks
> > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> >
>

Xuan Zhuo March 20, 2024, 9:54 a.m. UTC | #11

On Wed, 20 Mar 2024 17:39:29 +0800, Xuan Zhuo <[e-mail]> wrote:
> On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <[e-mail]> wrote:
> > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
> > >
> > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > >
> > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > > > >
> > > > > > > > > The name seems broken here.
> > > > > > > >
> > > > > > > > Email APP bug.
> > > > > > > >
> > > > > > > > I will fix.
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > > > >
> > > > > > > > > > +/**
> > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > > > + *     initializing vq.
> > > > > > > > >
> > > > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > > > >
> > > > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > > > >
> > > > > > > I don't get here, it's an internal logic and we've already done that.
> > > > > >
> > > > > >
> > > > > > ## Then these must add one param "cfg_idx";
> > > > > >
> > > > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > > > >                                          unsigned int index,
> > > > > >                                          struct vq_transport_config *tp_cfg,
> > > > > >                                          struct virtio_vq_config *cfg,
> > > > > > -->                                      uint cfg_idx);
> > > > > >
> > > > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > > > >                                       unsigned int index,
> > > > > >                                       void *pages,
> > > > > >                                       struct vq_transport_config *tp_cfg,
> > > > > >                                       struct virtio_vq_config *cfg,
> > > > > > -->                                      uint cfg_idx);
> > > > > >
> > > > > >
> > > > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > > > >
> > > > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > > > >                                                       unsigned int index,
> > > > > >                                                       struct vq_transport_config *tp_cfg,
> > > > > >                                                       struct virtio_vq_config,
> > > > > > -->                                                   uint cfg_idx);
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > > I guess what I'm missing is when could the index differ from cfg_idx?
> > > >
> > > >
> > > >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > >      During the initialization of each vq(vring setup), we need to know which
> > > >      item in the array should be used at that time. But since the item in
> > > >      names can be null, which causes some item of array to be skipped, we
> > > >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > >      know how to get the current configuration from the array when
> > > >      initializing vq.
> > > >
> > > >
> > > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> > > >
> > > >       ................
> > > >
> > > >       for (i = 0; i < nvqs; ++i) {
> > > >               if (!names[i]) {
> > > >                       vqs[i] = NULL;
> > > >                       continue;
> > > >               }
> > > >
> > > >               if (!callbacks[i])
> > > >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> > > >               else if (vp_dev->per_vq_vectors)
> > > >                       msix_vec = allocated_vectors++;
> > > >               else
> > > >                       msix_vec = VP_MSIX_VQ_VECTOR;
> > > >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> > > >                                    ctx ? ctx[i] : false,
> > > >                                    msix_vec);
> > > >
> > > >
> > > > Thanks.
> > >
> > >
> > > Jason what do you think is the way to resolve this?
> >
> > I wonder which driver doesn't use a specific virtqueue in this case.
>
>
> commit 6457f126c888b3481fdae6f702e616cd0c79646e
> Author: Michael S. Tsirkin <[e-mail]>
> Date:   Wed Sep 5 21:47:45 2012 +0300
>
>     virtio: support reserved vqs
>
>     virtio network device multiqueue support reserves
>     vq 3 for future use (useful both for future extensions and to make it
>     pretty - this way receive vqs have even and transmit - odd numbers).
>     Make it possible to skip initialization for
>     specific vq numbers by specifying NULL for name.
>     Document this usage as well as (existing) NULL callback.
>
>     Drivers using this not coded up yet, so I simply tested
>     with virtio-pci and verified that this patch does
>     not break existing drivers.
>
>     Signed-off-by: Michael S. Tsirkin <[e-mail]>
>     Signed-off-by: Rusty Russell <[e-mail]>
>
> This patch introduced this.
>
> Could we remove this? Then we can use the vq.index directly. That will
> be great.

I checked the current kernel.
Just virtio-ballon uses this feture. But we can fix by one line code.

Thanks.


>
> >
> > And it looks to me, introducing a per-vq-config struct might be better
> > then we have
> >
> > virtio_vqs_config {
> >       unsigned int nvqs;
> >       struct virtio_vq_config *configs;
> > }
>
> YES. I prefer this. But we need to refactor every driver.
>
> >
> > So we don't need the cfg_idx stuff.
>
> This still needs cfg_idx.
>
> Thanks
>
>
> >
> > Thanks
> >
> > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
>

Jason Wang March 21, 2024, 4:02 a.m. UTC | #12

On Wed, Mar 20, 2024 at 6:07 PM Xuan Zhuo <[e-mail]> wrote:
>
> On Wed, 20 Mar 2024 17:39:29 +0800, Xuan Zhuo <[e-mail]> wrote:
> > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <[e-mail]> wrote:
> > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
> > > >
> > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > > > > >
> > > > > > > > > > The name seems broken here.
> > > > > > > > >
> > > > > > > > > Email APP bug.
> > > > > > > > >
> > > > > > > > > I will fix.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > [...]
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > > > > >
> > > > > > > > > > > +/**
> > > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > > > > + *     initializing vq.
> > > > > > > > > >
> > > > > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > > > > >
> > > > > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > > > > >
> > > > > > > > I don't get here, it's an internal logic and we've already done that.
> > > > > > >
> > > > > > >
> > > > > > > ## Then these must add one param "cfg_idx";
> > > > > > >
> > > > > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > > > > >                                          unsigned int index,
> > > > > > >                                          struct vq_transport_config *tp_cfg,
> > > > > > >                                          struct virtio_vq_config *cfg,
> > > > > > > -->                                      uint cfg_idx);
> > > > > > >
> > > > > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > > > > >                                       unsigned int index,
> > > > > > >                                       void *pages,
> > > > > > >                                       struct vq_transport_config *tp_cfg,
> > > > > > >                                       struct virtio_vq_config *cfg,
> > > > > > > -->                                      uint cfg_idx);
> > > > > > >
> > > > > > >
> > > > > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > > > > >
> > > > > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > > > > >                                                       unsigned int index,
> > > > > > >                                                       struct vq_transport_config *tp_cfg,
> > > > > > >                                                       struct virtio_vq_config,
> > > > > > > -->                                                   uint cfg_idx);
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > I guess what I'm missing is when could the index differ from cfg_idx?
> > > > >
> > > > >
> > > > >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > >      During the initialization of each vq(vring setup), we need to know which
> > > > >      item in the array should be used at that time. But since the item in
> > > > >      names can be null, which causes some item of array to be skipped, we
> > > > >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > >      know how to get the current configuration from the array when
> > > > >      initializing vq.
> > > > >
> > > > >
> > > > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> > > > >
> > > > >       ................
> > > > >
> > > > >       for (i = 0; i < nvqs; ++i) {
> > > > >               if (!names[i]) {
> > > > >                       vqs[i] = NULL;
> > > > >                       continue;
> > > > >               }
> > > > >
> > > > >               if (!callbacks[i])
> > > > >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> > > > >               else if (vp_dev->per_vq_vectors)
> > > > >                       msix_vec = allocated_vectors++;
> > > > >               else
> > > > >                       msix_vec = VP_MSIX_VQ_VECTOR;
> > > > >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> > > > >                                    ctx ? ctx[i] : false,
> > > > >                                    msix_vec);
> > > > >
> > > > >
> > > > > Thanks.
> > > >
> > > >
> > > > Jason what do you think is the way to resolve this?
> > >
> > > I wonder which driver doesn't use a specific virtqueue in this case.
> >
> >
> > commit 6457f126c888b3481fdae6f702e616cd0c79646e
> > Author: Michael S. Tsirkin <[e-mail]>
> > Date:   Wed Sep 5 21:47:45 2012 +0300
> >
> >     virtio: support reserved vqs
> >
> >     virtio network device multiqueue support reserves
> >     vq 3 for future use (useful both for future extensions and to make it
> >     pretty - this way receive vqs have even and transmit - odd numbers).
> >     Make it possible to skip initialization for
> >     specific vq numbers by specifying NULL for name.
> >     Document this usage as well as (existing) NULL callback.
> >
> >     Drivers using this not coded up yet, so I simply tested
> >     with virtio-pci and verified that this patch does
> >     not break existing drivers.
> >
> >     Signed-off-by: Michael S. Tsirkin <[e-mail]>
> >     Signed-off-by: Rusty Russell <[e-mail]>
> >
> > This patch introduced this.
> >
> > Could we remove this? Then we can use the vq.index directly. That will
> > be great.

I think we can, as multiqueue virtio-net use 2N as ctrl vq finally, so
the logic doesn't apply.

>
> I checked the current kernel.
> Just virtio-ballon uses this feture. But we can fix by one line code.

That would be fine.

Thanks

>
> Thanks.
>
>
> >
> > >
> > > And it looks to me, introducing a per-vq-config struct might be better
> > > then we have
> > >
> > > virtio_vqs_config {
> > >       unsigned int nvqs;
> > >       struct virtio_vq_config *configs;
> > > }
> >
> > YES. I prefer this. But we need to refactor every driver.
> >
> > >
> > > So we don't need the cfg_idx stuff.
> >
> > This still needs cfg_idx.
> >
> > Thanks
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
>

Jason Wang March 21, 2024, 4:03 a.m. UTC | #13

On Wed, Mar 20, 2024 at 5:41 PM Xuan Zhuo <[e-mail]> wrote:
>
> On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <[e-mail]> wrote:
> > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
> > >
> > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > >
> > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > > > >
> > > > > > > > > The name seems broken here.
> > > > > > > >
> > > > > > > > Email APP bug.
> > > > > > > >
> > > > > > > > I will fix.
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > > > >
> > > > > > > > > > +/**
> > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > > > + *     initializing vq.
> > > > > > > > >
> > > > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > > > >
> > > > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > > > >
> > > > > > > I don't get here, it's an internal logic and we've already done that.
> > > > > >
> > > > > >
> > > > > > ## Then these must add one param "cfg_idx";
> > > > > >
> > > > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > > > >                                          unsigned int index,
> > > > > >                                          struct vq_transport_config *tp_cfg,
> > > > > >                                          struct virtio_vq_config *cfg,
> > > > > > -->                                      uint cfg_idx);
> > > > > >
> > > > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > > > >                                       unsigned int index,
> > > > > >                                       void *pages,
> > > > > >                                       struct vq_transport_config *tp_cfg,
> > > > > >                                       struct virtio_vq_config *cfg,
> > > > > > -->                                      uint cfg_idx);
> > > > > >
> > > > > >
> > > > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > > > >
> > > > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > > > >                                                       unsigned int index,
> > > > > >                                                       struct vq_transport_config *tp_cfg,
> > > > > >                                                       struct virtio_vq_config,
> > > > > > -->                                                   uint cfg_idx);
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > > I guess what I'm missing is when could the index differ from cfg_idx?
> > > >
> > > >
> > > >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > >      During the initialization of each vq(vring setup), we need to know which
> > > >      item in the array should be used at that time. But since the item in
> > > >      names can be null, which causes some item of array to be skipped, we
> > > >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > >      know how to get the current configuration from the array when
> > > >      initializing vq.
> > > >
> > > >
> > > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> > > >
> > > >       ................
> > > >
> > > >       for (i = 0; i < nvqs; ++i) {
> > > >               if (!names[i]) {
> > > >                       vqs[i] = NULL;
> > > >                       continue;
> > > >               }
> > > >
> > > >               if (!callbacks[i])
> > > >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> > > >               else if (vp_dev->per_vq_vectors)
> > > >                       msix_vec = allocated_vectors++;
> > > >               else
> > > >                       msix_vec = VP_MSIX_VQ_VECTOR;
> > > >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> > > >                                    ctx ? ctx[i] : false,
> > > >                                    msix_vec);
> > > >
> > > >
> > > > Thanks.
> > >
> > >
> > > Jason what do you think is the way to resolve this?
> >
> > I wonder which driver doesn't use a specific virtqueue in this case.
>
>
> commit 6457f126c888b3481fdae6f702e616cd0c79646e
> Author: Michael S. Tsirkin <[e-mail]>
> Date:   Wed Sep 5 21:47:45 2012 +0300
>
>     virtio: support reserved vqs
>
>     virtio network device multiqueue support reserves
>     vq 3 for future use (useful both for future extensions and to make it
>     pretty - this way receive vqs have even and transmit - odd numbers).
>     Make it possible to skip initialization for
>     specific vq numbers by specifying NULL for name.
>     Document this usage as well as (existing) NULL callback.
>
>     Drivers using this not coded up yet, so I simply tested
>     with virtio-pci and verified that this patch does
>     not break existing drivers.
>
>     Signed-off-by: Michael S. Tsirkin <[e-mail]>
>     Signed-off-by: Rusty Russell <[e-mail]>
>
> This patch introduced this.
>
> Could we remove this? Then we can use the vq.index directly. That will
> be great.
>
> >
> > And it looks to me, introducing a per-vq-config struct might be better
> > then we have
> >
> > virtio_vqs_config {
> >       unsigned int nvqs;
> >       struct virtio_vq_config *configs;
> > }
>
> YES. I prefer this. But we need to refactor every driver.

Yes.

>
> >
> > So we don't need the cfg_idx stuff.
>
> This still needs cfg_idx.

Drive needs to pass config for each virtqueue. We can still check
config->name so the virtqueue index could be used.

Thanks

>
> Thanks
>
>
> >
> > Thanks
> >
> > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> >
>

Xuan Zhuo March 21, 2024, 4:11 a.m. UTC | #14

On Thu, 21 Mar 2024 12:03:36 +0800, Jason Wang <[e-mail]> wrote:
> On Wed, Mar 20, 2024 at 5:41 PM Xuan Zhuo <[e-mail]> wrote:
> >
> > On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <[e-mail]> wrote:
> > > On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <[e-mail]> wrote:
> > > >
> > > > On Mon, Mar 18, 2024 at 01:59:52PM +0800, Xuan Zhuo wrote:
> > > > > On Mon, 18 Mar 2024 12:18:23 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <[e-mail]> wrote:
> > > > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <[e-mail]> 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 <[e-mail]>
> > > > > > > > > > > Acked-by: Johannes Berg <[e-mail]>
> > > > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <[e-mail]>
> > > > > > > > > >
> > > > > > > > > > The name seems broken here.
> > > > > > > > >
> > > > > > > > > Email APP bug.
> > > > > > > > >
> > > > > > > > > I will fix.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > [...]
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >  typedef void vq_callback_t(struct virtqueue *);
> > > > > > > > > > >
> > > > > > > > > > > +/**
> > > > > > > > > > > + * struct virtio_vq_config - configure for find_vqs()
> > > > > > > > > > > + * @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > > > > > > > > + *     During the initialization of each vq(vring setup), we need to know which
> > > > > > > > > > > + *     item in the array should be used at that time. But since the item in
> > > > > > > > > > > + *     names can be null, which causes some item of array to be skipped, we
> > > > > > > > > > > + *     cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > > > > > > > > + *     know how to get the current configuration from the array when
> > > > > > > > > > > + *     initializing vq.
> > > > > > > > > >
> > > > > > > > > > So this design is not good. If it is not something that the driver
> > > > > > > > > > needs to care about, the core needs to hide it from the API.
> > > > > > > > >
> > > > > > > > > The driver just ignore it. That will be beneficial to the virtio core.
> > > > > > > > > Otherwise, we must pass one more parameter everywhere.
> > > > > > > >
> > > > > > > > I don't get here, it's an internal logic and we've already done that.
> > > > > > >
> > > > > > >
> > > > > > > ## Then these must add one param "cfg_idx";
> > > > > > >
> > > > > > >  struct virtqueue *vring_create_virtqueue(struct virtio_device *vdev,
> > > > > > >                                          unsigned int index,
> > > > > > >                                          struct vq_transport_config *tp_cfg,
> > > > > > >                                          struct virtio_vq_config *cfg,
> > > > > > > -->                                      uint cfg_idx);
> > > > > > >
> > > > > > >  struct virtqueue *vring_new_virtqueue(struct virtio_device *vdev,
> > > > > > >                                       unsigned int index,
> > > > > > >                                       void *pages,
> > > > > > >                                       struct vq_transport_config *tp_cfg,
> > > > > > >                                       struct virtio_vq_config *cfg,
> > > > > > > -->                                      uint cfg_idx);
> > > > > > >
> > > > > > >
> > > > > > > ## The functions inside virtio_ring also need to add a new param, such as:
> > > > > > >
> > > > > > >  static struct virtqueue *vring_create_virtqueue_split(struct virtio_device *vdev,
> > > > > > >                                                       unsigned int index,
> > > > > > >                                                       struct vq_transport_config *tp_cfg,
> > > > > > >                                                       struct virtio_vq_config,
> > > > > > > -->                                                   uint cfg_idx);
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > I guess what I'm missing is when could the index differ from cfg_idx?
> > > > >
> > > > >
> > > > >  @cfg_idx: Used by virtio core. The drivers should set this to 0.
> > > > >      During the initialization of each vq(vring setup), we need to know which
> > > > >      item in the array should be used at that time. But since the item in
> > > > >      names can be null, which causes some item of array to be skipped, we
> > > > >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > >      cannot use vq.index as the current id. So add a cfg_idx to let vring
> > > > >      know how to get the current configuration from the array when
> > > > >      initializing vq.
> > > > >
> > > > >
> > > > > static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> > > > >
> > > > >       ................
> > > > >
> > > > >       for (i = 0; i < nvqs; ++i) {
> > > > >               if (!names[i]) {
> > > > >                       vqs[i] = NULL;
> > > > >                       continue;
> > > > >               }
> > > > >
> > > > >               if (!callbacks[i])
> > > > >                       msix_vec = VIRTIO_MSI_NO_VECTOR;
> > > > >               else if (vp_dev->per_vq_vectors)
> > > > >                       msix_vec = allocated_vectors++;
> > > > >               else
> > > > >                       msix_vec = VP_MSIX_VQ_VECTOR;
> > > > >               vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> > > > >                                    ctx ? ctx[i] : false,
> > > > >                                    msix_vec);
> > > > >
> > > > >
> > > > > Thanks.
> > > >
> > > >
> > > > Jason what do you think is the way to resolve this?
> > >
> > > I wonder which driver doesn't use a specific virtqueue in this case.
> >
> >
> > commit 6457f126c888b3481fdae6f702e616cd0c79646e
> > Author: Michael S. Tsirkin <[e-mail]>
> > Date:   Wed Sep 5 21:47:45 2012 +0300
> >
> >     virtio: support reserved vqs
> >
> >     virtio network device multiqueue support reserves
> >     vq 3 for future use (useful both for future extensions and to make it
> >     pretty - this way receive vqs have even and transmit - odd numbers).
> >     Make it possible to skip initialization for
> >     specific vq numbers by specifying NULL for name.
> >     Document this usage as well as (existing) NULL callback.
> >
> >     Drivers using this not coded up yet, so I simply tested
> >     with virtio-pci and verified that this patch does
> >     not break existing drivers.
> >
> >     Signed-off-by: Michael S. Tsirkin <[e-mail]>
> >     Signed-off-by: Rusty Russell <[e-mail]>
> >
> > This patch introduced this.
> >
> > Could we remove this? Then we can use the vq.index directly. That will
> > be great.
> >
> > >
> > > And it looks to me, introducing a per-vq-config struct might be better
> > > then we have
> > >
> > > virtio_vqs_config {
> > >       unsigned int nvqs;
> > >       struct virtio_vq_config *configs;
> > > }
> >
> > YES. I prefer this. But we need to refactor every driver.
>
> Yes.
>
> >
> > >
> > > So we don't need the cfg_idx stuff.
> >
> > This still needs cfg_idx.
>
> Drive needs to pass config for each virtqueue. We can still check
> config->name so the virtqueue index could be used.

I see. If we pass the config for one queue to virtio ring,
then we do not to pass the idx.

But for now, I will remove the logic of checking name,
then we can use vq->index inside virtio ring.

Thanks.


>
> Thanks
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
>

Patch

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,
+				    vu_notify,
+				    cfg->callbacks[cfg->cfg_idx], 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;
 	int i, queue_idx = 0, rc;
 	struct virtqueue *vq;
 
@@ -1031,13 +1032,13 @@  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]) {
 			vqs[i] = NULL;
 			continue;
 		}
 
-		vqs[i] = vu_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
-				     ctx ? ctx[i] : false);
+		cfg->cfg_idx = i;
+		vqs[i] = vu_setup_vq(vdev, queue_idx++, 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 83d76915a6ad..57d51c9c7b63 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);
@@ -119,7 +118,7 @@  static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	if (id >= ARRAY_SIZE(rvdev->vring))
 		return ERR_PTR(-EINVAL);
 
-	if (!name)
+	if (!cfg->names[cfg->cfg_idx])
 		return NULL;
 
 	/* Search allocated memory region by name */
@@ -143,10 +142,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[cfg->cfg_idx] : false,
+				 addr, rproc_virtio_notify, cfg->callbacks[cfg->cfg_idx],
+				 cfg->names[cfg->cfg_idx]);
 	if (!vq) {
-		dev_err(dev, "vring_new_virtqueue %s failed\n", name);
+		dev_err(dev, "vring_new_virtqueue %s failed\n", cfg->names[cfg->cfg_idx]);
 		rproc_free_vring(rvring);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -180,23 +181,20 @@  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, queue_idx = 0;
 
 	for (i = 0; i < nvqs; ++i) {
-		if (!names[i]) {
+		if (!cfg->names[i]) {
 			vqs[i] = NULL;
 			continue;
 		}
 
-		vqs[i] = rp_find_vq(vdev, queue_idx++, callbacks[i], names[i],
-				    ctx ? ctx[i] : false);
+		cfg->cfg_idx = i;
+		vqs[i] = rp_find_vq(vdev, queue_idx++, 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 ac67576301bf..11eea5086cff 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[cfg->cfg_idx] : false,
+				    notify,
+				    cfg->callbacks[cfg->cfg_idx],
+				    cfg->names[cfg->cfg_idx]);
 
 	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, queue_idx = 0;
 	struct ccw1 *ccw;
 
@@ -667,14 +667,13 @@  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]) {
 			vqs[i] = NULL;
 			continue;
 		}
 
-		vqs[i] = virtio_ccw_setup_vq(vdev, queue_idx++, callbacks[i],
-					     names[i], ctx ? ctx[i] : false,
-					     ccw);
+		cfg->cfg_idx = i;
+		vqs[i] = virtio_ccw_setup_vq(vdev, queue_idx++, 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 59892a31cf76..feb823d279d2 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,7 +385,7 @@  static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
 	else
 		notify = vm_notify;
 
-	if (!name)
+	if (!cfg->names[index])
 		return NULL;
 
 	/* Select the queue we're interested in */
@@ -414,7 +413,11 @@  static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
 
 	/* Create the vring */
 	vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev,
-				 true, true, ctx, notify, callback, name);
+				 true, true,
+				 cfg->ctx ? cfg->ctx[cfg->cfg_idx] : false,
+				 notify,
+				 cfg->callbacks[cfg->cfg_idx],
+				 cfg->names[cfg->cfg_idx]);
 	if (!vq) {
 		err = -ENOMEM;
 		goto error_new_virtqueue;
@@ -487,15 +490,12 @@  static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
 	return ERR_PTR(err);
 }
 
-static int vm_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 vm_find_vqs(struct virtio_device *vdev, struct virtio_vq_config *cfg)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
 	int irq = platform_get_irq(vm_dev->pdev, 0);
+	struct virtqueue **vqs = cfg->vqs;
+	unsigned int nvqs = cfg->nvqs;
 	int i, err, queue_idx = 0;
 
 	if (irq < 0)
@@ -510,13 +510,13 @@  static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 		enable_irq_wake(irq);
 
 	for (i = 0; i < nvqs; ++i) {
-		if (!names[i]) {
+		if (!cfg->names[i]) {
 			vqs[i] = NULL;
 			continue;
 		}
 
-		vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
-				     ctx ? ctx[i] : false);
+		cfg->cfg_idx = i;
+		vqs[i] = vm_setup_vq(vdev, queue_idx++, cfg);
 		if (IS_ERR(vqs[i])) {
 			vm_del_vqs(vdev);
 			return PTR_ERR(vqs[i]);
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index b655fccaf773..775537234b00 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,12 @@  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,
-			      msix_vec);
+	vq = vp_dev->setup_vq(vp_dev, info, index, cfg, msix_vec);
 	if (IS_ERR(vq))
 		goto out_info;
 
 	info->vq = vq;
-	if (callback) {
+	if (cfg->callbacks[cfg->cfg_idx]) {
 		spin_lock_irqsave(&vp_dev->lock, flags);
 		list_add(&info->node, &vp_dev->virtqueues);
 		spin_unlock_irqrestore(&vp_dev->lock, flags);
@@ -284,15 +281,15 @@  void vp_del_vqs(struct virtio_device *vdev)
 	vp_dev->vqs = NULL;
 }
 
-static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
-		struct virtqueue *vqs[], vq_callback_t *callbacks[],
-		const char * const names[], bool per_vq_vectors,
-		const bool *ctx,
-		struct irq_affinity *desc)
+static int vp_find_vqs_msix(struct virtio_device *vdev,
+			    struct virtio_vq_config *cfg,
+			    bool per_vq_vectors)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	u16 msix_vec;
 	int i, err, nvectors, allocated_vectors, queue_idx = 0;
+	struct virtqueue **vqs = cfg->vqs;
+	unsigned int nvqs = cfg->nvqs;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
@@ -302,7 +299,7 @@  static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 		/* Best option: one for change interrupt, one per vq. */
 		nvectors = 1;
 		for (i = 0; i < nvqs; ++i)
-			if (names[i] && callbacks[i])
+			if (cfg->names[i] && cfg->callbacks[i])
 				++nvectors;
 	} else {
 		/* Second best: one for change, shared for all vqs. */
@@ -310,27 +307,27 @@  static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 	}
 
 	err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors,
-				      per_vq_vectors ? desc : NULL);
+				      per_vq_vectors ? cfg->desc : NULL);
 	if (err)
 		goto error_find;
 
 	vp_dev->per_vq_vectors = per_vq_vectors;
 	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
-		if (!names[i]) {
+		if (!cfg->names[i]) {
 			vqs[i] = NULL;
 			continue;
 		}
 
-		if (!callbacks[i])
+		if (!cfg->callbacks[i])
 			msix_vec = VIRTIO_MSI_NO_VECTOR;
 		else if (vp_dev->per_vq_vectors)
 			msix_vec = allocated_vectors++;
 		else
 			msix_vec = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
-				     ctx ? ctx[i] : false,
-				     msix_vec);
+
+		cfg->cfg_idx = i;
+		vqs[i] = vp_setup_vq(vdev, queue_idx++, cfg, msix_vec);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
@@ -343,7 +340,7 @@  static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 		snprintf(vp_dev->msix_names[msix_vec],
 			 sizeof *vp_dev->msix_names,
 			 "%s-%s",
-			 dev_name(&vp_dev->vdev.dev), names[i]);
+			 dev_name(&vp_dev->vdev.dev), cfg->names[i]);
 		err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
 				  vring_interrupt, 0,
 				  vp_dev->msix_names[msix_vec],
@@ -358,11 +355,11 @@  static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 	return err;
 }
 
-static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
-		struct virtqueue *vqs[], vq_callback_t *callbacks[],
-		const char * const names[], const bool *ctx)
+static int vp_find_vqs_intx(struct virtio_device *vdev, struct virtio_vq_config *cfg)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	struct virtqueue **vqs = cfg->vqs;
+	unsigned int nvqs = cfg->nvqs;
 	int i, err, queue_idx = 0;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
@@ -377,13 +374,13 @@  static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 	vp_dev->intx_enabled = 1;
 	vp_dev->per_vq_vectors = false;
 	for (i = 0; i < nvqs; ++i) {
-		if (!names[i]) {
+		if (!cfg->names[i]) {
 			vqs[i] = NULL;
 			continue;
 		}
-		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
-				     ctx ? ctx[i] : false,
-				     VIRTIO_MSI_NO_VECTOR);
+
+		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;
@@ -397,26 +394,23 @@  static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 }
 
 /* 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)
 {
 	int err;
 
 	/* Try MSI-X with one vector per queue. */
-	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx, desc);
+	err = vp_find_vqs_msix(vdev, cfg, true);
 	if (!err)
 		return 0;
 	/* Fallback: MSI-X with one vector for config, one shared for queues. */
-	err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
+	err = vp_find_vqs_msix(vdev, cfg, false);
 	if (!err)
 		return 0;
 	/* Is there an interrupt? If not give up. */
 	if (!(to_vp_device(vdev)->pci_dev->irq))
 		return err;
 	/* Finally fall back to regular interrupts. */
-	return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
+	return vp_find_vqs_intx(vdev, cfg);
 }
 
 const char *vp_bus_name(struct virtio_device *vdev)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 7fef52bee455..5ba8b82fb765 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -95,9 +95,7 @@  struct virtio_pci_device {
 	struct virtqueue *(*setup_vq)(struct virtio_pci_device *vp_dev,
 				      struct virtio_pci_vq_info *info,
 				      unsigned int idx,
-				      void (*callback)(struct virtqueue *vq),
-				      const char *name,
-				      bool ctx,
+				      struct virtio_vq_config *vq_cfg,
 				      u16 msix_vec);
 	void (*del_vq)(struct virtio_pci_vq_info *info);
 
@@ -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);
 const char *vp_bus_name(struct virtio_device *vdev);
 
 /* Setup the affinity for a virtqueue:
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index d9cbb02b35a1..e8d22fce32f5 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -110,9 +110,7 @@  static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
 static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  struct virtio_pci_vq_info *info,
 				  unsigned int index,
-				  void (*callback)(struct virtqueue *vq),
-				  const char *name,
-				  bool ctx,
+				  struct virtio_vq_config *cfg,
 				  u16 msix_vec)
 {
 	struct virtqueue *vq;
@@ -130,8 +128,11 @@  static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	/* create the vring */
 	vq = vring_create_virtqueue(index, num,
 				    VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,
-				    true, false, ctx,
-				    vp_notify, callback, name);
+				    true, false,
+				    cfg->ctx ? cfg->ctx[cfg->cfg_idx] : false,
+				    vp_notify,
+				    cfg->callbacks[cfg->cfg_idx],
+				    cfg->names[cfg->cfg_idx]);
 	if (!vq)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index f62b530aa3b5..b2cdf5d3824d 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -530,9 +530,7 @@  static bool vp_notify_with_data(struct virtqueue *vq)
 static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  struct virtio_pci_vq_info *info,
 				  unsigned int index,
-				  void (*callback)(struct virtqueue *vq),
-				  const char *name,
-				  bool ctx,
+				  struct virtio_vq_config *cfg,
 				  u16 msix_vec)
 {
 
@@ -563,8 +561,11 @@  static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	/* create the vring */
 	vq = vring_create_virtqueue(index, num,
 				    SMP_CACHE_BYTES, &vp_dev->vdev,
-				    true, true, ctx,
-				    notify, callback, name);
+				    true, true,
+				    cfg->ctx ? cfg->ctx[cfg->cfg_idx] : false,
+				    notify,
+				    cfg->callbacks[cfg->cfg_idx],
+				    cfg->names[cfg->cfg_idx]);
 	if (!vq)
 		return ERR_PTR(-ENOMEM);
 
@@ -593,15 +594,11 @@  static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	return ERR_PTR(err);
 }
 
-static int vp_modern_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 vp_modern_find_vqs(struct virtio_device *vdev, struct virtio_vq_config *cfg)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtqueue *vq;
-	int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names, ctx, desc);
+	int rc = vp_find_vqs(vdev, cfg);
 
 	if (rc)
 		return rc;
@@ -739,10 +736,17 @@  static bool vp_get_shm_region(struct virtio_device *vdev,
 static int vp_modern_create_avq(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	vq_callback_t *callbacks[] = { NULL };
+	struct virtio_vq_config cfg = {};
 	struct virtio_pci_admin_vq *avq;
 	struct virtqueue *vq;
+	const char *names[1];
 	u16 admin_q_num;
 
+	cfg.nvqs = 1;
+	cfg.callbacks = callbacks;
+	cfg.names = names;
+
 	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
 		return 0;
 
@@ -753,8 +757,11 @@  static int vp_modern_create_avq(struct virtio_device *vdev)
 	avq = &vp_dev->admin_vq;
 	avq->vq_index = vp_modern_avq_index(&vp_dev->mdev);
 	sprintf(avq->name, "avq.%u", avq->vq_index);
-	vq = vp_dev->setup_vq(vp_dev, &vp_dev->admin_vq.info, avq->vq_index, NULL,
-			      avq->name, NULL, VIRTIO_MSI_NO_VECTOR);
+
+	cfg.names[0] = avq->name;
+	cfg.cfg_idx = 0;
+	vq = vp_dev->setup_vq(vp_dev, &vp_dev->admin_vq.info, avq->vq_index,
+			      &cfg, VIRTIO_MSI_NO_VECTOR);
 	if (IS_ERR(vq)) {
 		dev_err(&vdev->dev, "failed to setup admin virtqueue, err=%ld",
 			PTR_ERR(vq));
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index e803db0da307..7f3e173f669c 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -142,8 +142,7 @@  static irqreturn_t virtio_vdpa_virtqueue_cb(void *private)
 
 static struct virtqueue *
 virtio_vdpa_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_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
@@ -161,7 +160,7 @@  virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 	bool may_reduce_num = true;
 	int err;
 
-	if (!name)
+	if (!cfg->names[cfg->cfg_idx])
 		return NULL;
 
 	if (index >= vdpa->nvqs)
@@ -206,8 +205,12 @@  virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 	else
 		dma_dev = vdpa_get_dma_dev(vdpa);
 	vq = vring_create_virtqueue_dma(index, max_num, align, vdev,
-					true, may_reduce_num, ctx,
-					notify, callback, name, dma_dev);
+					true, may_reduce_num,
+					cfg->ctx ? cfg->ctx[cfg->cfg_idx] : false,
+					notify,
+					cfg->callbacks[cfg->cfg_idx],
+					cfg->names[cfg->cfg_idx],
+					dma_dev);
 	if (!vq) {
 		err = -ENOMEM;
 		goto error_new_virtqueue;
@@ -216,7 +219,7 @@  virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 	vq->num_max = max_num;
 
 	/* Setup virtqueue callback */
-	cb.callback = callback ? virtio_vdpa_virtqueue_cb : NULL;
+	cb.callback = cfg->callbacks[cfg->cfg_idx] ? virtio_vdpa_virtqueue_cb : NULL;
 	cb.private = info;
 	cb.trigger = NULL;
 	ops->set_vq_cb(vdpa, index, &cb);
@@ -356,12 +359,8 @@  create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
 	return masks;
 }
 
-static int virtio_vdpa_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 virtio_vdpa_find_vqs(struct virtio_device *vdev,
+				struct virtio_vq_config *cfg)
 {
 	struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
@@ -369,24 +368,25 @@  static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 	struct irq_affinity default_affd = { 0 };
 	struct cpumask *masks;
 	struct vdpa_callback cb;
-	bool has_affinity = desc && ops->set_vq_affinity;
+	bool has_affinity = cfg->desc && ops->set_vq_affinity;
+	struct virtqueue **vqs = cfg->vqs;
+	unsigned int nvqs = cfg->nvqs;
 	int i, err, queue_idx = 0;
 
 	if (has_affinity) {
-		masks = create_affinity_masks(nvqs, desc ? desc : &default_affd);
+		masks = create_affinity_masks(nvqs, cfg->desc ? cfg->desc : &default_affd);
 		if (!masks)
 			return -ENOMEM;
 	}
 
 	for (i = 0; i < nvqs; ++i) {
-		if (!names[i]) {
+		if (!cfg->names[i]) {
 			vqs[i] = NULL;
 			continue;
 		}
 
-		vqs[i] = virtio_vdpa_setup_vq(vdev, queue_idx++,
-					      callbacks[i], names[i], ctx ?
-					      ctx[i] : false);
+		cfg->cfg_idx = i;
+		vqs[i] = virtio_vdpa_setup_vq(vdev, queue_idx++, cfg);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto err_setup_vq;
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index da9b271b54db..33a6d9b068c1 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -18,6 +18,38 @@  struct virtio_shm_region {
 
 typedef void vq_callback_t(struct virtqueue *);
 
+/**
+ * struct virtio_vq_config - configure for find_vqs()
+ * @cfg_idx: Used by virtio core. The drivers should set this to 0.
+ *	During the initialization of each vq(vring setup), we need to know which
+ *	item in the array should be used at that time. But since the item in
+ *	names can be null, which causes some item of array to be skipped, we
+ *	cannot use vq.index as the current id. So add a cfg_idx to let vring
+ *	know how to get the current configuration from the array when
+ *	initializing vq.
+ * @nvqs: the number of virtqueues to find
+ * @vqs: on success, includes new virtqueues
+ * @callbacks: array of callbacks, for each virtqueue
+ *	include a NULL entry for vqs that do not need a callback
+ * @names: array of virtqueue names (mainly for debugging)
+ *	include a NULL entry for vqs unused by driver
+ * @ctx: (optional) array of context. If the value of the vq in the array
+ *	is true, the driver can pass ctx to virtio core when adding bufs to
+ *	virtqueue.
+ * @desc: desc for interrupts
+ */
+struct virtio_vq_config {
+	unsigned int cfg_idx;
+
+	unsigned int nvqs;
+
+	struct virtqueue   **vqs;
+	vq_callback_t      **callbacks;
+	const char         **names;
+	const bool          *ctx;
+	struct irq_affinity *desc;
+};
+
 /**
  * struct virtio_config_ops - operations for configuring a virtio device
  * Note: Do not assume that a transport implements all of the operations
@@ -51,12 +83,7 @@  typedef void vq_callback_t(struct virtqueue *);
  *	parallel with being added/removed.
  * @find_vqs: find virtqueues and instantiate them.
  *	vdev: the virtio_device
- *	nvqs: the number of virtqueues to find
- *	vqs: on success, includes new virtqueues
- *	callbacks: array of callbacks, for each virtqueue
- *		include a NULL entry for vqs that do not need a callback
- *	names: array of virtqueue names (mainly for debugging)
- *		include a NULL entry for vqs unused by driver
+ *	cfg: the config from the driver
  *	Returns 0 on success or error status
  * @del_vqs: free virtqueues found by find_vqs().
  * @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
@@ -96,6 +123,7 @@  typedef void vq_callback_t(struct virtqueue *);
  * @create_avq: create admin virtqueue resource.
  * @destroy_avq: destroy admin virtqueue resource.
  */
+
 struct virtio_config_ops {
 	void (*get)(struct virtio_device *vdev, unsigned offset,
 		    void *buf, unsigned len);
@@ -105,10 +133,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);
@@ -217,8 +242,14 @@  struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
 	vq_callback_t *callbacks[] = { c };
 	const char *names[] = { n };
 	struct virtqueue *vq;
-	int err = vdev->config->find_vqs(vdev, 1, &vq, callbacks, names, NULL,
-					 NULL);
+	struct virtio_vq_config cfg = {};
+
+	cfg.nvqs = 1;
+	cfg.vqs = &vq;
+	cfg.callbacks = callbacks;
+	cfg.names = names;
+
+	int err = vdev->config->find_vqs(vdev, &cfg);
 	if (err < 0)
 		return ERR_PTR(err);
 	return vq;
@@ -226,21 +257,37 @@  struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
 
 static inline
 int virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-			struct virtqueue *vqs[], vq_callback_t *callbacks[],
-			const char * const names[],
-			struct irq_affinity *desc)
+		    struct virtqueue *vqs[], vq_callback_t *callbacks[],
+		    const char * const names[],
+		    struct irq_affinity *desc)
 {
-	return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, NULL, desc);
+	struct virtio_vq_config cfg = {};
+
+	cfg.nvqs = nvqs;
+	cfg.vqs = vqs;
+	cfg.callbacks = callbacks;
+	cfg.names = (const char **)names;
+	cfg.desc = desc;
+
+	return vdev->config->find_vqs(vdev, &cfg);
 }
 
 static inline
 int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
 			struct virtqueue *vqs[], vq_callback_t *callbacks[],
-			const char * const names[], const bool *ctx,
+			const char *names[], const bool *ctx,
 			struct irq_affinity *desc)
 {
-	return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, ctx,
-				      desc);
+	struct virtio_vq_config cfg = {};
+
+	cfg.nvqs = nvqs;
+	cfg.vqs = vqs;
+	cfg.callbacks = callbacks;
+	cfg.names = names;
+	cfg.ctx = ctx;
+	cfg.desc = desc;
+
+	return vdev->config->find_vqs(vdev, &cfg);
 }
 
 /**