Rename queue number to queue index

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 Cornelia Huck <[email protected]>

Signed-off-by Parav Pandit <[email protected]>

---
changelog:
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 | 4 ++++
 1 file changed, 4 insertions(+)

content.tex +0 -0

index cff548a..e64115c 100644

content.tex +0 -0

 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

Signature · 1 line

2.26.2

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 1:26 AM

Quoted 33 lines · 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 Cornelia Huck <[email protected]>

Signed-off-by Parav Pandit <[email protected]>

---
changelog:
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 | 4 ++++
 1 file changed, 4 insertions(+)

content.tex +0 -0

@@ -298,6 +298,10 @@ \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.}.

content.tex

Quoted 3 lines

content.tex +3 -0

+Each virtqueue is identified by a vq index (also referred to as a
+virtqueue index); vq index range is from 0 to 65535 inclusive.
+

It's the other way around: start with full name, then abbreviation. Each virtqueue is identified by virtqueue index (sometimes abbreviated to vq index). A virtqueue index range ...

are there more abbreviations we want to mention here? vq_idx?

Quoted 6 lines

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

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 Cornelia Huck <[email protected]>

Signed-off-by Parav Pandit <[email protected]>

---
changelog:
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 | 4 ++++
 1 file changed, 4 insertions(+)

content.tex +0 -0

index cff548a..e64115c 100644

content.tex +0 -0

 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

Signature · 1 line

2.26.2

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:19 AM

In fact can we just say "virtqueue index" everywhere? The less slang reader has to learn, the better.

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:19 AM

In fact can we just say "virtqueue index" everywhere? The less slang reader has to learn, the better.

Wow, didn't know that "vq" is a slang. :)

From: Michael S. Tsirkin <[email protected]>
Sent: Friday, April 7, 2023 7:30 AM

Quoted 5 lines · Parav Pandit

Quoted 5 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:19 AM

Quoted 4 lines

Quoted 2 lines

In fact can we just say "virtqueue index" everywhere? The less slang reader has to learn, the better.

Wow, didn't know that "vq" is a slang. :)

And so is "virtqueue". It is certainly not in my dictionary of standard english. One term to learn is enough I think.

Quoted 13 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Friday, April 7, 2023 7:30 AM

Quoted 8 lines · Parav Pandit

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:19 AM

In fact can we just say "virtqueue index" everywhere? The less slang reader has to learn, the better.

Quoted 4 lines

Quoted 1 line

Wow, didn't know that "vq" is a slang. :)

And so is "virtqueue". It is certainly not in my dictionary of standard english. One term to learn is enough I think.

Sound good to me.
With above reasoning please replace all "vq" with "virtqueue" in the commit log and the spec area in aq series v12.

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 <[email protected]>

---
changelog:
v11->v12:
- new patch

---
 content.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Currently queue_notify_data register indicates the device internal queue notification identifier. 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).

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 vq identifier, rename it to queue_notify_id.

Reflect vq identifier in the driver notification structure by renaming ambiguous vqn to vq_notify_id.

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.

Signed-off-by Parav Pandit <[email protected]>

---
Some side notes:
renaming vqn to vqnd is even more confusing because data is really the queue identifier.

And NOTIFICATION_DATA really contains queue progress info (data).

(vqn - n is number or notification?, notification word in the notification structure does not make sense).

Hence above renaming.

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 1:22 AM

This is not much of an improvement.

Quoted 28 lines · Parav Pandit

Currently queue_notify_data register indicates the device internal queue notification identifier. 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).

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 vq identifier, rename it to queue_notify_id.

Reflect vq identifier in the driver notification structure by renaming ambiguous vqn to vq_notify_id.

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.

Signed-off-by Parav Pandit <[email protected]>

Quoted 4 lines

---
Some side notes:
renaming vqn to vqnd is even more confusing because data is really the queue identifier.

Clear to whom? Why do you think so? Marvell who pushed this feature said they stick some kind of constant value there which matches what their hardware expects. Sounds like a valid way to use this.

So no, not an identifier, and in any case "vq identifier" is a really general and useful term, I would rather not burn it up on a baroque feature that almost no one sets - for almost everyone else this is simply vq index.

If you really insist on renaming this away from "vqn", maybe vq_index_config_data will do, and we can add a comment /* Either vq index or vq config data, previously named vqn */

Quoted 41 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 1:22 AM

This is not much of an improvement.

Quoted 32 lines · Parav Pandit

Currently queue_notify_data register indicates the device internal queue notification identifier. 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).

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 vq identifier, rename it to queue_notify_id.

Reflect vq identifier in the driver notification structure by renaming ambiguous vqn to vq_notify_id.

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.

Signed-off-by Parav Pandit <[email protected]>

---
Some side notes:
renaming vqn to vqnd is even more confusing because data is really the queue identifier.

Quoted 3 lines

Clear to whom? Why do you think so? Marvell who pushed this feature said they stick some kind of constant value there which matches what their hardware expects. Sounds like a valid way to use this.

Spec currently has described it as "identifier".

Snippet:
"for example an internal virtqueue identifier, or an internal offset related to the virtqueue number"

Quoted 4 lines

So no, not an identifier, and in any case "vq identifier" is a really general and useful term, I would rather not burn it up on a baroque feature that almost no one sets - for almost everyone else this is simply vq index.

Quoted 4 lines

If you really insist on renaming this away from "vqn", maybe vq_index_config_data will do, and we can add a comment /* Either vq index or vq config data, previously named vqn */

Vq_index_config_data also aligns with the crazy feature bit name. So I will use that.

Currently queue_notify_data register indicates the device internal queue notification identifier. 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).

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 vq identifier, rename it to queue_notify_id.

Reflect vq identifier in the driver notification structure by renaming ambiguous vqn to vq_notify_id.

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.

Signed-off-by Parav Pandit <[email protected]>

---
Some side notes:
renaming vqn to vqnd is even more confusing because data is really the queue identifier.

And NOTIFICATION_DATA really contains queue progress info (data).

(vqn - n is number or notification?, notification word in the notification structure does not make sense).

Hence above renaming.

---
changelog:
v11->v12:
- new patch

---
 content.tex        | 17 ++++++++++++++---
 notifications-be.c |  2 +-
 notifications-le.c |  2 +-
 transport-pci.tex  | 24 ++++++++++++++----------
 4 files changed, 30 insertions(+), 15 deletions(-)

content.tex +5 -2

index cd93db2..d5f8026 100644
@@ -405,8 +405,18 @@ \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 involves sending only the 16-bit virtqueue notification
+identifier (notification method depends on the transport).
+
+\begin{itemize}
+\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated, virtqueue

content.tex +2 -1

@@ -417,7 +427,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_notify_id] VQ notification identifier of a
+      corresponding virtqueue.
 \item [next_off] Offset
       within the ring where the next available ring entry
       will be written.

notifications-be.c +1 -1

index 5be947e..0a7cbf1 100644
@@ -1,5 +1,5 @@
 be32 {
-	vqn : 16;
+	vq_notify_id : 16; /* previously known as vqn */
 	next_off : 15;
 	next_wrap : 1;
 };

notifications-le.c +1 -1

index fe51267..6cca8fb 100644
@@ -1,5 +1,5 @@
 le32 {
-	vqn : 16;
+	vq_notify_id : 16; /* previously known as vqn */
 	next_off : 15;
 	next_wrap : 1;
 };

transport-pci.tex +14 -10

index 5d98467..6bbf61c 100644
@@ -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_notify_id;           /* 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_notify_id}]
         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_notify_id} to
+        vq 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_id.
         \end{note}
 
 \item[\field{queue_reset}]

@@ -468,7 +472,7 @@ \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_notify_id}.
 
 If VIRTIO_F_RING_PACKED has been negotiated,
 the driver MUST NOT write the value 0 to \field{queue_size}.

@@ -1053,9 +1057,9 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
 If VIRTIO_F_NOTIF_CONFIG_DATA has been 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.
+\field{queue_notify_id} 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.
+\field{vq_notify_id} field to the \field{queue_notify_id} 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}

Signature · 1 line

2.26.2

Quoted 15 lines · Parav Pandit

content.tex +5 -2

index cd93db2..d5f8026 100644
@@ -405,8 +405,18 @@ \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 involves sending only the 16-bit virtqueue notification
+identifier (notification method depends on the transport).
+
+\begin{itemize}
+\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated, virtqueue

content.tex \section{Driver Notifications} \label{sec:Basic Facilities of a Virtio Device / · line 422

Quoted 10 lines

content.tex +7 -0

+notification identifier is a 16-bit vq index.
+
+\item When VIRTIO_F_NOTIF_CONFIG_DATA is negotiated, virtqueue
+notification identifier is a device supplied virtqueue identifier. A method
+to supply such virtqueue notification identifier is transport
+specific.
+\end{itemize}
 
 However, some devices benefit from the ability to find out the
 amount of available data in the queue without accessing the virtqueue in memory:

VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning "vq identifier" on this. How about we just say something along the lines of:

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this notification involves sending either the virtqueue index or the virtqueue config data to the device (method depending on the transport).

And then "the data sent is a device supplied virtqueue config data".

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 1:30 AM

VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning "vq identifier" on this. How about we just say something along the lines of:

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this notification involves sending either the virtqueue index or the virtqueue config data to the device (method depending on the transport).

And then "the data sent is a device supplied virtqueue config data".

Quoted 3 lines

VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning "vq identifier" on this. How about we just say something along the lines of:

Ok.

Quoted 1 line

Quoted 6 lines

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this notification involves sending either the virtqueue index or the virtqueue config data to the device (method depending on the transport).

And then "the data sent is a device supplied virtqueue config data".

Sounds fine. I will reword it.

From: Halil Pasic <[email protected]>
Sent: Wednesday, April 5, 2023 11:28 AM

Quoted 2 lines · On Wed, 5 Apr 2023 13:21:40 +0000 Parav Pandit <[email protected]> wrote:

Quoted 2 lines

VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning "vq identifier" on this. How about we just say something along the

lines of:

Quoted 7 lines

Quoted 1 line

Ok.

Quoted 5 lines

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this notification involves sending either the virtqueue index or the virtqueue config data to the device (method depending on the transport).

Quoted 3 lines

Quoted 2 lines

And then "the data sent is a device supplied virtqueue config data".

Sounds fine. I will reword it.

FYI in an other thread I proposed calling this a "cookie". Sorry for being late to the party. Yet again.

Quoted 18 lines

From: Halil Pasic <[email protected]>
Sent: Wednesday, April 5, 2023 11:28 AM

Quoted 2 lines · On Wed, 5 Apr 2023 13:21:40 +0000 Parav Pandit <[email protected]> wrote:

Quoted 2 lines

VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning "vq identifier" on this. How about we just say something along the

lines of:

Quoted 9 lines

Quoted 1 line

Ok.

Quoted 7 lines

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this notification involves sending either the virtqueue index or the virtqueue config data to the device (method depending on the transport).

And then "the data sent is a device supplied virtqueue config data".

Quoted 4 lines

Quoted 1 line

Sounds fine. I will reword it.

FYI in an other thread I proposed calling this a "cookie". Sorry for being late to the party. Yet again.

If we spend (waste) more time, we will find many examples where "identifier" and "cookie" both are used in things associated with computer science.

That too when same set of people has accepted text " internal virtqueue identifier" for same feature of CONFIG_DATA even though somehow it was not "id"!

And when this spec refers to an RFC of UUID, session id (not "session cookie", even though session id is opaque and not meaningful to the recipient as per Wikipedia usage desc that you listed).

For sure "cookie" is better than "config_data" and I don't have objection to "cookie".

But I disagree to the claim that "identifier" is less good than "cookie".

It is pointless debate of "identifier" vs "cookie".

The union format is very useful to describe this crisply, I will use it.

Do you prefer to rename F_CONFIG_DATA To F_CONFIG_COOKIE?

Quoted 4 lines · Parav Pandit

For sure "cookie" is better than "config_data" and I don't have objection to "cookie".

But I disagree to the claim that "identifier" is less good than "cookie".

Quoted 3 lines

It is pointless debate of "identifier" vs "cookie".

The union format is very useful to describe this crisply, I will use it.

I guess I'm fine with "cookie" in that in CS it is by now widely understood to mean "some opaque data".
identifier comes from "identical", so implies a 1:1 mapping IMO.

The logic behind using a cookie is that it's a bit similar to host cookie from ccw.
However, with ccw host cookie is used unconditionally, as opposed to depending on the flag.

Quoted 1 line

Do you prefer to rename F_CONFIG_DATA To F_CONFIG_COOKIE?

It should all be consistent, but I worry about ccw which uses cookies unconditionally. I am fine with leaving it as F_CONFIG_DATA for now unless we see a good way to avoid confusion for ccw.

From: Cornelia Huck <[email protected]>
Sent: Tuesday, April 11, 2023 4:56 AM

Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" there in the end (and F_CONFIG_COOKIE might indeed be confusing for the ccw case.)

Quoted 7 lines

From: Cornelia Huck <[email protected]>
Sent: Tuesday, April 11, 2023 4:56 AM

Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" there in the end (and F_CONFIG_COOKIE might indeed be confusing for the ccw case.)

Since Halil didn't respond for 5+ days + Michel and you propose to continue use CONFIG_DATA and this is rare used field, I will rename

vqn to union of

vq_index
vq_config_data

Thanks. Will roll v13.

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Subscribe: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Quoted 4 lines · On Tue, 11 Apr 2023 13:35:09 +0000 Parav Pandit <[email protected]> wrote:

Quoted 4 lines

From: Cornelia Huck <[email protected]>
Sent: Tuesday, April 11, 2023 4:56 AM

Quoted 6 lines

Quoted 3 lines

Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" there in the end (and F_CONFIG_COOKIE might indeed be confusing for the ccw case.)

Since Halil didn't respond for 5+ days + Michel and you propose to continue use CONFIG_DATA and this is rare used field, I will rename

Sorry, this one has fallen through the cracks.

For
the preceding ones: I do not have a strong opinion. I do share Michael's and Connie's assessment regarding a possible clash with CCW.

Let me just note that the feature ain't called, "F_CONFIG_DATA" (i.e. with full name VIRTIO_F_CONFIG_DATA) but rather "F_NOTIF_CONFIG_DATA" (i.e. with full name VIRTIO_F_NOTIF_CONFIG_DATA).

Quoted 4 lines

vqn to union of

vq_index
vq_config_data

In that sense vq_config_data is not good in my opinion, because it misses "notif" which is present in both "F_NOTIF_CONFIG_DATA" and "queue_notify_data".

Quoted 1 line

Quoted 2 lines

Thanks. Will roll v13.

I'm about tho have a look how this panned out in v13. I propose let us continue the discussion there.

Regards,
Halil

Quoted 16 lines

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Subscribe: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

I am not saying don't give feedback but I'm saying please help us all be more organized, feedback time really should be within a day or two, in rare cases up to a week.

And I'd like to remind everyone if you are going away you are supposed to report a leave of absence.

TC's that have meetings just take away voting rights from someone who does not attend two meetings in a row. We do it by ballot so this does not apply, but I think we should set some limits in group's bylaws, anyway. Ideas on formalizing this? If not we can just have informal guidelines. There's of course a flip side to this. Some patches seemingly go through two versions a day. Keeping up becomes a full time job. We'd need a guideline for that, too.

I also feel high latency is one of the reasons people are beginning to ask to split into subcommitees where they won't have to deal with this kind of thing. Let's try to keep the latency low, please.

I agree that patch reposting is happening much too fast in some cases. Not sure how to formalize that, either. Can we please just be more mindful of that? Reviewing time is not free. If I'm trying to do a timely review of something and constantly see new versions while I'm not finished yet, I do not feel like my feedback is actually valued.

Quoted 14 lines · Halil Pasic

Quoted 10 lines · On Tue, 11 Apr 2023 13:35:09 +0000 Parav Pandit <[email protected]> wrote:

Quoted 7 lines

From: Cornelia Huck <[email protected]>
Sent: Tuesday, April 11, 2023 4:56 AM

Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" there in the end (and F_CONFIG_COOKIE might indeed be confusing for the ccw case.)

Since Halil didn't respond for 5+ days + Michel and you propose to continue use CONFIG_DATA and this is rare used field, I will rename

Quoted 1 line

Sorry, this one has fallen through the cracks.

Well this whole patchset is just a cleanup so it's not holding up other work at least. But I have to say it's difficult to make progress when someone comes back from outer space after more than a week of silence while others finished a discussion and reopens it with some new feedback.

I also feel high latency is one of the reasons people are beginning to ask to split into subcommitees where they won't have to deal with this kind of thing.

Let's try to keep the latency low, please.

Quoted 4 lines

For
the preceding ones: I do not have a strong opinion. I do share Michael's and Connie's assessment regarding a possible clash with CCW.

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 1:30 AM

VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning "vq identifier" on this. How about we just say something along the lines of:

Ok.

Quoted 1 line

Quoted 6 lines

When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this notification involves sending either the virtqueue index or the virtqueue config data to the device (method depending on the transport).

And then "the data sent is a device supplied virtqueue config data".

Sounds fine. I will reword it.

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 <[email protected]>

---
changelog:
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(-)

device-types/net/description.tex +0 -0

index 435c1fc..4eb10d1 100644

device-types/net/description.tex +18 -7

+   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];

@@ -1442,10 +1447,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{vqn_1_16}
+consists of bits 1 to 16 of a vq index. For example, a
+\field{vqn_1_16} value of 3 corresponds to vq 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}).
 

@@ -1455,7 +1465,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.
 

@@ -1468,7 +1479,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}
 

Signature · 1 line

2.26.2

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:18 AM

following format for \field{command-specific-data}:

device-types/net/description.tex \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi · line 1427

Quoted 3 lines

\begin{lstlisting}

device-types/net/description.tex +2 -0

+struct rss_rq_id {
+   le16 vqn_1_16: 15; /* Bits 1 to 16 of the vq index */

Do you want to then call this vq_index_1_16 ?

And in the comment I would just write virtqueue index.

Quoted 10 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:18 AM

following format for \field{command-specific-data}:

Quoted 3 lines

\begin{lstlisting}

device-types/net/description.tex +2 -0

+struct rss_rq_id {
+   le16 vqn_1_16: 15; /* Bits 1 to 16 of the vq index */

Quoted 1 line

Do you want to then call this vq_index_1_16 ?

Yes. will change.

Quoted 2 lines

And in the comment I would just write virtqueue index.

Why do you prefer longer to name here instead of abbreviation?

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. Filed name vqn in the driver notification structure is ambiguous as it is supposed to hold either vq index or device supplied vq identifier.

4. Device is really supplying queue identifier 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_id
e. rename vqn to vq_notify_id 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_id 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

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:
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 (10):
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

 content.tex                      | 27 ++++++++++++----
 device-types/net/description.tex | 29 ++++++++++++-----
 notifications-be.c               |  2 +-
 notifications-le.c               |  2 +-
 transport-ccw.tex                | 15 +++++----
 transport-mmio.tex               | 55 +++++++++++++++++++-------------
 transport-pci.tex                | 26 ++++++++-------
 7 files changed, 99 insertions(+), 57 deletions(-)

Signature · 1 line

2.26.2

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 1:33 AM

I like where this is going. I will change AQ patches to be like this, but it's holidays here in israel so not reposting immediately, pls review assuming I will address that change.

For same reason pls don't expect quick review.

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. Filed name vqn in the driver notification structure is ambiguous as it is supposed to hold either vq index or device supplied vq identifier.

4. Device is really supplying queue identifier 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_id
e. rename vqn to vq_notify_id 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_id 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

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:
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 (10):
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

 content.tex                      | 27 ++++++++++++----
 device-types/net/description.tex | 29 ++++++++++++-----
 notifications-be.c               |  2 +-
 notifications-le.c               |  2 +-
 transport-ccw.tex                | 15 +++++----
 transport-mmio.tex               | 55 +++++++++++++++++++-------------
 transport-pci.tex                | 26 ++++++++-------
 7 files changed, 99 insertions(+), 57 deletions(-)

Signature · 1 line

2.26.2

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 5, 2023 5:21 AM

Quoted 1 line

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

I feel there are now two patchsets here:
1. renaming of queue num to queue size
2. virtqueue index and rss clarification depending on it

1 seems mostly ready, for 2 I don't yet know how long it will take. Split them up, also from POV of github issues?