On Wed, Mar 20, 2024 at 5:41 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote: > > 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. 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >