[virtio-dev] [PATCH v14 00/11] Rename queue number to queue index

[virtio-dev] [PATCH v14 00/11] Rename queue number to queue index

[Parav Pandit]

1. Currently, virtqueue is identified between driver and device
interchangeably using either number or index terminology.

2. Between PCI and MMIO transport the queue size (depth) is
defined as queue_size and QueueNum respectively.

To avoid confusion and to have consistency, unify them to use
index.

3. Field name vqn in the driver notification structure is
ambiguous as it is supposed to hold either vq index or device
supplied vq config data.

4. Device is really supplying queue identifier or a opaque data in the 
queue_notify_data register, and this often get confused with
very similar looking feature bit NOTIFICATION_DATA.

Solution:
a. Use virtqueue index description, and rename MMIO register as QueueSize.
b. Replace virtqueue number with virtqueue index
c. RSS area of virtio net has inherited some logic, describe it
using abstract rss_rq_id.
d. rename queue_notifify_data to queue_notify_config_data.
e. rename vqn to vq_notify_config_data to reflect it can hold either vq
index of device supplied some id.

Patch summary:
patch-1 introduce vq number as generic term
patch-2 renames index to number for pci transport
patch-3 rename queue_notify_data to queue_notify_config_data
patch-4 remove first vq index reference
patch-5 renames mmio register from Num to Size
patch-6 renames index to number for mmio transport
patch-7 renames num field to size for ccw transport
patch-8 renames vq by its index for ccw transport
patch-9 for virtio-net removes duplicate example from requirements
patch-10 for virtio-net updates rss description to use vq index
patch-11 for virtio-net to update cvq notifications coalescing commands

This series only improves the documentation, it does not change any
transport or device functionality.

Please review.
This series fixes the issue [1].

[1] https://github.com/oasis-tcs/virtio-spec/issues/163

---
changelog:
v13->v14:
- added Halil's RB tag
- added pci transport specific union structure
- added normative lines for case when VIRTIO_F_NOTIF_CONFIG_DATA
  is not negotiated.
- added normataive lines for clarify bit-width for driver notification
- replace left over _id with _config_data
- use _notif_config_data name to align to feature name
v12->v13:
- renamed queue_notify_id to queue_notify_config_data
- added patch to cover notifications coalescing commands after rebase
- fixed left out virtqueue number to virtqueue index
- dropped abbreviation of virtqueue to vq
v11->v12:
- replace number to index
- avoid confusion around vqn field and rename to vq_notify_id
- rename queue_notify_data to avoid confusing with NOTIFY_DATA
v10->v11:
- added Reviewed-by for all the reviewed patches
- updated commit log of patch-8 to drop rq_handle reference
- skipped comment to further use rss_rq_id, as rss_rq_id usage
  and structure are self describing
v9->v10:
- added virtqueue number part in content in braces
- replaced queue_select to vqn in ccw
- avoided aggrasive alignment of 65 chars
- updated commit log to drop reference to already merged patches
- added review-by tag for already reviewed patches
v8->v9:
- addressed comments from David
- few corrections with article
- renaming 'virtqueue number' to 'vq number'
- improving text and wording for rss_rq_id, avail notification
- commit log of specific change in individual patches
v7->v8:
- remove note about first virtqueue number
- skipped Max's comment to put word 'structure' in same line as its
  crosses 65 chars limit per line
- reworded queue_notification data set line, as '=' and vq number
  wording was odd
v6->v7:
- remove text around first vq as it is already covered in the basic
  virtqueues facility section
v5->v6:
- moved the vq number description from middle of vq operation
  to beginning of vq introduction
v4->v5:
- fixed accidental removal of "unclassifed packets".
- simplfied text around indirection_table mask
- removed rss_rq_id references as indirection table and
  unclassified_queue data type is self explanatory
v3->v4:
- moved note to comment for ccw
- renamed rq_handle to rss_rq_id
- moved rss_rq_id next to rss_config structure
- define rss_config structure using rss_rq_id
v2->v3:
- addressed comments from Michael
- added previous definitions for ccw fields
- moved rq_handle definition before using it
- added first patch to describe vq number
- updated pci for available buffer notification section
v1->v2:
- added patches for virtio net for rss area
- added patches for covering ccw transport
- added missing entries to refer in mmio transport


Parav Pandit (11):
  content: Add vq index text
  content.tex Replace virtqueue number with index
  content: Rename confusing queue_notify_data and vqn names
  transport-pci: Avoid first vq index reference
  transport-mmio: Rename QueueNum register
  transport-mmio: Avoid referring to zero based index
  transport-ccw: Rename queue depth/size to other transports
  transport-ccw: Refer to the vq by its index
  virtio-net: Avoid duplicate receive queue example
  virtio-net: Describe RSS using rss rq id
  virtio-net: Update vqn to vq_index for cvq cmds

 content.tex                      | 20 ++++++---
 device-types/net/description.tex | 53 ++++++++++++++---------
 notifications-be.c               |  2 +-
 notifications-le.c               |  2 +-
 notifications-pci-le.c           |  8 ++++
 transport-ccw.tex                | 15 ++++---
 transport-mmio.tex               | 55 ++++++++++++++----------
 transport-pci.tex                | 74 +++++++++++++++++++++++---------
 8 files changed, 151 insertions(+), 78 deletions(-)
 create mode 100644 notifications-pci-le.c

-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 01/11] content: Add vq index text

[Parav Pandit]

Introduce vq index and its range so that subsequent patches can refer
to it.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v12->v13:
- avoid virtqueue -> vq abbreviation
- removed Cornelia's reviewed-by due to vq abbreviation change
v11->v12:
- renamed 'number' to 'index'
v9->v10:
- added braces around vq number wording
- added vqn as another term for vq number
v8->v9:
- added inclusive when describing the vq number range
- skipped comment to put virtqueue number wording first because we
  prefer to use shorter vq number as much as possible
v5->v6:
- moved description close to introduction, it was in middle of
  queue data transfer description
v2->v3:
- new patch
---
 content.tex | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/content.tex b/content.tex
index cff548a..43be58b 100644
--- a/content.tex
+++ b/content.tex
@@ -298,6 +298,9 @@ \section{Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues}
 virtqueues\footnote{For example, the simplest network device has one virtqueue for
 transmit and one for receive.}.
 
+Each virtqueue is identified by a virtqueue index; virtqueue index range is
+from 0 to 65535 inclusive.
+
 Driver makes requests available to device by adding
 an available buffer to the queue, i.e., adding a buffer
 describing the request to a virtqueue, and optionally triggering
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 02/11] content.tex Replace virtqueue number with index

[Parav Pandit]

Replace virtqueue number with index to align to rest of the
specification.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v11->v12:
- new patch
---
 content.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/content.tex b/content.tex
index 43be58b..e79d722 100644
--- a/content.tex
+++ b/content.tex
@@ -405,7 +405,7 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
 this notification involves sending the
-virtqueue number to the device (method depending on the transport).
+virtqueue index to the device (method depending on the transport).
 
 However, some devices benefit from the ability to find out the
 amount of available data in the queue without accessing the virtqueue in memory:
@@ -789,13 +789,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   buffer notifications.
   As mentioned in section \ref{sec:Basic Facilities of a Virtio Device / Driver notifications}, when the
   driver is required to send an available buffer notification to the device, it
-  sends the virtqueue number to be notified. The method of delivering
+  sends the virtqueue index to be notified. The method of delivering
   notifications is transport specific.
   With the PCI transport, the device can optionally provide a per-virtqueue value
-  for the driver to use in driver notifications, instead of the virtqueue number.
+  for the driver to use in driver notifications, instead of the virtqueue index.
   Some devices may benefit from this flexibility by providing, for example,
   an internal virtqueue identifier, or an internal offset related to the
-  virtqueue number.
+  virtqueue index.
 
   This feature indicates the availability of such value. The definition of the
   data to be provided in driver notification and the delivery method is
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 03/11] content: Rename confusing queue_notify_data and vqn names

[Parav Pandit]

Currently queue_notify_data register indicates the device
internal queue notification content. This register is
meaningful only when feature bit VIRTIO_F_NOTIF_CONFIG_DATA is
negotiated.

However, above register name often get confusing association with
very similar feature bit VIRTIO_F_NOTIFICATION_DATA.

When VIRTIO_F_NOTIFICATION_DATA feature bit is negotiated,
notification really involves sending additional queue progress
related information (not queue identifier or index).

Hence
1. to avoid any misunderstanding and association of
queue_notify_data with similar name VIRTIO_F_NOTIFICATION_DATA,

and
2. to reflect that queue_notify_data is the actual device
internal virtqueue identifier/index/data/cookie,

a. rename queue_notify_data to queue_notif_config_data.

b. rename ambiguous vqn to a union of vq_index and vq_config_data

c. The driver notification section assumes that queue notification contains
vq index always. CONFIG_DATA feature bit introduction missed to
update the driver notification section. Hence, correct it.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v13->v14:
- added pci transport specific union structure
- added normative lines for case when VIRTIO_F_NOTIF_CONFIG_DATA
  is not negotiated.
- added normataive lines for clarify bit-width for driver notification
- replace left over _id with _config_data
- use _notif_config_data name to align to feature name
v12->v13:
- replace _id with _config_data
- dropped vq identifier
- dropped the idea of union as description is for config data feature
v11->v12:
- new patch
---
 content.tex            | 11 +++++--
 notifications-be.c     |  2 +-
 notifications-le.c     |  2 +-
 notifications-pci-le.c |  8 +++++
 transport-pci.tex      | 72 +++++++++++++++++++++++++++++++-----------
 5 files changed, 71 insertions(+), 24 deletions(-)
 create mode 100644 notifications-pci-le.c

diff --git a/content.tex b/content.tex
index e79d722..e1345ea 100644
--- a/content.tex
+++ b/content.tex
@@ -404,8 +404,12 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
 notification to the device.
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-this notification involves sending the
-virtqueue index to the device (method depending on the transport).
+this notification contains either virtqueue index if VIRTIO_F_NOTIF_CONFIG_DATA
+is not negotiated or device supplied virtqueue notification config data if
+VIRTIO_F_NOTIF_CONFIG_DATA is negotiated.
+
+A notification method and suppling such virtqueue notification config data is
+transport specific.
 
 However, some devices benefit from the ability to find out the
 amount of available data in the queue without accessing the virtqueue in memory:
@@ -416,7 +420,8 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
 the following information:
 
 \begin{description}
-\item [vqn] VQ number to be notified.
+\item [vq_index_config_data] Either virtqueue index or device supplied
+      queue notification config data corresponding to a virtqueue.
 \item [next_off] Offset
       within the ring where the next available ring entry
       will be written.
diff --git a/notifications-be.c b/notifications-be.c
index 5be947e..bf6d1cd 100644
--- a/notifications-be.c
+++ b/notifications-be.c
@@ -1,5 +1,5 @@
 be32 {
-	vqn : 16;
+	vq_index: 16; /* previously known as vqn */
 	next_off : 15;
 	next_wrap : 1;
 };
diff --git a/notifications-le.c b/notifications-le.c
index fe51267..8a19389 100644
--- a/notifications-le.c
+++ b/notifications-le.c
@@ -1,5 +1,5 @@
 le32 {
-	vqn : 16;
+	vq_index: 16; /* previously known as vqn */
 	next_off : 15;
 	next_wrap : 1;
 };
diff --git a/notifications-pci-le.c b/notifications-pci-le.c
new file mode 100644
index 0000000..ef60d15
--- /dev/null
+++ b/notifications-pci-le.c
@@ -0,0 +1,8 @@
+le32 {
+	union {
+		vq_index: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not negotiated */
+		vq_notif_config_data: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA negotiated */ 
+	};
+	next_off : 15;
+	next_wrap : 1;
+};
diff --git a/transport-pci.tex b/transport-pci.tex
index 5d98467..04a9a80 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -319,7 +319,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         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_notif_config_data;   /* read-only for driver */
         le16 queue_reset;               /* read-write */
 };
 \end{lstlisting}
@@ -388,17 +388,21 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 \item[\field{queue_device}]
         The driver writes the physical address of Device Area here.  See section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
 
-\item[\field{queue_notify_data}]
+\item[\field{queue_notif_config_data}]
         This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated.
-        The driver will use this value to put it in the 'virtqueue number' field
-        in the available buffer notification structure.
+        The driver will use this value when driver sends available buffer
+        notification to the device.
         See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
         \begin{note}
         This field provides the device with flexibility to determine how virtqueues
         will be referred to in available buffer notifications.
-        In a trivial case the device can set \field{queue_notify_data}=vqn. Some devices
-        may benefit from providing another value, for example an internal virtqueue
-        identifier, or an internal offset related to the virtqueue number.
+        In a trivial case the device can set \field{queue_notif_config_data} to
+        virtqueue index. Some devices may benefit from providing another value,
+        for example an internal virtqueue identifier, or an internal offset
+        related to the virtqueue index.
+        \end{note}
+        \begin{note}
+        This field is previously known as queue_notify_data.
         \end{note}
 
 \item[\field{queue_reset}]
@@ -468,7 +472,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 
 \drivernormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
 
-The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
+The driver MUST NOT write to \field{device_feature}, \field{num_queues},
+\field{config_generation}, \field{queue_notify_off} or
+\field{queue_notif_config_data}.
 
 If VIRTIO_F_RING_PACKED has been negotiated,
 the driver MUST NOT write the value 0 to \field{queue_size}.
@@ -1035,13 +1041,22 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
 the driver sends an available buffer notification to the device by writing
-the 16-bit virtqueue index
-of this virtqueue to the Queue Notify address.
+only the 16-bit notification value to the Queue Notify address of the
+virtqueue. A notification value depends on the negotiation of
+VIRTIO_F_NOTIF_CONFIG_DATA.
 
-When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
-the driver sends an available buffer notification to the device by writing
-the following 32-bit value to the Queue Notify address:
-\lstinputlisting{notifications-le.c}
+\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver sends an
+available buffer notification to the device by writing the following 32-bit
+value to the Queue Notify address:
+\lstinputlisting{notifications-pci-le.c}
+
+\begin{itemize}
+\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated \field{vq_index} is set
+to the virtqueue index.
+
+\item When VIRTIO_F_NOTIFICATION_DATA is negotiated,
+\field{vq_notif_config_data} is set to \field{queue_notif_config_data}.
+\end{itemize}
 
 See \ref{sec:Basic Facilities of a Virtio Device / Driver notifications}~\nameref{sec:Basic Facilities of a Virtio Device / Driver notifications}
 for the definition of the components.
@@ -1050,12 +1065,31 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 for how to calculate the Queue Notify address.
 
 \drivernormative{\paragraph}{Available Buffer Notifications}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}
-If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
+
+If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver notification 
+MUST be a 16-bit notification.
+
+If VIRTIO_F_NOTIFICATION_DATA is negotiated, the driver notification 
+MUST be a 32-bit notification.
+
+If VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated:
+\begin{itemize}
+\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set the
+notification value to virtqueue index.
+
+\item If VIRTIO_F_NOTIFICATION_DATA is negotiated, the driver MUST set the
+\field{vq_index} to the virtqueue index.
+
+\end{itemize}
+
+If VIRTIO_F_NOTIF_CONFIG_DATA is negotiated:
 \begin{itemize}
-\item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver MUST use the
-\field{queue_notify_data} value instead of the virtqueue index.
-\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver MUST set the
-\field{vqn} field to the \field{queue_notify_data} value.
+\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set
+the notification value to \field{queue_notif_config_data}.
+
+\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set the
+\field{vq_notify_config_data} to the \field{queue_notif_config_data}
+value.
 \end{itemize}
 
 \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Parav Pandit]

Drop reference to first virtqueue as it is already
covered now by the generic section in first patch.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v11->v12:
- drop changes related to vq number
v9->v10:
- updated commit log to drop reference to old patch
v8->v9:
- reword the sentence to avoid future tense, like rest of the other
  fields description
- reword the sentence to avoid multiple verbs use and put -> uses
- use shorter name 'vq number' instead of 'virtqueue number'
v7->v8:
- remove note about first virtqueue number
- skipped Max's comment to put word 'structure' in same line as its
  crosses 65 chars limit per line
- reworded queue_notification data set line, as '=' and vq number
  wording was odd
v2->v3:
- addressed comments from Michael
- changed vqn to virtqueue number in the Note
- refer to vqn field instead of virtqueue number
---
 transport-pci.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-pci.tex b/transport-pci.tex
index 04a9a80..1507c69 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -1011,7 +1011,7 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 The driver typically does this as follows, for each virtqueue a device has:
 
 \begin{enumerate}
-\item Write the virtqueue index (first queue is 0) to \field{queue_select}.
+\item Write the virtqueue index to \field{queue_select}.
 
 \item Read the virtqueue size from \field{queue_size}. This controls how big the virtqueue is
   (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}~\nameref{sec:Basic Facilities of a Virtio Device / Virtqueues}). If this field is 0, the virtqueue does not exist.
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 05/11] transport-mmio: Rename QueueNum register

[Parav Pandit]

These are further named differently between pci and mmio transport.
PCI transport indicates queue size as queue_size.

To bring consistency between pci and mmio transport,
rename the QueueNumMax and QueueNum
registers to QueueSizeMax and QueueSize respectively.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v8->v9:
- added field tag to indicate field name instead of English word
v0->v1:
- replaced references of QueueNumMax to QueueSizeMax
- replaced references of QueueNum to QueueSize
- added note for renamed fields old name suggested by @Michael Tsirkin
---
 transport-mmio.tex | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/transport-mmio.tex b/transport-mmio.tex
index f884a2c..164e640 100644
--- a/transport-mmio.tex
+++ b/transport-mmio.tex
@@ -110,24 +110,31 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
   \hline
   \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{%
     Writing to this register selects the virtual queue that the
-    following operations on \field{QueueNumMax}, \field{QueueNum}, \field{QueueReady},
+    following operations on \field{QueueSizeMax},
+    \field{QueueSize}, \field{QueueReady},
     \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
     \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to. The index
     number of the first queue is zero (0x0).
   }
   \hline
-  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
+  \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
     Reading from the register returns the maximum size (number of
     elements) of the queue the device is ready to process or
     zero (0x0) if the queue is not available. This applies to the
     queue selected by writing to \field{QueueSel}.
+    \begin{note}
+    \field{QueueSizeMax} was previously known as \field{QueueNumMax}.
+    \end{note}
   }
   \hline
-  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
+  \mmioreg{QueueSize}{Virtual queue size}{0x038}{W}{%
     Queue size is the number of elements in the queue.
     Writing to this register notifies the device what size of the
     queue the driver will use. This applies to the queue selected by
     writing to \field{QueueSel}.
+    \begin{note}
+    \field{QueueSize} was previously known as \field{QueueNum}.
+    \end{note}
   }
   \hline
   \mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{%
@@ -308,11 +315,11 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 
 Before writing to the \field{DriverFeatures} register, the driver MUST write a value to the \field{DriverFeaturesSel} register.
 
-The driver MUST write a value to \field{QueueNum} which is less than
-or equal to the value presented by the device in \field{QueueNumMax}.
+The driver MUST write a value to \field{QueueSize} which is less than
+or equal to the value presented by the device in \field{QueueSizeMax}.
 
 When \field{QueueReady} is not zero, the driver MUST NOT access
-\field{QueueNum}, \field{QueueDescLow}, \field{QueueDescHigh},
+\field{QueueSize}, \field{QueueDescLow}, \field{QueueDescHigh},
 \field{QueueDriverLow}, \field{QueueDriverHigh}, \field{QueueDeviceLow}, \field{QueueDeviceHigh}.
 
 To stop using the queue the driver MUST write zero (0x0) to this
@@ -363,14 +370,14 @@ \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / Vir
    and expect a returned value of zero (0x0).
 
 \item Read maximum queue size (number of elements) from
-   \field{QueueNumMax}. If the returned value is zero (0x0) the
+   \field{QueueSizeMax}. If the returned value is zero (0x0) the
    queue is not available.
 
 \item Allocate and zero the queue memory, making sure the memory
    is physically contiguous.
 
 \item Notify the device about the queue size by writing the size to
-   \field{QueueNum}.
+   \field{QueueSize}.
 
 \item Write physical addresses of the queue's Descriptor Area,
    Driver Area and Device Area to (respectively) the
@@ -465,25 +472,32 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
   \hline
   \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{%
     Writing to this register selects the virtual queue that the
-    following operations on the \field{QueueNumMax}, \field{QueueNum}, \field{QueueAlign}
+    following operations on the \field{QueueSizeMax},
+    \field{QueueSize}, \field{QueueAlign}
     and \field{QueuePFN} registers apply to. The index
     number of the first queue is zero (0x0).
 .
   }
   \hline
-  \mmioreg{QueueNumMax}{Maximum virtual queue size}{0x034}{R}{%
+  \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
     Reading from the register returns the maximum size of the queue
     the device is ready to process or zero (0x0) if the queue is not
     available. This applies to the queue selected by writing to
     \field{QueueSel} and is allowed only when \field{QueuePFN} is set to zero
     (0x0), so when the queue is not actively used.
+    \begin{note}
+    \field{QueueSizeMax} was previously known as \field{QueueNumMax}.
+    \end{note}
   }
   \hline
-  \mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{%
+  \mmioreg{QueueSize}{Virtual queue size}{0x038}{W}{%
     Queue size is the number of elements in the queue.
     Writing to this register notifies the device what size of the
     queue the driver will use. This applies to the queue selected by
     writing to \field{QueueSel}.
+    \begin{note}
+    \field{QueueSize} was previously known as \field{QueueNum}.
+    \end{note}
   }
   \hline
   \mmioreg{QueueAlign}{Used Ring alignment in the virtual queue}{0x03c}{W}{%
@@ -543,16 +557,16 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
    expecting a returned value of zero (0x0).
 
 \item Read maximum queue size (number of elements) from
-   \field{QueueNumMax}. If the returned value is zero (0x0) the
+   \field{QueueSizeMax}. If the returned value is zero (0x0) the
    queue is not available.
 
 \item Allocate and zero the queue pages in contiguous virtual
    memory, aligning the Used Ring to an optimal boundary (usually
    page size). The driver should choose a queue size smaller than or
-   equal to \field{QueueNumMax}.
+   equal to \field{QueueSizeMax}.
 
 \item Notify the device about the queue size by writing the size to
-   \field{QueueNum}.
+   \field{QueueSize}.
 
 \item Notify the device about the used alignment by writing its value
    in bytes to \field{QueueAlign}.
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 06/11] transport-mmio: Avoid referring to zero based index

[Parav Pandit]

VQ range is already described in the first patch in basic virtqueue
section. Hence remove the duplicate reference to it.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v12->v13:
- corrected number to index
v11->v12:
- remove changes related to 'vq number'
v8->v9:
- added 'by' at two places
- replaced 'queue number' with 'vq number'

v6->v7:
- remove text around first vq as it is already covered in the basic
  virtqueues facility section
---
 transport-mmio.tex | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/transport-mmio.tex b/transport-mmio.tex
index 164e640..2d24b4c 100644
--- a/transport-mmio.tex
+++ b/transport-mmio.tex
@@ -113,8 +113,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     following operations on \field{QueueSizeMax},
     \field{QueueSize}, \field{QueueReady},
     \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
-    \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to. The index
-    number of the first queue is zero (0x0).
+    \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to.
   }
   \hline
   \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
@@ -363,8 +362,7 @@ \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / Vir
 The driver will typically initialize the virtual queue in the following way:
 
 \begin{enumerate}
-\item Select the queue writing its index (first queue is 0) to
-   \field{QueueSel}.
+\item Select the queue by writing its index to \field{QueueSel}.
 
 \item Check if the queue is not already in use: read \field{QueueReady},
    and expect a returned value of zero (0x0).
@@ -474,9 +472,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
     Writing to this register selects the virtual queue that the
     following operations on the \field{QueueSizeMax},
     \field{QueueSize}, \field{QueueAlign}
-    and \field{QueuePFN} registers apply to. The index
-    number of the first queue is zero (0x0).
-.
+    and \field{QueuePFN} registers apply to.
   }
   \hline
   \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
@@ -550,8 +546,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
 
 The virtual queue is configured as follows:
 \begin{enumerate}
-\item Select the queue writing its index (first queue is 0) to
-   \field{QueueSel}.
+\item Select the queue by writing its index to \field{QueueSel}.
 
 \item Check if the queue is not already in use: read \field{QueuePFN},
    expecting a returned value of zero (0x0).
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 07/11] transport-ccw: Rename queue depth/size to other transports

[Parav Pandit]

max_num field reflects the maximum queue size/depth. Hence align name of
this field with similar field in PCI and MMIO transport to
max_queue_size.
Similarly rename 'num' to 'size'.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v9>v10:
- used lower case letter for comment first word
v8->v9:
- replaced 'named' as 'known'
v3->v4:
- moved note to comment
---
 transport-ccw.tex | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/transport-ccw.tex b/transport-ccw.tex
index c492cb9..cb476c7 100644
--- a/transport-ccw.tex
+++ b/transport-ccw.tex
@@ -237,12 +237,12 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
 \begin{lstlisting}
 struct vq_config_block {
         be16 index;
-        be16 max_num;
+        be16 max_queue_size; /* previously known as max_num */
 };
 \end{lstlisting}
 
 The requested number of buffers for queue \field{index} is returned in
-\field{max_num}.
+\field{max_queue_size}.
 
 Afterwards, CCW_CMD_SET_VQ is issued by the driver to inform the
 device about the location used for its queue. The transmitted
@@ -253,7 +253,7 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
         be64 desc;
         be32 res0;
         be16 index;
-        be16 num;
+        be16 size; /* previously known as num */
         be64 driver;
         be64 device;
 };
@@ -262,7 +262,7 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
 \field{desc}, \field{driver} and \field{device} contain the guest
 addresses for the descriptor area,
 available area and used area for queue \field{index}, respectively. The actual
-virtqueue size (number of allocated buffers) is transmitted in \field{num}.
+virtqueue size (number of allocated buffers) is transmitted in \field{size}.
 
 \devicenormative{\paragraph}{Configuring a Virtqueue}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Configuring a Virtqueue}
 
@@ -278,11 +278,12 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
         be64 queue;
         be32 align;
         be16 index;
-        be16 num;
+        be16 size; /* previously known as num */
 };
 \end{lstlisting}
 
-\field{queue} contains the guest address for queue \field{index}, \field{num} the number of buffers
+\field{queue} contains the guest address for queue \field{index},
+\field{size} the number of buffers
 and \field{align} the alignment. The queue layout follows \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}~\nameref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}.
 
 \subsubsection{Communicating Status Information}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Communicating Status Information}
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 08/11] transport-ccw: Refer to the vq by its index

[Parav Pandit]

Currently specification uses virtqueue index and
number interchangeably to refer to the virtqueue.

Instead refer to it by its index.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v11->v12:
- removed changes related to index
- replace number with index
- added 'only' to make it more clear that
  notification has only vq index
v9->v10:
- replaced queue_select with vqn
- used lower case letter for first word in comment
v8->v9:
- replaced 'named' with 'known'
- replaced 'queue number' with 'vq number'
v3->v4:
- moved note to comment
v2->v3:
- added comment note for queue_select similar to max_queue_size
v0->v1:
- new patch
---
 transport-ccw.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-ccw.tex b/transport-ccw.tex
index cb476c7..86272d1 100644
--- a/transport-ccw.tex
+++ b/transport-ccw.tex
@@ -545,7 +545,7 @@ \subsubsection{Guest->Host Notification}\label{sec:Virtio Transport Options / Vi
 \end{tabular}
 
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-the \field{Notification data} contains the Virtqueue number.
+the \field{Notification data} contains the virtqueue index.
 
 When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
 the value has the following format:
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 09/11] virtio-net: Avoid duplicate receive queue example

[Parav Pandit]

Receive queue number/index example is duplicate which is already defined
in the Setting RSS parameters section.

Hence, avoid such duplicate example and prepare it for the subsequent
patch to describe using receive queue handle.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 device-types/net/description.tex | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 875c4e6..f3f9f1d 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1478,8 +1478,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 The device MUST determine the destination queue for a network packet as follows:
 \begin{itemize}
 \item Calculate the hash of the packet as defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.
-\item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure (value of 0 corresponds to receiveq1).
-\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq (value of 0 corresponds to receiveq1).
+\item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.
+\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq.
 \item If the destination receive queue is being reset (See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.
 \end{itemize}
 
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

[Parav Pandit]

The content of the indirection table and unclassified_queue were
originally described based on mathematical operations. In order to
make it easier to understand and to avoid intermixing the array
index with the vq index, introduce a structure
rss_rq_id (RSS receive queue
ID) and use it to describe the unclassified_queue and
indirection_table fields.

As part of it, have the example that uses non-zero virtqueue
index which helps to have better mapping between receiveX
object with virtqueue index and the actual value in the
indirection table.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v12->v13:
- rename vqn to vq_index
- renamed vq index to virtqueue index
v11->v12:
- use 'virtqueue index' instead of 'virtqueue number'
v10->v11:
- commit log updated to drop old reference to rq_handle, replaced with
  rss_rq_id detail
v8->v9:
- reword rss_rq_id and unclassified_packets description
- use article
- use 'vq number' instead of 'virtqueue number'
v4->v5:
- change subject to reflect rss_rq_id
- fixed accidental removal of "unclassifed packets".
- simplfied text around indirection_table mask to use the term
  destination receive virtqueue. This aligns with next line about queue
  reset.
- removed rss_rq_id references as indirection table and
  unclassified_queue data type is self explanatory
v3->v4:
- renamed rq_handle to rss_rq_id
- moved rss_rq_id definition close to its usage in rss_config struct
v2->v3:
- moved rq_handle definition before using it
- removed duplicate example as rq_handle is now described first
v0->v1:
- new patch suggested by Michael Tsirkin
---
 device-types/net/description.tex | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index f3f9f1d..e4d236e 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1434,11 +1434,16 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command using the following format for \field{command-specific-data}:
 \begin{lstlisting}
+struct rss_rq_id {
+   le16 vq_index_1_16: 15; /* Bits 1 to 16 of the virtqueue index */
+   le16 reserved: 1; /* Set to zero */
+};
+
 struct virtio_net_rss_config {
     le32 hash_types;
     le16 indirection_table_mask;
-    le16 unclassified_queue;
-    le16 indirection_table[indirection_table_length];
+    struct rss_rq_id unclassified_queue;
+    struct rss_rq_id indirection_table[indirection_table_length];
     le16 max_tx_vq;
     u8 hash_key_length;
     u8 hash_key_data[hash_key_length];
@@ -1453,10 +1458,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \field{indirection_table} array.
 Number of entries in \field{indirection_table} is (\field{indirection_table_mask} + 1).
 
-Field \field{unclassified_queue} contains the 0-based index of
-the receive virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.
+\field{rss_rq_id} is a receive virtqueue id. \field{vq_index_1_16}
+consists of bits 1 to 16 of a virtqueue index. For example, a
+\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
+which maps to receiveq4.
+
+Field \field{unclassified_queue} contains the receive virtqueue
+in which to place unclassified packets.
 
-Field \field{indirection_table} contains an array of 0-based indices of receive virtqueues. Index 0 corresponds to receiveq1.
+Field \field{indirection_table} is an array of receive virtqueues.
 
 A driver sets \field{max_tx_vq} to inform a device how many transmit virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).
 
@@ -1466,7 +1476,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the feature VIRTIO_NET_F_RSS has not been negotiated.
 
-A driver MUST fill the \field{indirection_table} array only with indices of enabled queues. Index 0 corresponds to receiveq1.
+A driver MUST fill the \field{indirection_table} array only with
+enabled receive virtqueues.
 
 The number of entries in \field{indirection_table} (\field{indirection_table_mask} + 1) MUST be a power of two.
 
@@ -1479,7 +1490,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \begin{itemize}
 \item Calculate the hash of the packet as defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.
 \item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.
-\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq.
+\item Apply \field{indirection_table_mask} to the calculated hash
+and use the result as the index in the indirection table to get
+the destination receive virtqueue.
 \item If the destination receive queue is being reset (See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.
 \end{itemize}
 
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v14 11/11] virtio-net: Update vqn to vq_index for cvq cmds

[Parav Pandit]

Replace field name vqn to vq_index for recent virtqueue level commands.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v12->v13:
- new patch
---
 device-types/net/description.tex | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e4d236e..a55554c 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1560,7 +1560,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 };
 
 struct virtio_net_ctrl_coal_vq {
-    le16 vqn;
+    le16 vq_index;
     le16 reserved;
     struct virtio_net_ctrl_coal coal;
 };
@@ -1574,7 +1574,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 Coalescing parameters:
 \begin{itemize}
-\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.
+\item \field{vq_index}: The virtqueue index of an enabled transmit or receive virtqueue.
 \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
 \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
 \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
@@ -1587,7 +1587,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \begin{itemize}
 \item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, the structure virtio_net_ctrl_coal is write-only for the driver.
 \item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure virtio_net_ctrl_coal_vq is write-only for the driver.
-\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
+\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vq_index} and \field{reserved} are write-only
       for the driver, and the structure virtio_net_ctrl_coal is read-only for the driver.
 \end{itemize}
 
@@ -1596,9 +1596,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: use the structure virtio_net_ctrl_coal to set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues.
 \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: use the structure virtio_net_ctrl_coal to set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues.
 \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: use the structure virtio_net_ctrl_coal_vq to set the \field{max_usecs} and \field{max_packets} parameters
-                                        for an enabled transmit/receive virtqueue whose number is \field{vqn}.
+                                        for an enabled transmit/receive virtqueue whose index is \field{vq_index}.
 \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: use the structure virtio_net_ctrl_coal_vq to get the \field{max_usecs} and \field{max_packets} parameters
-                                        for an enabled transmit/receive virtqueue whose number is \field{vqn}.
+                                        for an enabled transmit/receive virtqueue whose index is \field{vq_index}.
 \end{enumerate}
 
 The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
@@ -1608,12 +1608,12 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 with two pairs of virtqueues as an example:
 Each of the following commands sets \field{max_usecs} and \field{max_packets} parameters for virtqueues.
 \begin{itemize}
-\item Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing parameters for virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and vqn 3 retain their previous parameters.
-\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets coalescing parameters for virtqueue having vqn 0. Virtqueue having vqn 2 retains the parameters from command1.
-\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of vqn 0 set by command2.
-\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3 retains its previous parameters.
-\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having vqn 1 and vqn 3, and overrides the parameters set by command4.
-\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the device responds with coalescing parameters of vqn 1 set by command5.
+\item Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing parameters for virtqueues having index 0 and index 2. Virtqueues having index 1 and index 3 retain their previous parameters.
+\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vq_index} = 0 sets coalescing parameters for virtqueue having index 0. Virtqueue having index 2 retains the parameters from command1.
+\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vq_index} = 0, the device responds with coalescing parameters of vq_index 0 set by command2.
+\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vq_index} = 1 sets coalescing parameters for virtqueue having index 1. Virtqueue having index 3 retains its previous parameters.
+\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing parameters for virtqueues having index 1 and index 3, and overrides the parameters set by command4.
+\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vq_index} = 1, the device responds with coalescing parameters of index 1 set by command5.
 \end{itemize}
 
 \subparagraph{Operation}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / Operation}
@@ -1663,7 +1663,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 
 \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 
-The driver MUST set \field{vqn} to the virtqueue number of an enabled transmit or receive virtqueue.
+The driver MUST set \field{vq_index} to the virtqueue index of an enabled transmit or receive virtqueue.
 
 The driver MUST have negotiated the VIRTIO_NET_F_NOTF_COAL feature when issuing commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
 
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v14 02/11] content.tex Replace virtqueue number with index

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:30 +0300
Parav Pandit <parav@nvidia.com> wrote:

> Replace virtqueue number with index to align to rest of the
> specification.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v11->v12:
> - new patch
> ---
>  content.tex | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 43be58b..e79d722 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -405,7 +405,7 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
>  
>  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
>  this notification involves sending the
> -virtqueue number to the device (method depending on the transport).
> +virtqueue index to the device (method depending on the transport).

This is not entirely accurate. If VIRTIO_F_NOTIF_CONFIG_DATA was
negotiated it is not the virtqueue index that is sent but the
vq_notif_config_data.

>  
>  However, some devices benefit from the ability to find out the
>  amount of available data in the queue without accessing the virtqueue in memory:
> @@ -789,13 +789,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    buffer notifications.
>    As mentioned in section \ref{sec:Basic Facilities of a Virtio Device / Driver notifications}, when the
>    driver is required to send an available buffer notification to the device, it
> -  sends the virtqueue number to be notified. The method of delivering
> +  sends the virtqueue index to be notified. The method of delivering
>    notifications is transport specific.
>    With the PCI transport, the device can optionally provide a per-virtqueue value
> -  for the driver to use in driver notifications, instead of the virtqueue number.
> +  for the driver to use in driver notifications, instead of the virtqueue index.
>    Some devices may benefit from this flexibility by providing, for example,
>    an internal virtqueue identifier, or an internal offset related to the
> -  virtqueue number.
> +  virtqueue index.
>  

And the above paragraph is supposed to make that point. 

I guess this is in some sense an improvement over the current state.
Thus:
Acked-by: Halil Pasic <pasic@linux.ibm.com>

But to get this really clean, we need to do some rewording here. Which
can be done on top of this series.


>    This feature indicates the availability of such value. The definition of the
>    data to be provided in driver notification and the delivery method is


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v14 02/11] content.tex Replace virtqueue number with index

[Parav Pandit]

> From: Halil Pasic <pasic@linux.ibm.com>
> Sent: Wednesday, April 19, 2023 12:09 PM
> >  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,  this
> > notification involves sending the -virtqueue number to the device
> > (method depending on the transport).
> > +virtqueue index to the device (method depending on the transport).
> 
> This is not entirely accurate. If VIRTIO_F_NOTIF_CONFIG_DATA was negotiated
> it is not the virtqueue index that is sent but the vq_notif_config_data.
> 
You are correct.
Patch 3 further fixes this as part of CONFIG_DATA related cleanup.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v14 03/11] content: Rename confusing queue_notify_data and vqn names

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:31 +0300
Parav Pandit <parav@nvidia.com> wrote:

> Currently queue_notify_data register indicates the device
> internal queue notification content. This register is
> meaningful only when feature bit VIRTIO_F_NOTIF_CONFIG_DATA is
> negotiated.
> 
> However, above register name often get confusing association with
> very similar feature bit VIRTIO_F_NOTIFICATION_DATA.
> 
> When VIRTIO_F_NOTIFICATION_DATA feature bit is negotiated,
> notification really involves sending additional queue progress
> related information (not queue identifier or index).
> 
> Hence
> 1. to avoid any misunderstanding and association of
> queue_notify_data with similar name VIRTIO_F_NOTIFICATION_DATA,
> 
> and
> 2. to reflect that queue_notify_data is the actual device
> internal virtqueue identifier/index/data/cookie,
> 
> a. rename queue_notify_data to queue_notif_config_data.
> 
> b. rename ambiguous vqn to a union of vq_index and vq_config_data
> 
> c. The driver notification section assumes that queue notification contains
> vq index always. CONFIG_DATA feature bit introduction missed to
> update the driver notification section. Hence, correct it.
[..]
> diff --git a/content.tex b/content.tex
> index e79d722..e1345ea 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -404,8 +404,12 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
>  notification to the device.
>  
>  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> -this notification involves sending the
> -virtqueue index to the device (method depending on the transport).
> +this notification contains either virtqueue index if VIRTIO_F_NOTIF_CONFIG_DATA
> +is not negotiated or device supplied virtqueue notification config data if
> +VIRTIO_F_NOTIF_CONFIG_DATA is negotiated.

Here comes the cleanup I talked about in v3. 

> +
> +A notification method and suppling such virtqueue notification config data is
> +transport specific.
>  
>  However, some devices benefit from the ability to find out the
>  amount of available data in the queue without accessing the virtqueue in memory:
> @@ -416,7 +420,8 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
>  the following information:
>  
>  \begin{description}
> -\item [vqn] VQ number to be notified.
> +\item [vq_index_config_data] Either virtqueue index or device supplied

Apparently this our name for either index or notification config data,
but I don't think we ever use it.


> +      queue notification config data corresponding to a virtqueue.
>  \item [next_off] Offset
>        within the ring where the next available ring entry
>        will be written.
> diff --git a/notifications-be.c b/notifications-be.c
> --- a/notifications-be.c
> +++ b/notifications-be.c
> @@ -1,5 +1,5 @@
>  be32 {
> -	vqn : 16;
> +	vq_index: 16; /* previously known as vqn */
>  	next_off : 15;
>  	next_wrap : 1;
>  };
> diff --git a/notifications-le.c b/notifications-le.c
> index fe51267..8a19389 100644
> --- a/notifications-le.c
> +++ b/notifications-le.c

Should this file  be renamed to notifications-mmio-le.c? It is only
used for mmio now.

I guess, there is a two to think about it:
* Notifications are mostly transport specific. In particular what
  data structures are used for the virtqueue notifications and how
  they are passed, that is entirely transport specific. So the 'layout'
  and the normative statements describing the details should be in the
  transport sections.
* The facility bits and the capabilities are basically common. I.e. if
  MMIO or/and Channel I/O were to ever support VIRTIO_F_NOTIF_CONFIG data
  (by providing means for transporting the virtqueue notification config
  data from the device to the driver), what you call the notification
  value would be either virtqueue index or this notification config data.

You are taking the first approach here, and I'm fine with that. IMHO
before we had the second approach implemented with vqn. I'm fine
with taking the first approach as well, but your commit message says
"rename ambiguous vqn to a union of vq_index and vq_config_data" which
for me implies you actually intended to take the second approach.

> @@ -1,5 +1,5 @@
>  le32 {
> -	vqn : 16;
> +	vq_index: 16; /* previously known as vqn */
>  	next_off : 15;
>  	next_wrap : 1;
>  };
> diff --git a/notifications-pci-le.c b/notifications-pci-le.c
> new file mode 100644
> index 0000000..ef60d15
> --- /dev/null
> +++ b/notifications-pci-le.c
> @@ -0,0 +1,8 @@
> +le32 {
> +	union {
> +		vq_index: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not negotiated */
> +		vq_notif_config_data: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA negotiated */ 
> +	};
> +	next_off : 15;
> +	next_wrap : 1;
> +};
> diff --git a/transport-pci.tex b/transport-pci.tex
> index 5d98467..04a9a80 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -319,7 +319,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          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_notif_config_data;   /* read-only for driver */
>          le16 queue_reset;               /* read-write */
>  };
>  \end{lstlisting}
> @@ -388,17 +388,21 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  \item[\field{queue_device}]
>          The driver writes the physical address of Device Area here.  See section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
>  
> -\item[\field{queue_notify_data}]
> +\item[\field{queue_notif_config_data}]
>          This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated.
> -        The driver will use this value to put it in the 'virtqueue number' field
> -        in the available buffer notification structure.
> +        The driver will use this value when driver sends available buffer
> +        notification to the device.
>          See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>          \begin{note}
>          This field provides the device with flexibility to determine how virtqueues
>          will be referred to in available buffer notifications.
> -        In a trivial case the device can set \field{queue_notify_data}=vqn. Some devices
> -        may benefit from providing another value, for example an internal virtqueue
> -        identifier, or an internal offset related to the virtqueue number.
> +        In a trivial case the device can set \field{queue_notif_config_data} to
> +        virtqueue index. Some devices may benefit from providing another value,
> +        for example an internal virtqueue identifier, or an internal offset
> +        related to the virtqueue index.
> +        \end{note}
> +        \begin{note}
> +        This field is previously known as queue_notify_data.
>          \end{note}
>  
>  \item[\field{queue_reset}]
> @@ -468,7 +472,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  
>  \drivernormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
>  
> -The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
> +The driver MUST NOT write to \field{device_feature}, \field{num_queues},
> +\field{config_generation}, \field{queue_notify_off} or
> +\field{queue_notif_config_data}.
>  
>  If VIRTIO_F_RING_PACKED has been negotiated,
>  the driver MUST NOT write the value 0 to \field{queue_size}.
> @@ -1035,13 +1041,22 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
>  
>  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
>  the driver sends an available buffer notification to the device by writing
> -the 16-bit virtqueue index
> -of this virtqueue to the Queue Notify address.
> +only the 16-bit notification value to the Queue Notify address of the


You introduce the term notification value. Which basically means
virtqueue index or virtqueue notification config data. Which is basically
what is defined where you define vq_index_config_data (the latter is
never ever used again).

> +virtqueue. A notification value depends on the negotiation of
> +VIRTIO_F_NOTIF_CONFIG_DATA.
>  
> -When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> -the driver sends an available buffer notification to the device by writing
> -the following 32-bit value to the Queue Notify address:
> -\lstinputlisting{notifications-le.c}
> +\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver sends an


The leading \item is probably not intended. Breaks the build and makes
no sense.

> +available buffer notification to the device by writing the following 32-bit
> +value to the Queue Notify address:
> +\lstinputlisting{notifications-pci-le.c}
> +
> +\begin{itemize}
> +\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated \field{vq_index} is set
> +to the virtqueue index.
> +
> +\item When VIRTIO_F_NOTIFICATION_DATA is negotiated,
> +\field{vq_notif_config_data} is set to \field{queue_notif_config_data}.
> +\end{itemize}
>  
>  See \ref{sec:Basic Facilities of a Virtio Device / Driver notifications}~\nameref{sec:Basic Facilities of a Virtio Device / Driver notifications}
>  for the definition of the components.
> @@ -1050,12 +1065,31 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
>  for how to calculate the Queue Notify address.
>  
>  \drivernormative{\paragraph}{Available Buffer Notifications}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}
> -If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
> +
> +If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver notification 
> +MUST be a 16-bit notification.
> +
> +If VIRTIO_F_NOTIFICATION_DATA is negotiated, the driver notification 
> +MUST be a 32-bit notification.
> +
> +If VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated:
> +\begin{itemize}
> +\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set the
> +notification value to virtqueue index.
> +
> +\item If VIRTIO_F_NOTIFICATION_DATA is negotiated, the driver MUST set the
> +\field{vq_index} to the virtqueue index.
> +
> +\end{itemize}
> +
> +If VIRTIO_F_NOTIF_CONFIG_DATA is negotiated:
>  \begin{itemize}
> -\item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver MUST use the
> -\field{queue_notify_data} value instead of the virtqueue index.
> -\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver MUST set the
> -\field{vqn} field to the \field{queue_notify_data} value.
> +\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set
> +the notification value to \field{queue_notif_config_data}.
> +
> +\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set the
> +\field{vq_notify_config_data} to the \field{queue_notif_config_data}
> +value.
>  \end{itemize}
>  
>  \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH v14 05/11] transport-mmio: Rename QueueNum register

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:33 +0300
Parav Pandit <parav@nvidia.com> wrote:

> These are further named differently between pci and mmio transport.
> PCI transport indicates queue size as queue_size.
> 
> To bring consistency between pci and mmio transport,
> rename the QueueNumMax and QueueNum
> registers to QueueSizeMax and QueueSize respectively.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH v14 07/11] transport-ccw: Rename queue depth/size to other transports

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:35 +0300
Parav Pandit <parav@nvidia.com> wrote:

> max_num field reflects the maximum queue size/depth. Hence align name of
> this field with similar field in PCI and MMIO transport to
> max_queue_size.
> Similarly rename 'num' to 'size'.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH v14 09/11] virtio-net: Avoid duplicate receive queue example

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:37 +0300
Parav Pandit <parav@nvidia.com> wrote:

> Receive queue number/index example is duplicate which is already defined
> in the Setting RSS parameters section.
> 
> Hence, avoid such duplicate example and prepare it for the subsequent
> patch to describe using receive queue handle.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:38 +0300
Parav Pandit <parav@nvidia.com> wrote:

> The content of the indirection table and unclassified_queue were
> originally described based on mathematical operations. In order to
> make it easier to understand and to avoid intermixing the array
> index with the vq index, introduce a structure
> rss_rq_id (RSS receive queue
> ID) and use it to describe the unclassified_queue and
> indirection_table fields.
> 
> As part of it, have the example that uses non-zero virtqueue
> index which helps to have better mapping between receiveX
> object with virtqueue index and the actual value in the
> indirection table.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v12->v13:
> - rename vqn to vq_index
> - renamed vq index to virtqueue index
> v11->v12:
> - use 'virtqueue index' instead of 'virtqueue number'
> v10->v11:
> - commit log updated to drop old reference to rq_handle, replaced with
>   rss_rq_id detail
> v8->v9:
> - reword rss_rq_id and unclassified_packets description
> - use article
> - use 'vq number' instead of 'virtqueue number'
> v4->v5:
> - change subject to reflect rss_rq_id
> - fixed accidental removal of "unclassifed packets".
> - simplfied text around indirection_table mask to use the term
>   destination receive virtqueue. This aligns with next line about queue
>   reset.
> - removed rss_rq_id references as indirection table and
>   unclassified_queue data type is self explanatory
> v3->v4:
> - renamed rq_handle to rss_rq_id
> - moved rss_rq_id definition close to its usage in rss_config struct
> v2->v3:
> - moved rq_handle definition before using it
> - removed duplicate example as rq_handle is now described first
> v0->v1:
> - new patch suggested by Michael Tsirkin
> ---
>  device-types/net/description.tex | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index f3f9f1d..e4d236e 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -1434,11 +1434,16 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  
>  Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command using the following format for \field{command-specific-data}:
>  \begin{lstlisting}
> +struct rss_rq_id {
> +   le16 vq_index_1_16: 15; /* Bits 1 to 16 of the virtqueue index */
> +   le16 reserved: 1; /* Set to zero */
> +};
> +

I'm not a fan of this being done with bitmaps, as I have stated before,
but I can live with it.

>  struct virtio_net_rss_config {
>      le32 hash_types;
>      le16 indirection_table_mask;
> -    le16 unclassified_queue;
> -    le16 indirection_table[indirection_table_length];
> +    struct rss_rq_id unclassified_queue;
> +    struct rss_rq_id indirection_table[indirection_table_length];
>      le16 max_tx_vq;
>      u8 hash_key_length;
>      u8 hash_key_data[hash_key_length];
> @@ -1453,10 +1458,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \field{indirection_table} array.
>  Number of entries in \field{indirection_table} is (\field{indirection_table_mask} + 1).
>  
> -Field \field{unclassified_queue} contains the 0-based index of
> -the receive virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.
> +\field{rss_rq_id} is a receive virtqueue id. \field{vq_index_1_16}
> +consists of bits 1 to 16 of a virtqueue index. For example, a
> +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> +which maps to receiveq4.
> +
> +Field \field{unclassified_queue} contains the receive virtqueue
> +in which to place unclassified packets.

It does not contain a receive virtqueue but its \field{rss_rq_id},
i.e. it's "receive virtqueue id".

>  
> -Field \field{indirection_table} contains an array of 0-based indices of receive virtqueues. Index 0 corresponds to receiveq1.
> +Field \field{indirection_table} is an array of receive virtqueues.

Same here.

>  
>  A driver sets \field{max_tx_vq} to inform a device how many transmit virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).
>  
> @@ -1466,7 +1476,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  
>  A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the feature VIRTIO_NET_F_RSS has not been negotiated.
>  
> -A driver MUST fill the \field{indirection_table} array only with indices of enabled queues. Index 0 corresponds to receiveq1.
> +A driver MUST fill the \field{indirection_table} array only with
> +enabled receive virtqueues.

Same here.
>  
>  The number of entries in \field{indirection_table} (\field{indirection_table_mask} + 1) MUST be a power of two.
>  
> @@ -1479,7 +1490,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  \begin{itemize}
>  \item Calculate the hash of the packet as defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.
>  \item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.
> -\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq.
> +\item Apply \field{indirection_table_mask} to the calculated hash
> +and use the result as the index in the indirection table to get
> +the destination receive virtqueue.

Same here.

>  \item If the destination receive queue is being reset (See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.
>  \end{itemize}
>  


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH v14 11/11] virtio-net: Update vqn to vq_index for cvq cmds

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:39 +0300
Parav Pandit <parav@nvidia.com> wrote:

> Replace field name vqn to vq_index for recent virtqueue level commands.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:32 +0300
Parav Pandit <parav@nvidia.com> wrote:

> Drop reference to first virtqueue as it is already
> covered now by the generic section in first patch.
> 

TL;DR:

Acked-by: Halil Pasic <pasic@linux.ibm.com>

Rationale: 
---------
I agree, what a virtqueue index is should be described in a
single place instead all over the place. And further improvements can be done
on top of this series. 

Discussion:
-----------
Is it indeed covered now by the generic section in first patch?

In [1] you state the following: "There is nothing like a zero based
index. A VQ can be any u16 _number_ in range of 0 to 65534.
Number can also start from zero. So zero based index = zero based
number."

I don't really understand what do you mean by this, but I'm afraid
it is not consistent with my understanding. Can not be any number in
range of 0 to 65534.

Rather in general it depends on the device type, and on the device
how many queues it support. Let us call this number dev_vq_max. And
if dev_vq_max > N > 0 index is associated with a queue the N-1 index is
also associated with a queue.

Let me also note that the number of queues supported/provided by the
device is a matter of the device. For many devices the number of queues
is a constant given in the specification. A Network Device tells
its driver how may queues it supports via max_virtqueue_pairs,
Crypto Device via max_dataqueues, and Console Device via max_dataqueues.
The transports PCI also has num_queues which is documented as "The
device specifies the maximum number of virtqueues supported here." while
CCW and MMIO have no transport specific means to tell the driver about
the number of queues supported by the device. In any case, if the number
of queues provided by the device is N, each of those queues is uniquely
identified by exactly one index from the range [0..N-1]. Furthermore
the role a certain queue plays is determined by its index. E.g. for
the Console Device virtqueue identified by the index 3 is the "control
receiveq".

[1]
https://lore.kernel.org/virtio-comment/d62d2090-de0d-0ea6-5c85-ecf19ae828c7@nvidia.com/

[..]
>  
>  \begin{enumerate}
> -\item Write the virtqueue index (first queue is 0) to \field{queue_select}.
> +\item Write the virtqueue index to \field{queue_select}.
>  
>  \item Read the virtqueue size from \field{queue_size}. This controls how big the virtqueue is
>    (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}~\nameref{sec:Basic Facilities of a Virtio Device / Virtqueues}). If this field is 0, the virtqueue does not exist.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

[Michael S. Tsirkin]

On Mon, Apr 24, 2023 at 03:22:21PM +0200, Halil Pasic wrote:
> On Wed, 19 Apr 2023 04:46:38 +0300
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > The content of the indirection table and unclassified_queue were
> > originally described based on mathematical operations. In order to
> > make it easier to understand and to avoid intermixing the array
> > index with the vq index, introduce a structure
> > rss_rq_id (RSS receive queue
> > ID) and use it to describe the unclassified_queue and
> > indirection_table fields.
> > 
> > As part of it, have the example that uses non-zero virtqueue
> > index which helps to have better mapping between receiveX
> > object with virtqueue index and the actual value in the
> > indirection table.
> > 
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > 
> > ---
> > changelog:
> > v12->v13:
> > - rename vqn to vq_index
> > - renamed vq index to virtqueue index
> > v11->v12:
> > - use 'virtqueue index' instead of 'virtqueue number'
> > v10->v11:
> > - commit log updated to drop old reference to rq_handle, replaced with
> >   rss_rq_id detail
> > v8->v9:
> > - reword rss_rq_id and unclassified_packets description
> > - use article
> > - use 'vq number' instead of 'virtqueue number'
> > v4->v5:
> > - change subject to reflect rss_rq_id
> > - fixed accidental removal of "unclassifed packets".
> > - simplfied text around indirection_table mask to use the term
> >   destination receive virtqueue. This aligns with next line about queue
> >   reset.
> > - removed rss_rq_id references as indirection table and
> >   unclassified_queue data type is self explanatory
> > v3->v4:
> > - renamed rq_handle to rss_rq_id
> > - moved rss_rq_id definition close to its usage in rss_config struct
> > v2->v3:
> > - moved rq_handle definition before using it
> > - removed duplicate example as rq_handle is now described first
> > v0->v1:
> > - new patch suggested by Michael Tsirkin
> > ---
> >  device-types/net/description.tex | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > index f3f9f1d..e4d236e 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -1434,11 +1434,16 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  
> >  Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command

maybe add:
	including virtqueue index of relevant receive queues

> using the following format for \field{command-specific-data}:
> >  \begin{lstlisting}
> > +struct rss_rq_id {
> > +   le16 vq_index_1_16: 15; /* Bits 1 to 16 of the virtqueue index */
> > +   le16 reserved: 1; /* Set to zero */
> > +};
> > +
> 
> I'm not a fan of this being done with bitmaps, as I have stated before,
> but I can live with it.

Same.

> >  struct virtio_net_rss_config {
> >      le32 hash_types;
> >      le16 indirection_table_mask;
> > -    le16 unclassified_queue;
> > -    le16 indirection_table[indirection_table_length];
> > +    struct rss_rq_id unclassified_queue;
> > +    struct rss_rq_id indirection_table[indirection_table_length];
> >      le16 max_tx_vq;
> >      u8 hash_key_length;
> >      u8 hash_key_data[hash_key_length];
> > @@ -1453,10 +1458,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \field{indirection_table} array.
> >  Number of entries in \field{indirection_table} is (\field{indirection_table_mask} + 1).
> >  
> > -Field \field{unclassified_queue} contains the 0-based index of
> > -the receive virtqueue to place unclassified packets in. Index 0 corresponds to receiveq1.
> > +\field{rss_rq_id} is a receive virtqueue id. \field{vq_index_1_16}
> > +consists of bits 1 to 16 of a virtqueue index. For example, a
> > +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> > +which maps to receiveq4.
> > +
> > +Field \field{unclassified_queue} contains the receive virtqueue
> > +in which to place unclassified packets.
> 
> It does not contain a receive virtqueue but its \field{rss_rq_id},
> i.e. it's "receive virtqueue id".

Yes all this last chunk is not an improvement.  
My whole point was that we do not need a name for this
thing that is struct rss_rq_id. It's just a weird way to store a vq
index.

Also \field{rss_rq_id} is confusing since it's actually
\field{struct rss_rq_id}. We are using C-like not C++ like syntax ;)

One way to address all this:

\field{struct rss_rq_id} contains a virtqueue index: \field{vq_index_1_16}
consists of bits 1 to 16 of a virtqueue index. For example, a
\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
which maps to receiveq4.

Field \field{unclassified_queue} contains the virtqueue index
of the receive virtqueue to place unclassified packets in,
in \field{struct rss_rq_id} format.





> > +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> > +which maps to receiveq4.
> > +
> > +Field \field{unclassified_queue} contains the receive virtqueue
> > +in which to place unclassified packets.




> >  
> > -Field \field{indirection_table} contains an array of 0-based indices of receive virtqueues. Index 0 corresponds to receiveq1.
> > +Field \field{indirection_table} is an array of receive virtqueues.
> 
> Same here.
> 
> >  
> >  A driver sets \field{max_tx_vq} to inform a device how many transmit virtqueues it may use (transmitq1\ldots transmitq \field{max_tx_vq}).
> >  
> > @@ -1466,7 +1476,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  
> >  A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the feature VIRTIO_NET_F_RSS has not been negotiated.
> >  
> > -A driver MUST fill the \field{indirection_table} array only with indices of enabled queues. Index 0 corresponds to receiveq1.
> > +A driver MUST fill the \field{indirection_table} array only with
> > +enabled receive virtqueues.
> 
> Same here.
> >  
> >  The number of entries in \field{indirection_table} (\field{indirection_table_mask} + 1) MUST be a power of two.
> >  
> > @@ -1479,7 +1490,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >  \begin{itemize}
> >  \item Calculate the hash of the packet as defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets}.
> >  \item If the device did not calculate the hash for the specific packet, the device directs the packet to the receiveq specified by \field{unclassified_queue} of virtio_net_rss_config structure.
> > -\item Apply \field{indirection_table_mask} to the calculated hash and use the result as the index in the indirection table to get 0-based number of destination receiveq.
> > +\item Apply \field{indirection_table_mask} to the calculated hash
> > +and use the result as the index in the indirection table to get
> > +the destination receive virtqueue.
> 
> Same here.
> 
> >  \item If the destination receive queue is being reset (See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}), the device MUST drop the packet.
> >  \end{itemize}
> >  


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v14 03/11] content: Rename confusing queue_notify_data and vqn names

[Michael S. Tsirkin]

On Sat, Apr 22, 2023 at 12:37:57AM +0200, Halil Pasic wrote:
> On Wed, 19 Apr 2023 04:46:31 +0300
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > Currently queue_notify_data register indicates the device
> > internal queue notification content. This register is
> > meaningful only when feature bit VIRTIO_F_NOTIF_CONFIG_DATA is
> > negotiated.
> > 
> > However, above register name often get confusing association with
> > very similar feature bit VIRTIO_F_NOTIFICATION_DATA.
> > 
> > When VIRTIO_F_NOTIFICATION_DATA feature bit is negotiated,
> > notification really involves sending additional queue progress
> > related information (not queue identifier or index).
> > 
> > Hence
> > 1. to avoid any misunderstanding and association of
> > queue_notify_data with similar name VIRTIO_F_NOTIFICATION_DATA,
> > 
> > and
> > 2. to reflect that queue_notify_data is the actual device
> > internal virtqueue identifier/index/data/cookie,
> > 
> > a. rename queue_notify_data to queue_notif_config_data.
> > 
> > b. rename ambiguous vqn to a union of vq_index and vq_config_data
> > 
> > c. The driver notification section assumes that queue notification contains
> > vq index always. CONFIG_DATA feature bit introduction missed to
> > update the driver notification section. Hence, correct it.
> [..]
> > diff --git a/content.tex b/content.tex
> > index e79d722..e1345ea 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -404,8 +404,12 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
> >  notification to the device.
> >  
> >  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > -this notification involves sending the
> > -virtqueue index to the device (method depending on the transport).
> > +this notification contains either virtqueue index if VIRTIO_F_NOTIF_CONFIG_DATA
> > +is not negotiated or device supplied virtqueue notification config data if
> > +VIRTIO_F_NOTIF_CONFIG_DATA is negotiated.
> 
> Here comes the cleanup I talked about in v3. 
> 
> > +
> > +A notification method and suppling such virtqueue notification config data is
> > +transport specific.
> >  
> >  However, some devices benefit from the ability to find out the
> >  amount of available data in the queue without accessing the virtqueue in memory:
> > @@ -416,7 +420,8 @@ \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device /
> >  the following information:
> >  
> >  \begin{description}
> > -\item [vqn] VQ number to be notified.
> > +\item [vq_index_config_data] Either virtqueue index or device supplied
> 
> Apparently this our name for either index or notification config data,
> but I don't think we ever use it.
> 
> 
> > +      queue notification config data corresponding to a virtqueue.
> >  \item [next_off] Offset
> >        within the ring where the next available ring entry
> >        will be written.
> > diff --git a/notifications-be.c b/notifications-be.c
> > --- a/notifications-be.c
> > +++ b/notifications-be.c
> > @@ -1,5 +1,5 @@
> >  be32 {
> > -	vqn : 16;
> > +	vq_index: 16; /* previously known as vqn */
> >  	next_off : 15;
> >  	next_wrap : 1;
> >  };
> > diff --git a/notifications-le.c b/notifications-le.c
> > index fe51267..8a19389 100644
> > --- a/notifications-le.c
> > +++ b/notifications-le.c
> 
> Should this file  be renamed to notifications-mmio-le.c? It is only
> used for mmio now.

I think it is good policy to name files after what they contain not after how they
are used.

> I guess, there is a two to think about it:
> * Notifications are mostly transport specific. In particular what
>   data structures are used for the virtqueue notifications and how
>   they are passed, that is entirely transport specific. So the 'layout'
>   and the normative statements describing the details should be in the
>   transport sections.
> * The facility bits and the capabilities are basically common. I.e. if
>   MMIO or/and Channel I/O were to ever support VIRTIO_F_NOTIF_CONFIG data
>   (by providing means for transporting the virtqueue notification config
>   data from the device to the driver), what you call the notification
>   value would be either virtqueue index or this notification config data.
> 
> You are taking the first approach here, and I'm fine with that. IMHO
> before we had the second approach implemented with vqn. I'm fine
> with taking the first approach as well, but your commit message says
> "rename ambiguous vqn to a union of vq_index and vq_config_data" which
> for me implies you actually intended to take the second approach.
> 
> > @@ -1,5 +1,5 @@
> >  le32 {
> > -	vqn : 16;
> > +	vq_index: 16; /* previously known as vqn */
> >  	next_off : 15;
> >  	next_wrap : 1;
> >  };
> > diff --git a/notifications-pci-le.c b/notifications-pci-le.c
> > new file mode 100644
> > index 0000000..ef60d15
> > --- /dev/null
> > +++ b/notifications-pci-le.c
> > @@ -0,0 +1,8 @@
> > +le32 {
> > +	union {
> > +		vq_index: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not negotiated */
> > +		vq_notif_config_data: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA negotiated */ 
> > +	};
> > +	next_off : 15;
> > +	next_wrap : 1;
> > +};
> > diff --git a/transport-pci.tex b/transport-pci.tex
> > index 5d98467..04a9a80 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -319,7 +319,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >          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_notif_config_data;   /* read-only for driver */
> >          le16 queue_reset;               /* read-write */
> >  };
> >  \end{lstlisting}
> > @@ -388,17 +388,21 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >  \item[\field{queue_device}]
> >          The driver writes the physical address of Device Area here.  See section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
> >  
> > -\item[\field{queue_notify_data}]
> > +\item[\field{queue_notif_config_data}]
> >          This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated.
> > -        The driver will use this value to put it in the 'virtqueue number' field
> > -        in the available buffer notification structure.
> > +        The driver will use this value when driver sends available buffer
> > +        notification to the device.
> >          See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
> >          \begin{note}
> >          This field provides the device with flexibility to determine how virtqueues
> >          will be referred to in available buffer notifications.
> > -        In a trivial case the device can set \field{queue_notify_data}=vqn. Some devices
> > -        may benefit from providing another value, for example an internal virtqueue
> > -        identifier, or an internal offset related to the virtqueue number.
> > +        In a trivial case the device can set \field{queue_notif_config_data} to
> > +        virtqueue index. Some devices may benefit from providing another value,
> > +        for example an internal virtqueue identifier, or an internal offset
> > +        related to the virtqueue index.
> > +        \end{note}
> > +        \begin{note}
> > +        This field is previously known as queue_notify_data.
> >          \end{note}
> >  
> >  \item[\field{queue_reset}]
> > @@ -468,7 +472,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >  
> >  \drivernormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
> >  
> > -The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
> > +The driver MUST NOT write to \field{device_feature}, \field{num_queues},
> > +\field{config_generation}, \field{queue_notify_off} or
> > +\field{queue_notif_config_data}.
> >  
> >  If VIRTIO_F_RING_PACKED has been negotiated,
> >  the driver MUST NOT write the value 0 to \field{queue_size}.
> > @@ -1035,13 +1041,22 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
> >  
> >  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> >  the driver sends an available buffer notification to the device by writing
> > -the 16-bit virtqueue index
> > -of this virtqueue to the Queue Notify address.
> > +only the 16-bit notification value to the Queue Notify address of the
> 
> 
> You introduce the term notification value. Which basically means
> virtqueue index or virtqueue notification config data. Which is basically
> what is defined where you define vq_index_config_data (the latter is
> never ever used again).
> 
> > +virtqueue. A notification value depends on the negotiation of
> > +VIRTIO_F_NOTIF_CONFIG_DATA.
> >  
> > -When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > -the driver sends an available buffer notification to the device by writing
> > -the following 32-bit value to the Queue Notify address:
> > -\lstinputlisting{notifications-le.c}
> > +\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver sends an
> 
> 
> The leading \item is probably not intended. Breaks the build and makes
> no sense.
> 
> > +available buffer notification to the device by writing the following 32-bit
> > +value to the Queue Notify address:
> > +\lstinputlisting{notifications-pci-le.c}
> > +
> > +\begin{itemize}
> > +\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated \field{vq_index} is set
> > +to the virtqueue index.
> > +
> > +\item When VIRTIO_F_NOTIFICATION_DATA is negotiated,
> > +\field{vq_notif_config_data} is set to \field{queue_notif_config_data}.
> > +\end{itemize}
> >  
> >  See \ref{sec:Basic Facilities of a Virtio Device / Driver notifications}~\nameref{sec:Basic Facilities of a Virtio Device / Driver notifications}
> >  for the definition of the components.
> > @@ -1050,12 +1065,31 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
> >  for how to calculate the Queue Notify address.
> >  
> >  \drivernormative{\paragraph}{Available Buffer Notifications}{Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}
> > -If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
> > +
> > +If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver notification 
> > +MUST be a 16-bit notification.
> > +
> > +If VIRTIO_F_NOTIFICATION_DATA is negotiated, the driver notification 
> > +MUST be a 32-bit notification.
> > +
> > +If VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated:
> > +\begin{itemize}
> > +\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set the
> > +notification value to virtqueue index.
> > +
> > +\item If VIRTIO_F_NOTIFICATION_DATA is negotiated, the driver MUST set the
> > +\field{vq_index} to the virtqueue index.
> > +
> > +\end{itemize}
> > +
> > +If VIRTIO_F_NOTIF_CONFIG_DATA is negotiated:
> >  \begin{itemize}
> > -\item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver MUST use the
> > -\field{queue_notify_data} value instead of the virtqueue index.
> > -\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver MUST set the
> > -\field{vqn} field to the \field{queue_notify_data} value.
> > +\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set
> > +the notification value to \field{queue_notif_config_data}.
> > +
> > +\item If VIRTIO_F_NOTIFICATION_DATA is not negotiated, the driver MUST set the
> > +\field{vq_notify_config_data} to the \field{queue_notif_config_data}
> > +value.
> >  \end{itemize}
> >  
> >  \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, April 24, 2023 9:59 AM

> > >  Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command
> 
> maybe add:
> 	including virtqueue index of relevant receive queues
> 
Structure struct virtio_net_rss_config has many fields for RSS_CONFIG command.
It is not much help to say including receive queues, including indirection table etc.
The structure is evident and all those fields are described in the struct including the receive virtqueue.


> > >  struct virtio_net_rss_config {
> > >      le32 hash_types;
> > >      le16 indirection_table_mask;
> > > -    le16 unclassified_queue;
> > > -    le16 indirection_table[indirection_table_length];
> > > +    struct rss_rq_id unclassified_queue;
> > > +    struct rss_rq_id indirection_table[indirection_table_length];
> > >      le16 max_tx_vq;
> > >      u8 hash_key_length;
> > >      u8 hash_key_data[hash_key_length]; @@ -1453,10 +1458,15 @@
> > > \subsubsection{Control Virtqueue}\label{sec:Device Types / Network
> > > Device / Devi  \field{indirection_table} array.
> > >  Number of entries in \field{indirection_table} is
> (\field{indirection_table_mask} + 1).
> > >
> > > -Field \field{unclassified_queue} contains the 0-based index of -the
> > > receive virtqueue to place unclassified packets in. Index 0 corresponds to
> receiveq1.
> > > +\field{rss_rq_id} is a receive virtqueue id. \field{vq_index_1_16}
> > > +consists of bits 1 to 16 of a virtqueue index. For example, a
> > > +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> > > +which maps to receiveq4.
> > > +
> > > +Field \field{unclassified_queue} contains the receive virtqueue in
> > > +which to place unclassified packets.
> >
> > It does not contain a receive virtqueue but its \field{rss_rq_id},
> > i.e. it's "receive virtqueue id".
> 
receive virtqueue != virtqueue index.
The receive virtqueue is description. The structure rss_rq_id is clear enough to indicate that how an receive virtqueue is communicated with the device.

> Yes all this last chunk is not an improvement.
> My whole point was that we do not need a name for this thing that is struct
> rss_rq_id. It's just a weird way to store a vq index.
> 
> Also \field{rss_rq_id} is confusing since it's actually \field{struct rss_rq_id}. We
> are using C-like not C++ like syntax ;)
> 
> One way to address all this:
> 
> \field{struct rss_rq_id} contains a virtqueue index: \field{vq_index_1_16}
> consists of bits 1 to 16 of a virtqueue index. For example, a
> \field{vq_index_1_16} value of 3 corresponds to virtqueue index 6, which maps
> to receiveq4.
> 
> Field \field{unclassified_queue} contains the virtqueue index of the receive
> virtqueue to place unclassified packets in, in \field{struct rss_rq_id} format.
> 
The data type of the unclassified_queue is struct rss_rq id, so we don't need to emphasis it again.

For example, we don't say, max_tx_vq is in le16 format.

I am going to keep the current version as it is better than then extra verbosity.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

[Michael S. Tsirkin]

On Mon, Apr 24, 2023 at 03:30:33PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, April 24, 2023 9:59 AM
> 
> > > >  Driver sends a VIRTIO_NET_CTRL_MQ_RSS_CONFIG command
> > 
> > maybe add:
> > 	including virtqueue index of relevant receive queues
> > 
> Structure struct virtio_net_rss_config has many fields for RSS_CONFIG command.
> It is not much help to say including receive queues, including indirection table etc.
> The structure is evident and all those fields are described in the struct including the receive virtqueue.

ok

> 
> > > >  struct virtio_net_rss_config {
> > > >      le32 hash_types;
> > > >      le16 indirection_table_mask;
> > > > -    le16 unclassified_queue;
> > > > -    le16 indirection_table[indirection_table_length];
> > > > +    struct rss_rq_id unclassified_queue;
> > > > +    struct rss_rq_id indirection_table[indirection_table_length];
> > > >      le16 max_tx_vq;
> > > >      u8 hash_key_length;
> > > >      u8 hash_key_data[hash_key_length]; @@ -1453,10 +1458,15 @@
> > > > \subsubsection{Control Virtqueue}\label{sec:Device Types / Network
> > > > Device / Devi  \field{indirection_table} array.
> > > >  Number of entries in \field{indirection_table} is
> > (\field{indirection_table_mask} + 1).
> > > >
> > > > -Field \field{unclassified_queue} contains the 0-based index of -the
> > > > receive virtqueue to place unclassified packets in. Index 0 corresponds to
> > receiveq1.
> > > > +\field{rss_rq_id} is a receive virtqueue id. \field{vq_index_1_16}
> > > > +consists of bits 1 to 16 of a virtqueue index. For example, a
> > > > +\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> > > > +which maps to receiveq4.
> > > > +
> > > > +Field \field{unclassified_queue} contains the receive virtqueue in
> > > > +which to place unclassified packets.
> > >
> > > It does not contain a receive virtqueue but its \field{rss_rq_id},
> > > i.e. it's "receive virtqueue id".
> > 
> receive virtqueue != virtqueue index.
> The receive virtqueue is description. The structure rss_rq_id is clear enough to indicate that how an receive virtqueue is communicated with the device.

the text originally was pretty clear though, the only problem
was format of index was unclear. your change me and Halil
both feel obscured things.

> > Yes all this last chunk is not an improvement.
> > My whole point was that we do not need a name for this thing that is struct
> > rss_rq_id. It's just a weird way to store a vq index.
> > 
> > Also \field{rss_rq_id} is confusing since it's actually \field{struct rss_rq_id}. We
> > are using C-like not C++ like syntax ;)
> > 
> > One way to address all this:
> > 
> > \field{struct rss_rq_id} contains a virtqueue index: \field{vq_index_1_16}
> > consists of bits 1 to 16 of a virtqueue index. For example, a
> > \field{vq_index_1_16} value of 3 corresponds to virtqueue index 6, which maps
> > to receiveq4.
> > 
> > Field \field{unclassified_queue} contains the virtqueue index of the receive
> > virtqueue to place unclassified packets in, in \field{struct rss_rq_id} format.
> > 
> The data type of the unclassified_queue is struct rss_rq id, so we don't need to emphasis it again.

oh, you can drop that:
	\field{struct rss_rq_id} contains a virtqueue index: \field{vq_index_1_16}
	consists of bits 1 to 16 of a virtqueue index. For example, a
	\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6, which maps
	to receiveq4.
	 
	Field \field{unclassified_queue} contains the virtqueue index of the receive
	virtqueue to place unclassified packets in.

but in any case, there is no point in saying "\field{rss_rq_id} is a
receive virtqueue id." This concept of "receive virtqueue id"
is not really useful.



> For example, we don't say, max_tx_vq is in le16 format.
> 
> I am going to keep the current version as it is better than then extra verbosity.

Maybe "specifies" instead of "contains" then?

+Field \field{unclassified_queue} specifies the receive virtqueue in
+which to place unclassified packets.



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v14 10/11] virtio-net: Describe RSS using rss rq id

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, April 24, 2023 11:54 AM

> > The data type of the unclassified_queue is struct rss_rq id, so we don't need
> to emphasis it again.
> 
> oh, you can drop that:
Bit 1 to 16 mechanics to be defined once in rss_rq_id definition.
I don't want to repeat it 3 times for unclassified q, indirection table and on device side.

So I didn't follow this comment about comment.

> 	\field{struct rss_rq_id} contains a virtqueue index:
> \field{vq_index_1_16}
> 	consists of bits 1 to 16 of a virtqueue index. For example, a
> 	\field{vq_index_1_16} value of 3 corresponds to virtqueue index 6,
> which maps
> 	to receiveq4.
> 
> 	Field \field{unclassified_queue} contains the virtqueue index of the
> receive
> 	virtqueue to place unclassified packets in.
> 
What is proposed is this patch is:

"Field unclassified_queue _contains_ the receive virtqueue in which to place unclassified packets."

This we can change to,

Field unclassified_queue _specifies_ the receive virtqueue in which to place unclassified packets.

> but in any case, there is no point in saying "\field{rss_rq_id} is a receive
> virtqueue id." This concept of "receive virtqueue id"
> is not really useful.
> 
Without repeating text 3 times rss_rq_id serve the purpose.

> 
> 
> > For example, we don't say, max_tx_vq is in le16 format.
> >
> > I am going to keep the current version as it is better than then extra
> verbosity.
> 
> Maybe "specifies" instead of "contains" then?
> 
> +Field \field{unclassified_queue} specifies the receive virtqueue in
> +which to place unclassified packets.
> 
Yes "specifies" is good. This helps to define the format also at one place.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v14 03/11] content: Rename confusing queue_notify_data and vqn names

[Parav Pandit]

> From: Halil Pasic <pasic@linux.ibm.com>
> Sent: Friday, April 21, 2023 6:38 PM
> >
> >  \begin{description}
> > -\item [vqn] VQ number to be notified.
> > +\item [vq_index_config_data] Either virtqueue index or device
> > +supplied
> 
> Apparently this our name for either index or notification config data, but I don't
> think we ever use it.
> 
Right because this is generic description without the field data type and their endianness and transport specific information.
So, it uses an abstract term.

> I guess, there is a two to think about it:
> * Notifications are mostly transport specific. In particular what
>   data structures are used for the virtqueue notifications and how
>   they are passed, that is entirely transport specific. So the 'layout'
>   and the normative statements describing the details should be in the
>   transport sections.
Right. This addition did not add any normative. They are in transport section as you say.

> * The facility bits and the capabilities are basically common. I.e. if
>   MMIO or/and Channel I/O were to ever support VIRTIO_F_NOTIF_CONFIG
> data
>   (by providing means for transporting the virtqueue notification config
>   data from the device to the driver), what you call the notification
>   value would be either virtqueue index or this notification config data.
> 
> You are taking the first approach here, and I'm fine with that. IMHO before we
> had the second approach implemented with vqn. I'm fine with taking the first
> approach as well, but your commit message says "rename ambiguous vqn to a
> union of vq_index and vq_config_data" which for me implies you actually
> intended to take the second approach.
> 
Those are renamed where the actual vqn is defined in the structure like below.
In basic description section, it is just describing the functionality.

> > @@ -1,5 +1,5 @@
> >  le32 {
> > -	vqn : 16;
> > +	vq_index: 16; /* previously known as vqn */
> >  	next_off : 15;
> >  	next_wrap : 1;
> >  };
> > diff --git a/notifications-pci-le.c b/notifications-pci-le.c new file
> > mode 100644 index 0000000..ef60d15
> > --- /dev/null
> > +++ b/notifications-pci-le.c
> > @@ -0,0 +1,8 @@
> > +le32 {
> > +	union {
> > +		vq_index: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not
> negotiated */
> > +		vq_notif_config_data: 16; /* Used if
> VIRTIO_F_NOTIF_CONFIG_DATA negotiated */
> > +	};
> > +	next_off : 15;
> > +	next_wrap : 1;
> > +};
> > diff --git a/transport-pci.tex b/transport-pci.tex index
> > 5d98467..04a9a80 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -319,7 +319,7 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> >          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_notif_config_data;   /* read-only for driver */
> >          le16 queue_reset;               /* read-write */
> >  };
> >  \end{lstlisting}
> > @@ -388,17 +388,21 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport  \item[\field{queue_device}]
> >          The driver writes the physical address of Device Area here.  See section
> \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
> >
> > -\item[\field{queue_notify_data}]
> > +\item[\field{queue_notif_config_data}]
> >          This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been
> negotiated.
> > -        The driver will use this value to put it in the 'virtqueue number' field
> > -        in the available buffer notification structure.
> > +        The driver will use this value when driver sends available buffer
> > +        notification to the device.
> >          See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-
> specific Initialization And Device Operation / Available Buffer Notifications}.
> >          \begin{note}
> >          This field provides the device with flexibility to determine how
> virtqueues
> >          will be referred to in available buffer notifications.
> > -        In a trivial case the device can set \field{queue_notify_data}=vqn. Some
> devices
> > -        may benefit from providing another value, for example an internal
> virtqueue
> > -        identifier, or an internal offset related to the virtqueue number.
> > +        In a trivial case the device can set \field{queue_notif_config_data} to
> > +        virtqueue index. Some devices may benefit from providing another
> value,
> > +        for example an internal virtqueue identifier, or an internal offset
> > +        related to the virtqueue index.
> > +        \end{note}
> > +        \begin{note}
> > +        This field is previously known as queue_notify_data.
> >          \end{note}
> >
> >  \item[\field{queue_reset}]
> > @@ -468,7 +472,9 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport
> >
> >  \drivernormative{\paragraph}{Common configuration structure
> > layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device
> > Layout / Common configuration structure layout}
> >
> > -The driver MUST NOT write to \field{device_feature}, \field{num_queues},
> \field{config_generation}, \field{queue_notify_off} or
> \field{queue_notify_data}.
> > +The driver MUST NOT write to \field{device_feature},
> > +\field{num_queues}, \field{config_generation},
> > +\field{queue_notify_off} or \field{queue_notif_config_data}.
> >
> >  If VIRTIO_F_RING_PACKED has been negotiated,  the driver MUST NOT
> > write the value 0 to \field{queue_size}.
> > @@ -1035,13 +1041,22 @@ \subsubsection{Available Buffer
> > Notifications}\label{sec:Virtio Transport Option
> >
> >  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,  the driver
> > sends an available buffer notification to the device by writing -the
> > 16-bit virtqueue index -of this virtqueue to the Queue Notify address.
> > +only the 16-bit notification value to the Queue Notify address of the
> 
> 
> You introduce the term notification value. Which basically means virtqueue
> index or virtqueue notification config data. 
Right and it is captured in the union fields.

> Which is basically what is defined
> where you define vq_index_config_data (the latter is never ever used again).
> 
vq_index_config_data is abstract term without any bit fields, width, endinenss and without any attachment to transport details.

> > +virtqueue. A notification value depends on the negotiation of
> > +VIRTIO_F_NOTIF_CONFIG_DATA.
> >
> > -When VIRTIO_F_NOTIFICATION_DATA has been negotiated, -the driver
> > sends an available buffer notification to the device by writing -the
> > following 32-bit value to the Queue Notify address:
> > -\lstinputlisting{notifications-le.c}
> > +\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
> > +sends an
> 
> The leading \item is probably not intended. Breaks the build and makes no
> sense.
Yes. it was not intended. Good catch.
Fixing in v15.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Parav Pandit]

> From: Halil Pasic <pasic@linux.ibm.com>
> Sent: Monday, April 24, 2023 9:29 AM
> 
> On Wed, 19 Apr 2023 04:46:32 +0300
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > Drop reference to first virtqueue as it is already covered now by the
> > generic section in first patch.
> >
> 
> TL;DR:
> 
> Acked-by: Halil Pasic <pasic@linux.ibm.com>
> 
> Rationale:
> ---------
> I agree, what a virtqueue index is should be described in a single place instead
> all over the place. And further improvements can be done on top of this series.
> 
> Discussion:
> -----------
> Is it indeed covered now by the generic section in first patch?
> 
> In [1] you state the following: "There is nothing like a zero based index. A VQ
> can be any u16 _number_ in range of 0 to 65534.
> Number can also start from zero. So zero based index = zero based number."
> 
> I don't really understand what do you mean by this, 
Like you described the vq index range is device specific and it is already documented in each device type.

A driver can choose to enable its "first VQ" located at vq index 2 for a specific device type.
So, for device the first VQ is vq index 2 instead of 0.

PCI spec wrote "first queue is 0", this was little misleading how driver and device see it.

So vq index starts from 0 but it need not be the first VQ, that's what I meant by above line and range description.

> but I'm afraid it is not
> consistent with my understanding. Can not be any number in range of 0 to
> 65534.
> 
> Rather in general it depends on the device type, and on the device how many
> queues it support. Let us call this number dev_vq_max. And if dev_vq_max > N
> > 0 index is associated with a queue the N-1 index is also associated with a
> queue.
> 
> Let me also note that the number of queues supported/provided by the device
> is a matter of the device. For many devices the number of queues is a constant
> given in the specification. A Network Device tells its driver how may queues it
> supports via max_virtqueue_pairs, Crypto Device via max_dataqueues, and
> Console Device via max_dataqueues.
> The transports PCI also has num_queues which is documented as "The device
> specifies the maximum number of virtqueues supported here." while CCW and
> MMIO have no transport specific means to tell the driver about the number of
> queues supported by the device. In any case, if the number of queues provided
> by the device is N, each of those queues is uniquely identified by exactly one
> index from the range [0..N-1]. Furthermore the role a certain queue plays is
> determined by its index. E.g. for the Console Device virtqueue identified by the
> index 3 is the "control receiveq".
> 
Sure. All you wrote is correct.

Thanks.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [virtio-comment] [PATCH v14 06/11] transport-mmio: Avoid referring to zero based index

[Halil Pasic]

On Wed, 19 Apr 2023 04:46:34 +0300
Parav Pandit <parav@nvidia.com> wrote:

> VQ range is already described in the first patch in basic virtqueue
> section. Hence remove the duplicate reference to it.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Acked-by: Halil Pasic <pasic@linux.ibm.com>

Same comments apply as for patch #4.

> 
> ---
> changelog:
> v12->v13:
> - corrected number to index
> v11->v12:
> - remove changes related to 'vq number'
> v8->v9:
> - added 'by' at two places
> - replaced 'queue number' with 'vq number'
> 
> v6->v7:
> - remove text around first vq as it is already covered in the basic
>   virtqueues facility section
> ---
>  transport-mmio.tex | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/transport-mmio.tex b/transport-mmio.tex
> index 164e640..2d24b4c 100644
> --- a/transport-mmio.tex
> +++ b/transport-mmio.tex
> @@ -113,8 +113,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>      following operations on \field{QueueSizeMax},
>      \field{QueueSize}, \field{QueueReady},
>      \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
> -    \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to. The index
> -    number of the first queue is zero (0x0).
> +    \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to.
>    }
>    \hline
>    \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
> @@ -363,8 +362,7 @@ \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / Vir
>  The driver will typically initialize the virtual queue in the following way:
>  
>  \begin{enumerate}
> -\item Select the queue writing its index (first queue is 0) to
> -   \field{QueueSel}.
> +\item Select the queue by writing its index to \field{QueueSel}.
>  
>  \item Check if the queue is not already in use: read \field{QueueReady},
>     and expect a returned value of zero (0x0).
> @@ -474,9 +472,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
>      Writing to this register selects the virtual queue that the
>      following operations on the \field{QueueSizeMax},
>      \field{QueueSize}, \field{QueueAlign}
> -    and \field{QueuePFN} registers apply to. The index
> -    number of the first queue is zero (0x0).
> -.
> +    and \field{QueuePFN} registers apply to.
>    }
>    \hline
>    \mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
> @@ -550,8 +546,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
>  
>  The virtual queue is configured as follows:
>  \begin{enumerate}
> -\item Select the queue writing its index (first queue is 0) to
> -   \field{QueueSel}.
> +\item Select the queue by writing its index to \field{QueueSel}.
>  
>  \item Check if the queue is not already in use: read \field{QueuePFN},
>     expecting a returned value of zero (0x0).


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] RE: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Halil Pasic]

On Tue, 25 Apr 2023 04:10:00 +0000
Parav Pandit <parav@nvidia.com> wrote:

> > From: Halil Pasic <pasic@linux.ibm.com>
> > Sent: Monday, April 24, 2023 9:29 AM
[..]
> > 
> > In [1] you state the following: "There is nothing like a zero based index. A VQ
> > can be any u16 _number_ in range of 0 to 65534.
> > Number can also start from zero. So zero based index = zero based number."
> > 
> > I don't really understand what do you mean by this,   
> Like you described the vq index range is device specific and it is already documented in each device type.
> 

Yes, but we don't actually spell it out. In virtio 1.1 "index" or
"virtqueue number" can be found in chapter 5 "Device Types".

Yes each device has a section "5.X.2 Virtqueues", but we don't spell it
out that the numbers 0, 1, 2 are actually virtqueue indexes.

> A driver can choose to enable its "first VQ" located at vq index 2 for a specific device type.
> So, for device the first VQ is vq index 2 instead of 0.

You read "first queue" as the queue that was "enabled" first. I tend to
agree, the queues can be "enabled" in any particular order. But I don't
agree that the queue that was "first enabled" is the what the spec
currently refers to as "first queue".

I don't really agree with your statement. For example, IMHO, it does not
work if the device has just one queue (like block, entropy) or just two
queues.

I'm convinced, for the device the first vq is always the queue with
index 0, if there is such a thing as "first vq for the device". I believe
the specification does not need "first vq for the device" and similar,
because what we actually need is "the queue with virtqueue index N".

> 
> PCI spec wrote "first queue is 0", this was little misleading how driver and device see it.
> 
> So vq index starts from 0 but it need not be the first VQ, that's what I meant by above line and range description.
> 


IMHO we should make sure the spec can not be read in this way. Because
if one was to choose the vq index 5 from the allowed range of [0..65535].
And try to "enable" the vq with index 5 as the first and only queue of
the virtio-blk device, and use it as such: well that would not work out
well.

> > but I'm afraid it is not
> > consistent with my understanding. Can not be any number in range of 0 to
> > 65534.
> > 
> > Rather in general it depends on the device type, and on the device how many
> > queues it support. Let us call this number dev_vq_max. And if dev_vq_max > N  
> > > 0 index is associated with a queue the N-1 index is also associated with a  
> > queue.
> > 
> > Let me also note that the number of queues supported/provided by the device
> > is a matter of the device. For many devices the number of queues is a constant
> > given in the specification. A Network Device tells its driver how may queues it
> > supports via max_virtqueue_pairs, Crypto Device via max_dataqueues, and
> > Console Device via max_dataqueues.
> > The transports PCI also has num_queues which is documented as "The device
> > specifies the maximum number of virtqueues supported here." while CCW and
> > MMIO have no transport specific means to tell the driver about the number of
> > queues supported by the device. In any case, if the number of queues provided
> > by the device is N, each of those queues is uniquely identified by exactly one
> > index from the range [0..N-1]. Furthermore the role a certain queue plays is
> > determined by its index. E.g. for the Console Device virtqueue identified by the
> > index 3 is the "control receiveq".
> >   
> Sure. All you wrote is correct.
> 

I'm happy we agree. All I say we may want to rewrite the 

"Each virtqueue is identified by a virtqueue index; virtqueue index
range is from 0 to 65535 inclusive."
as
"Each virtqueue is uniquely identified by a virtqueue index. The number
of supported virtqueues is device dependent, but can never exceed 65536.
Thus 16 bit is sufficient to represent virtqueue indexes. If the number
of virtqueues currently supported by some device is N, each of it is
virtqueues is uniquely identified by a single index from the range
[0..N-1]."

And it may be ester than doing a separate issue+ballot for it.

Regards,
Halil

> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] RE: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 03:02:15PM +0200, Halil Pasic wrote:
> On Tue, 25 Apr 2023 04:10:00 +0000
> Parav Pandit <parav@nvidia.com> wrote:
> 
> > > From: Halil Pasic <pasic@linux.ibm.com>
> > > Sent: Monday, April 24, 2023 9:29 AM
> [..]
> > > 
> > > In [1] you state the following: "There is nothing like a zero based index. A VQ
> > > can be any u16 _number_ in range of 0 to 65534.
> > > Number can also start from zero. So zero based index = zero based number."
> > > 
> > > I don't really understand what do you mean by this,   
> > Like you described the vq index range is device specific and it is already documented in each device type.
> > 
> 
> Yes, but we don't actually spell it out. In virtio 1.1 "index" or
> "virtqueue number" can be found in chapter 5 "Device Types".
> 
> Yes each device has a section "5.X.2 Virtqueues", but we don't spell it
> out that the numbers 0, 1, 2 are actually virtqueue indexes.
> 
> > A driver can choose to enable its "first VQ" located at vq index 2 for a specific device type.
> > So, for device the first VQ is vq index 2 instead of 0.
> 
> You read "first queue" as the queue that was "enabled" first. I tend to
> agree, the queues can be "enabled" in any particular order. But I don't
> agree that the queue that was "first enabled" is the what the spec
> currently refers to as "first queue".
> 
> I don't really agree with your statement. For example, IMHO, it does not
> work if the device has just one queue (like block, entropy) or just two
> queues.
> 
> I'm convinced, for the device the first vq is always the queue with
> index 0, if there is such a thing as "first vq for the device". I believe
> the specification does not need "first vq for the device" and similar,
> because what we actually need is "the queue with virtqueue index N".
> 
> > 
> > PCI spec wrote "first queue is 0", this was little misleading how driver and device see it.
> > 
> > So vq index starts from 0 but it need not be the first VQ, that's what I meant by above line and range description.
> > 
> 
> 
> IMHO we should make sure the spec can not be read in this way. Because
> if one was to choose the vq index 5 from the allowed range of [0..65535].
> And try to "enable" the vq with index 5 as the first and only queue of
> the virtio-blk device, and use it as such: well that would not work out
> well.
> 
> > > but I'm afraid it is not
> > > consistent with my understanding. Can not be any number in range of 0 to
> > > 65534.
> > > 
> > > Rather in general it depends on the device type, and on the device how many
> > > queues it support. Let us call this number dev_vq_max. And if dev_vq_max > N  
> > > > 0 index is associated with a queue the N-1 index is also associated with a  
> > > queue.
> > > 
> > > Let me also note that the number of queues supported/provided by the device
> > > is a matter of the device. For many devices the number of queues is a constant
> > > given in the specification. A Network Device tells its driver how may queues it
> > > supports via max_virtqueue_pairs, Crypto Device via max_dataqueues, and
> > > Console Device via max_dataqueues.
> > > The transports PCI also has num_queues which is documented as "The device
> > > specifies the maximum number of virtqueues supported here." while CCW and
> > > MMIO have no transport specific means to tell the driver about the number of
> > > queues supported by the device. In any case, if the number of queues provided
> > > by the device is N, each of those queues is uniquely identified by exactly one
> > > index from the range [0..N-1]. Furthermore the role a certain queue plays is
> > > determined by its index. E.g. for the Console Device virtqueue identified by the
> > > index 3 is the "control receiveq".
> > >   
> > Sure. All you wrote is correct.
> > 
> 
> I'm happy we agree. All I say we may want to rewrite the 
> 
> "Each virtqueue is identified by a virtqueue index; virtqueue index
> range is from 0 to 65535 inclusive."
> as
> "Each virtqueue is uniquely identified by a virtqueue index. The number
> of supported virtqueues is device dependent, but can never exceed 65536.
> Thus 16 bit is sufficient to represent virtqueue indexes. If the number
> of virtqueues currently supported by some device is N, each of it is
> virtqueues is uniquely identified by a single index from the range
> [0..N-1]."

Seems to be repeating same thing over and over.
This redundancy has cost, e.g. more places to change when we
talk about admin queues.
Yes it can not be any number 0 to 65535 but this kind of nitpicking
belongs in conformance statements not in general description.


> And it may be ester than doing a separate issue+ballot for it.
> 
> Regards,
> Halil
> 
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > 


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] RE: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Halil Pasic]

On Tue, 25 Apr 2023 09:15:14 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 25, 2023 at 03:02:15PM +0200, Halil Pasic wrote:
> > On Tue, 25 Apr 2023 04:10:00 +0000
> > Parav Pandit <parav@nvidia.com> wrote:
[..]
> > > Sure. All you wrote is correct.
> > >   
> > 
> > I'm happy we agree. All I say we may want to rewrite the 
> > 
> > "Each virtqueue is identified by a virtqueue index; virtqueue index
> > range is from 0 to 65535 inclusive."
> > as
> > "Each virtqueue is uniquely identified by a virtqueue index. The number
> > of supported virtqueues is device dependent, but can never exceed 65536.
> > Thus 16 bit is sufficient to represent virtqueue indexes. If the number
> > of virtqueues currently supported by some device is N, each of it is
> > virtqueues is uniquely identified by a single index from the range
> > [0..N-1]."  
> 
> Seems to be repeating same thing over and over.

Nod.

> This redundancy has cost, e.g. more places to change when we
> talk about admin queues.
> Yes it can not be any number 0 to 65535 but this kind of nitpicking
> belongs in conformance statements not in general description.
> 

I tend to agree. I would prefer to have it in some sort of conformance
statement, but I would also prefer to have it in exactly one place (and
not all over the place).

Regards,
Halil

[..]

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] RE: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 06:20:08PM +0200, Halil Pasic wrote:
> On Tue, 25 Apr 2023 09:15:14 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Apr 25, 2023 at 03:02:15PM +0200, Halil Pasic wrote:
> > > On Tue, 25 Apr 2023 04:10:00 +0000
> > > Parav Pandit <parav@nvidia.com> wrote:
> [..]
> > > > Sure. All you wrote is correct.
> > > >   
> > > 
> > > I'm happy we agree. All I say we may want to rewrite the 
> > > 
> > > "Each virtqueue is identified by a virtqueue index; virtqueue index
> > > range is from 0 to 65535 inclusive."
> > > as
> > > "Each virtqueue is uniquely identified by a virtqueue index. The number
> > > of supported virtqueues is device dependent, but can never exceed 65536.
> > > Thus 16 bit is sufficient to represent virtqueue indexes. If the number
> > > of virtqueues currently supported by some device is N, each of it is
> > > virtqueues is uniquely identified by a single index from the range
> > > [0..N-1]."  
> > 
> > Seems to be repeating same thing over and over.
> 
> Nod.
> 
> > This redundancy has cost, e.g. more places to change when we
> > talk about admin queues.
> > Yes it can not be any number 0 to 65535 but this kind of nitpicking
> > belongs in conformance statements not in general description.
> > 
> 
> I tend to agree. I would prefer to have it in some sort of conformance
> statement, but I would also prefer to have it in exactly one place (and
> not all over the place).
> 
> Regards,
> Halil
> 
> [..]

I don't feel there is much to say in one place. We did not in the past
specify what happens if drivers e.g. set queue select to a large number.
So some drivers might do just that for a variety of reasons.

In other words, given a queue there's an index
and i feel Parav's text explains what it is
pretty unambigously and also succintly.
I would even maybe drop the 0 to 65535 thing and just
say that it is 0 based.
What are the specific limitations on the index is already
explained elsewhere.


-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org