Re: [PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Parav Pandit <parav@nvidia.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Maor Gottlieb <maorg@nvidia.com>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	Jason Gunthorpe <jgg@nvidia.com>, Jiri Pirko <jiri@nvidia.com>,
	Leon Romanovsky <leonro@nvidia.com>
Subject: Re: [PATCH V2 vfio 2/9] virtio-pci: Introduce admin virtqueue
Date: Tue, 31 Oct 2023 03:59:04 -0400	[thread overview]
Message-ID: <20231031034833-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <PH0PR12MB5481F2851BF40C5BBD59909CDCA0A@PH0PR12MB5481.namprd12.prod.outlook.com>

On Tue, Oct 31, 2023 at 03:11:57AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, October 31, 2023 5:02 AM
> > 
> > On Mon, Oct 30, 2023 at 06:10:06PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@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@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@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@nvidia.com>
> > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > > > > > > Signed-off-by: Yishai Hadas <yishaih@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.
> > 
> It isn't.
> Driver requirements are:
> 1. Driver needs to dynamically create vqs
> 2. Sometimes this VQ needs to have multiple physical addresses
> 3. Driver needs to create them after driver is fully running, past the bootstrap stage using tiny config registers
> 
> Device requirements are:
> 1. Not to keep growing 64K VQs *(8+8+8) bytes of address registers + enable bit
> 2. Ability to return appropriate error code when fail to create queue
> 3. Above #2
> 
> Users of this new infrastructure are eth tx,rx queues, flow filter queues, aq, blk rq per cpu.
> AQs are just one of those.
> When a generic infrastructure for this will be built in the spec as we started that, all above use cases will be handled.
> 
> > > So creating virtqueues dynamically using a generic scheme is desired with
> > new feature bit.

Reducing config registers and returning errors should be handled by
a new transport.
VQ with multiple addresses - I can see how you would maybe only
support that with that new transport?

I think I can guess why you are tying multiple addresses to dynamic VQs -
you suspect that allocating huge half-megabyte VQs dynamically will fail if
triggered on a busy system. Is that the point?

In that case I feel it's a good argument to special-case
admin VQs because there's no real need to make them
huge at the moment - for example this driver just adds one at a time.
No?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization