Re: [virtio-dev] Re: [PATCH v2 1/4] Add virtio Admin Virtqueue

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 31, 2022 at 04:47:29PM +0100, Halil Pasic wrote:
> On Mon, 31 Jan 2022 10:16:43 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Sun, Jan 30 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Sun, Jan 30, 2022 at 05:12:46PM +0200, Max Gurtovoy wrote:  
> > >> 
> > >> On 1/30/2022 4:41 PM, Michael S. Tsirkin wrote:  
> > >> > On Sun, Jan 30, 2022 at 11:56:30AM +0200, Max Gurtovoy wrote:  
> > >> > > On 1/30/2022 11:40 AM, Michael S. Tsirkin wrote:  
> > >> > > > On Sun, Jan 30, 2022 at 11:13:38AM +0200, Max Gurtovoy wrote:  
> > >> > > > > On 1/29/2022 5:53 AM, Jason Wang wrote:  
> > >> > > > > > On Fri, Jan 28, 2022 at 11:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:  
> > >> > > > > > > On Fri, Jan 28, 2022 at 04:49:34PM +0100, Cornelia Huck wrote:  
> > >> > > > > > > > On Fri, Jan 28 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >> > > > > > > >   
> > >> > > > > > > > > On Fri, Jan 28, 2022 at 01:14:14PM +0100, Cornelia Huck wrote:  
> > >> > > > > > > > > > On Mon, Jan 24 2022, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:  
> > >> > > > > > > > > > > +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
> > >> > > > > > > > > > > +
> > >> > > > > > > > > > > +Admin virtqueue is used to send administrative commands to manipulate
> > >> > > > > > > > > > > +various features of the device and/or to manipulate various features,
> > >> > > > > > > > > > > +if possible, of another device within the same group (e.g. PCI VFs of
> > >> > > > > > > > > > > +a parent PCI PF device are grouped together. These devices can be
> > >> > > > > > > > > > > +optionally managed by its parent PCI PF using its admin virtqueue.).
> > >> > > > > > > > > > > +
> > >> > > > > > > > > > > +Use of Admin virtqueue is negotiated by the VIRTIO_F_ADMIN_VQ
> > >> > > > > > > > > > > +feature bit.
> > >> > > > > > > > > > > +
> > >> > > > > > > > > > > +Admin virtqueue index may vary among different device types.  
> > >> > > > > > > > > > So, my understanding is:
> > >> > > > > > > > > > - any device type may or may not support the admin vq
> > >> > > > > > > > > > - if the device type wants to be able to accommodate the admin vq, it
> > >> > > > > > > > > >      also needs to specify where it shows up when the feature is negotiated
> > >> > > > > > > > > > 
> > >> > > > > > > > > > Do we expect that eventually all device types will need to support the
> > >> > > > > > > > > > admin vq (if some use case comes along that will require all devices to
> > >> > > > > > > > > > participate, for example?)  
> > >> > > > > > > > > I suspect yes. And that's one of the reasons why I'd rather we had a
> > >> > > > > > > > > device independent way to locate the admin queue. There are less
> > >> > > > > > > > > transports than device types.  
> > >> > > > > > > > So, do we want to bite the bullet now and simply say that every device
> > >> > > > > > > > type has the admin vq as the last vq if the feature is negotiated?
> > >> > > > > > > > Should be straightforward for the device types that have a fixed number
> > >> > > > > > > > of vqs, and doable for those that have a variable amount (two device
> > >> > > > > > > > types are covered by this series anyway.) I think we need to put it with
> > >> > > > > > > > the device types, as otherwise the numbering of virtqueues could change
> > >> > > > > > > > in unpredictable ways with the admin vq off/on.  
> > >> > > > > > > Well that only works once. The next thing we'll need we won't be able to
> > >> > > > > > > make the last one ;) So I am inclined to add a per-transport field that
> > >> > > > > > > gives the admin queue number.  
> > >> > > > > > Technically, there's no need to use the same namespace for admin
> > >> > > > > > virtqueue if it has a dedicated notification area. If we go this way,
> > >> > > > > > we can simply use 0 as queue index for admin virtqueue.  
> > >> > > > > Or we can use index 0xFFFF for admin virtqueue for compatibility.  
> > >> > > > I think I'd prefer a register with the #. For example we might want
> > >> > > > to limit the # of VQs in order to pass extra data with the kick write.  
> > >> > > So you are suggesting adding a new cfg_type (#define
> > >> > > VIRTIO_PCI_CAP_ADMIN_CFG 10) ?
> > >> > > 
> > >> > > that will look something like:
> > >> > > 
> > >> > > struct virtio_pci_admin_cfg {
> > >> > > 
> > >> > >      le32 queue_index; /* read only for the driver */
> > >> > > 
> > >> > >      le16 queue_size; /* read-write */
> > >> > >      le16 queue_msix_vector; /* read-write */
> > >> > >      le16 queue_enable; /* read-write */
> > >> > >      le16 queue_notify_off; /* read-only for driver */
> > >> > >      le64 queue_desc; /* read-write */
> > >> > >      le64 queue_driver; /* read-write */
> > >> > >      le64 queue_device; /* read-write */
> > >> > >      le16 queue_notify_data; /* read-only for driver */
> > >> > >      le16 queue_reset; /* read-write */
> > >> > > 
> > >> > > };
> > >> > > 
> > >> > > instead of re-using the struct virtio_pci_common_cfg ?
> > >> > > 
> > >> > > 
> > >> > > or do you prefer extending the struct virtio_pci_common_cfg with "le16
> > >> > > admin_queue_index; /* read only for the driver */ ?  
> > >> > The later. Other transports will need this too.
> > >> > 
> > >> > 
> > >> > Cornelia has another idea which is that instead of
> > >> > adding just the admin queue register to all transports,
> > >> > we instead add a misc_config structure to all
> > >> > transports. Working basically like device specific config,
> > >> > but being device independent. For now it will only have
> > >> > a single le16 admin_queue_index register.
> > >> > 
> > >> > For PCI we would thus add it with VIRTIO_PCI_CAP_MISC_CFG
> > >> > 
> > >> > The point here is that we are making it easier to add
> > >> > more fields just like admin queue index in the future.  
> > >> 
> > >> OK.
> > >> 
> > >> #define VIRTIO_PCI_CAP_MISC_CFG 10
> > >> 
> > >> and
> > >> 
> > >> struct virtio_pci_misc_cfg {
> > >>     le16 admin_queue_index; /* read-only for driver */
> > >> };
> > >> 
> > >> Is agreed by all for V3 ? instead of the net and blk AQ index definitions.  
> > >
> > > We need to add it to MMIO and CCW I guess too.  
> > 
> > That seems ok for pci.
> > 
> > For ccw, I'd do something like
> > 
> > #define CCW_CMD_READ_MISC_CONF 0x82
> > 
> > struct virtio_misc_conf {
> >        be16 admin_queue_index;
> > };
> > 
> > bound to revision 3, which gets a payload data containing the length of
> > this structure (for future expansions).
> > 
> > Halil, do you think that would work?
> 
> 
> I think so. But I would like to review the actual proposal :)
> 
> Some of the questions I have in mind are:
> * Are fields in this config protected with feature bits
> * Do we ever need to write this config? Adding a writable field to pci
>   is easier than to ccw (no need to invent a new ccw)
> * Looks like we will have an addressability mismatch with
>   ccw compared to pci and mmio: ccw can only read a prefix of this new
>   config, while mmio and pci have access to the individual fields
> * Is this static stuff only? If not, do we need notifications for config
>   change like for the device specific config?

I'd just reuse the generic config change if we ever add
something non-static.

Donnu about other questions.

> > 
> > For mmio, I'd need to think a bit more. Any mmio experts around?
> > 
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >