On Wed, 20 Mar 2024 17:22:50 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Tue, Mar 19, 2024 at 2:58 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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 <jasowang@xxxxxxxxxx> wrote:
> > > > On Fri, Mar 15, 2024 at 3:26 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 15 Mar 2024 11:51:48 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > On Thu, Mar 14, 2024 at 2:00 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Thu, 14 Mar 2024 11:12:24 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > > > On Tue, Mar 12, 2024 at 10:10 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > Now, we pass multi parameters to find_vqs. These parameters
> > > > > > > > > may work for transport or work for vring.
> > > > > > > > >
> > > > > > > > > And find_vqs has multi implements in many places:
> > > > > > > > >
> > > > > > > > > arch/um/drivers/virtio_uml.c
> > > > > > > > > drivers/platform/mellanox/mlxbf-tmfifo.c
> > > > > > > > > drivers/remoteproc/remoteproc_virtio.c
> > > > > > > > > drivers/s390/virtio/virtio_ccw.c
> > > > > > > > > drivers/virtio/virtio_mmio.c
> > > > > > > > > drivers/virtio/virtio_pci_legacy.c
> > > > > > > > > drivers/virtio/virtio_pci_modern.c
> > > > > > > > > drivers/virtio/virtio_vdpa.c
> > > > > > > > >
> > > > > > > > > Every time, we try to add a new parameter, that is difficult.
> > > > > > > > > We must change every find_vqs implement.
> > > > > > > > >
> > > > > > > > > One the other side, if we want to pass a parameter to vring,
> > > > > > > > > we must change the call path from transport to vring.
> > > > > > > > > Too many functions need to be changed.
> > > > > > > > >
> > > > > > > > > So it is time to refactor the find_vqs. We pass a structure
> > > > > > > > > cfg to find_vqs(), that will be passed to vring by transport.
> > > > > > > > >
> > > > > > > > > Because the vp_modern_create_avq() use the "const char *names[]",
> > > > > > > > > and the virtio_uml.c changes the name in the subsequent commit, so
> > > > > > > > > change the "names" inside the virtio_vq_config from "const char *const
> > > > > > > > > *names" to "const char **names".
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > > > > > > Acked-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> > > > > > > > > Reviewed-by: Ilpo J=E4rvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > > > > > >
> > > > > > > > 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 <mst@xxxxxxxxxx>
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 <mst@xxxxxxxxxx>
Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
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
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> >
>
