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

[Parav Pandit <parav@nvidia.com>]

> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> Sent: Wednesday, April 26, 2023 6:31 PM
> >>>
> >>>>> +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 ?

Oh yes, I agree.
In response [1] I acked it as,

"I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field."

So, admin_queue_index description needs to have description like below.

admin_queue_index must be greater than or equal to the value of num_queues.

Yes?

[1] a4b1596c-2132-714a-7557-fa5249a55945@nvidia.com/">https://lore.kernel.org/virtio-dev/a4b1596c-2132-714a-7557-fa5249a55945@nvidia.com/