Parav Pandit
2023-Oct-30 18:10 UTC
[PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue
> From: Michael S. Tsirkin <mst at redhat.com> > Sent: Monday, October 30, 2023 9:29 PM > On Mon, Oct 30, 2023 at 03:51:40PM +0000, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin <mst at redhat.com> > > > Sent: Monday, October 30, 2023 1:53 AM > > > > > > On Sun, Oct 29, 2023 at 05:59:45PM +0200, Yishai Hadas wrote: > > > > From: Feng Liu <feliu at nvidia.com> > > > > > > > > Introduce support for the admin virtqueue. By negotiating > > > > VIRTIO_F_ADMIN_VQ feature, driver detects capability and creates > > > > one administration virtqueue. Administration virtqueue > > > > implementation in virtio pci generic layer, enables multiple types > > > > of upper layer drivers such as vfio, net, blk to utilize it. > > > > > > > > Signed-off-by: Feng Liu <feliu at nvidia.com> > > > > Reviewed-by: Parav Pandit <parav at nvidia.com> > > > > Reviewed-by: Jiri Pirko <jiri at nvidia.com> > > > > Signed-off-by: Yishai Hadas <yishaih at nvidia.com> > > > > --- > > > > drivers/virtio/virtio.c | 37 ++++++++++++++-- > > > > drivers/virtio/virtio_pci_common.c | 3 ++ > > > > drivers/virtio/virtio_pci_common.h | 15 ++++++- > > > > drivers/virtio/virtio_pci_modern.c | 61 +++++++++++++++++++++++++- > > > > drivers/virtio/virtio_pci_modern_dev.c | 18 ++++++++ > > > > include/linux/virtio_config.h | 4 ++ > > > > include/linux/virtio_pci_modern.h | 5 +++ > > > > 7 files changed, 137 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > > > index > > > > 3893dc29eb26..f4080692b351 100644 > > > > --- a/drivers/virtio/virtio.c > > > > +++ b/drivers/virtio/virtio.c > > > > @@ -302,9 +302,15 @@ static int virtio_dev_probe(struct device *_d) > > > > if (err) > > > > goto err; > > > > > > > > + if (dev->config->create_avq) { > > > > + err = dev->config->create_avq(dev); > > > > + if (err) > > > > + goto err; > > > > + } > > > > + > > > > err = drv->probe(dev); > > > > if (err) > > > > - goto err; > > > > + goto err_probe; > > > > > > > > /* If probe didn't do it, mark device DRIVER_OK ourselves. */ > > > > if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) > > > > > > Hmm I am not all that happy that we are just creating avq unconditionally. > > > Can't we do it on demand to avoid wasting resources if no one uses it? > > > > > Virtio queues must be enabled before driver_ok as we discussed in > F_DYNAMIC bit exercise. > > So creating AQ when first legacy command is invoked, would be too late. > > Well we didn't release the spec with AQ so I am pretty sure there are no devices > using the feature. Do we want to already make an exception for AQ and allow > creating AQs after DRIVER_OK even without F_DYNAMIC? >No. it would abuse the init time config registers for the dynamic things like this. For flow filters and others there is need for dynamic q creation with multiple physical address anyway. So creating virtqueues dynamically using a generic scheme is desired with new feature bit.
Michael S. Tsirkin
2023-Oct-30 23:31 UTC
[PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue
On Mon, Oct 30, 2023 at 06:10:06PM +0000, Parav Pandit wrote:> > > > From: Michael S. Tsirkin <mst at redhat.com> > > Sent: Monday, October 30, 2023 9:29 PM > > On Mon, Oct 30, 2023 at 03:51:40PM +0000, Parav Pandit wrote: > > > > > > > > > > From: Michael S. Tsirkin <mst at redhat.com> > > > > Sent: Monday, October 30, 2023 1:53 AM > > > > > > > > On Sun, Oct 29, 2023 at 05:59:45PM +0200, Yishai Hadas wrote: > > > > > From: Feng Liu <feliu at nvidia.com> > > > > > > > > > > Introduce support for the admin virtqueue. By negotiating > > > > > VIRTIO_F_ADMIN_VQ feature, driver detects capability and creates > > > > > one administration virtqueue. Administration virtqueue > > > > > implementation in virtio pci generic layer, enables multiple types > > > > > of upper layer drivers such as vfio, net, blk to utilize it. > > > > > > > > > > Signed-off-by: Feng Liu <feliu at nvidia.com> > > > > > Reviewed-by: Parav Pandit <parav at nvidia.com> > > > > > Reviewed-by: Jiri Pirko <jiri at nvidia.com> > > > > > Signed-off-by: Yishai Hadas <yishaih at nvidia.com> > > > > > --- > > > > > drivers/virtio/virtio.c | 37 ++++++++++++++-- > > > > > drivers/virtio/virtio_pci_common.c | 3 ++ > > > > > drivers/virtio/virtio_pci_common.h | 15 ++++++- > > > > > drivers/virtio/virtio_pci_modern.c | 61 +++++++++++++++++++++++++- > > > > > drivers/virtio/virtio_pci_modern_dev.c | 18 ++++++++ > > > > > include/linux/virtio_config.h | 4 ++ > > > > > include/linux/virtio_pci_modern.h | 5 +++ > > > > > 7 files changed, 137 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > > > > index > > > > > 3893dc29eb26..f4080692b351 100644 > > > > > --- a/drivers/virtio/virtio.c > > > > > +++ b/drivers/virtio/virtio.c > > > > > @@ -302,9 +302,15 @@ static int virtio_dev_probe(struct device *_d) > > > > > if (err) > > > > > goto err; > > > > > > > > > > + if (dev->config->create_avq) { > > > > > + err = dev->config->create_avq(dev); > > > > > + if (err) > > > > > + goto err; > > > > > + } > > > > > + > > > > > err = drv->probe(dev); > > > > > if (err) > > > > > - goto err; > > > > > + goto err_probe; > > > > > > > > > > /* If probe didn't do it, mark device DRIVER_OK ourselves. */ > > > > > if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) > > > > > > > > Hmm I am not all that happy that we are just creating avq unconditionally. > > > > Can't we do it on demand to avoid wasting resources if no one uses it? > > > > > > > Virtio queues must be enabled before driver_ok as we discussed in > > F_DYNAMIC bit exercise. > > > So creating AQ when first legacy command is invoked, would be too late. > > > > Well we didn't release the spec with AQ so I am pretty sure there are no devices > > using the feature. Do we want to already make an exception for AQ and allow > > creating AQs after DRIVER_OK even without F_DYNAMIC? > > > No. it would abuse the init time config registers for the dynamic things like this. > For flow filters and others there is need for dynamic q creation with multiple physical address anyway.That seems like a completely unrelated issue.> So creating virtqueues dynamically using a generic scheme is desired with new feature bit.