Re: [PATCH v12 05/10] pci: add admin vq registers to virtio over pci

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 27/04/2023 20:57, Michael S. Tsirkin wrote:
> On Thu, Apr 27, 2023 at 01:31:29AM +0300, Max Gurtovoy wrote:
> > On 27/04/2023 1:11, Parav Pandit wrote:
> > > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > Sent: Wednesday, April 26, 2023 5:12 PM
> > > >
> > > > On 25/04/2023 1:14, Parav Pandit wrote:
> > > > > On 4/24/2023 6:06 PM, Max Gurtovoy wrote:
> > > > >
> > > > > > > +If VIRTIO_F_ADMIN_VQ has been negotiated, and if the driver
> > > > > > > +configures any administration virtqueues, the driver MUST configure
> > > > > > > +the administration virtqueues using the index in the range
> > > > > > > +\field{admin_queue_index} to \field{admin_queue_index} +
> > > > > > > +\field{admin_queue_num} - 1 inclusive.
> > > > > > > +The driver MAY configure less administration virtqueues than
> > > > > > > +supported by the device.
> > > > > >
> > > > > > we need to say something about other virtq's indexes.
> > > > > > For example, if aq index = 0 then for the vblk device the request
> > > > > > queues would start from index = 1.
> > > > > >
> > > > > > can we address this ?
> > > > >
> > > > > It may slightly complicate the device implementation when device
> > > > > dynamically changes the VQ indices based on the negotiation of the
> > > > > feature bit.
> > > > > Also the drivers with and without need a different VQ index handling.
> > > > >
> > > > > I am thinking that aq index should not overlap with the vq index in
> > > > > range exposed by num_queues field.
> > > > >
> > > > > This also aligns to the exclusion of aq count from num_queues field.
> > > >
> > > > The namespace of the aq and other vq index is the same.
> > > > And the configuration is done using the same queue_select and other registers.
> > > > Thus, we need to address the above comment otherwise all the device
> > > > virtqueues chapters are wrong.
> > >
> > > Michael has added below line in this patch in num_queues description so it covers the exclusion part.
> > >
> > > +        This excludes administration virtqueues if any are supported.
> >
> > This is not related to what I was talking about.
> >
> > For example if vnet device:
> >
> > 0 receiveq1
> > 1 transmitq1
> > ...
> > 2(N-1) receiveqN
> > 2(N-1)+1 transmitqN
> > 2N controlq
> > N=1 if neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are negotiated,
> > otherwise N is set by max_-
> > virtqueue_pairs.
> > controlq only exists if VIRTIO_NET_F_CTRL_VQ set
> >
> > and in vfs device:
> >
> > 0 hiprio
> > 1...n request queues
> >
> > So if one device will expose adminq that will be with index 0 the above is
> > wrong description.
> >
> > Agree that this should be addressed ?
>
> It's addressed by patch 9:
>
>
> +If VIRTIO_F_ADMIN_VQ has been negotiated, the value
> +\field{admin_queue_index} MUST be equal to, or bigger than
> +\field{num_queues}; also, \field{admin_queue_num} MUST be
> +smaller than, or equal to 0x10000 - \field{admin_queue_index},
> +to ensure that indices of valid admin queues fit into
> +a 16 bit range beyond all other virtqueues.
>
>
> Thus adminq can not have index 0.

Thanks.
Maybe we can mentioned it in this patch as well (in the configuration 
layout section) but this is up to you.

Looks good,
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>