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

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

On Wed, Apr 26, 2023 at 10:11:53PM +0000, 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.
> 
> I inspected all the devices.
> Following devices which has multi queue supports are fine, net, console, scsi host, gpu, input, crypto, socket, rpmb, iommu, sound, scmi, gpio. 
> 
> Following single q devices are fine too: entropy, mem ballon dev, mem, i2c, pmem.
> So mostly all chapters donât look wrong.
> 
> Block device needs below change.
> WDYT?
> 
> From 0592d167451280bc212df7322077f4c94b28c917 Mon Sep 17 00:00:00 2001
> From: Parav Pandit <parav@nvidia.com>
> Date: Thu, 27 Apr 2023 01:01:41 +0300
> Subject: [PATCH] virtio-blk: Rename num_queues to num_req_queues
> 
> num_queues field represents number of blk specific request queues.
> 
> Renaming it to num_req_queues reflect its precise usage.
> It also avoids confusion with PCI transport's generic num_queues field.

There's no confusion actually - both fields are exactly the same
are they not? The one in blk just predates the one in pci.

> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
>  device-types/blk/description.tex | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
> index f04c932..5a27399 100644
> --- a/device-types/blk/description.tex
> +++ b/device-types/blk/description.tex
> @@ -16,7 +16,7 @@ \subsection{Virtqueues}\label{sec:Device Types / Block Device / Virtqueues}
>  \end{description}
> 
>   N=1 if VIRTIO_BLK_F_MQ is not negotiated, otherwise N is set by
> - \field{num_queues}.
> + \field{num_req_queues}.
> 
>  \subsection{Feature bits}\label{sec:Device Types / Block Device / Feature bits}
> 
> @@ -108,7 +108,7 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>          } topology;
>          u8 writeback;
>          u8 unused0;
> -        u16 num_queues;
> +        u16 num_req_queues;
>          le32 max_discard_sectors;
>          le32 max_discard_seg;
>          le32 discard_sector_alignment;
> @@ -135,8 +135,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>  present. The availability of the others all depend on various feature
>  bits as indicated above.
> 
> -The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
> -the number of queues.
> +The field \field{num_req_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
> +the number of request queues.
> 
>  The parameters in the configuration space of the device \field{max_discard_sectors}
>  \field{discard_sector_alignment} are expressed in 512-byte units if the
> --
> 2.26.2