[virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Vitaly Mireyno]

Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification structure".
Made Driver Notifications description more consistent throughout the document wrt VIRTIO_F_NOTIF_CONFIG_DATA.

Changes from v2:
 * Reworked 'vqn' name and definition


Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
---
 content.tex        | 30 ++++++++++++++----------------
 notifications-be.c |  2 +-
 notifications-le.c |  2 +-
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/content.tex b/content.tex
index 00bc050..19f09d9 100644
--- a/content.tex
+++ b/content.tex
@@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
 notification to the 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).
+this notification involves sending to the device the virtqueue token
+(method depending on the transport).
+if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue token is
+the virtqueue number to be notified.
+if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token is
+the queue notification data of the virtqueue to be notified.
 
 However, some devices benefit from the ability to find out the
 amount of available data in the queue without accessing the virtqueue in memory:
@@ -349,7 +353,7 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
 the following information:
 
 \begin{description}
-\item [vqn] VQ number to be notified.
+\item [vq_tok] Virtqueue token
 \item [next_off] Offset
       within the ring where the next available ring entry
       will be written.
@@ -906,14 +910,14 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 
 \item[\field{queue_notify_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
+        The driver will use this value to put it in the 'virtqueue token' field
         in the available buffer notification structure.
         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
+        In a trivial case the device can set \field{queue_notify_data}=virtqueue number.
+        Some devices may benefit from providing another value, for example an internal virtqueue
         identifier, or an internal offset related to the virtqueue number.
         \end{note}
 \end{description}
@@ -1531,8 +1535,7 @@ \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.
+the 16-bit virtqueue token to the Queue Notify address.
 
 When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
 the driver sends an available buffer notification to the device by writing
@@ -1546,13 +1549,8 @@ \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:
-\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.
-\end{itemize}
+If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the driver MUST use the
+\field{queue_notify_data} value as a queue notification data.
 
 \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
 
@@ -6579,7 +6577,7 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
 
   \item[VIRTIO_F_NOTIF_CONFIG_DATA(39)] This feature indicates that the driver
-  uses the data provided by the device as a virtqueue identifier in available
+  uses the data provided by the device as a virtqueue token in available
   buffer notifications.
   As mentioned in section \ref{sec:Virtqueues / Driver notifications}, when the
   driver is required to send an available buffer notification to the device, it
diff --git a/notifications-be.c b/notifications-be.c
index 5be947e..6d36ee5 100644
--- a/notifications-be.c
+++ b/notifications-be.c
@@ -1,5 +1,5 @@
 be32 {
-	vqn : 16;
+	vq_tok : 16;
 	next_off : 15;
 	next_wrap : 1;
 };
diff --git a/notifications-le.c b/notifications-le.c
index fe51267..b3fc284 100644
--- a/notifications-le.c
+++ b/notifications-le.c
@@ -1,5 +1,5 @@
 le32 {
-	vqn : 16;
+	vq_tok : 16;
 	next_off : 15;
 	next_wrap : 1;
 };
--


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

[virtio-comment] RE: [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Vitaly Mireyno]

Requesting a TC vote.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/94

>-----Original Message-----
>From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-open.org> On Behalf Of Vitaly
>Mireyno
>Sent: Thursday, 7 January, 2021 19:40
>To: virtio-comment@lists.oasis-open.org
>Cc: Michael S. Tsirkin <mst@redhat.com>; Ariel Elior <aelior@marvell.com>
>Subject: [EXT] [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to
>VIRTIO_F_NOTIF_CONFIG_DATA
>
>External Email
>
>----------------------------------------------------------------------
>Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification
>structure".
>Made Driver Notifications description more consistent throughout the document wrt
>VIRTIO_F_NOTIF_CONFIG_DATA.
>
>Changes from v2:
> * Reworked 'vqn' name and definition
>
>
>Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
>---
> content.tex        | 30 ++++++++++++++----------------
> notifications-be.c |  2 +-
> notifications-le.c |  2 +-
> 3 files changed, 16 insertions(+), 18 deletions(-)
>
>diff --git a/content.tex b/content.tex
>index 00bc050..19f09d9 100644
>--- a/content.tex
>+++ b/content.tex
>@@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
>notification to the 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).
>+this notification involves sending to the device the virtqueue token
>+(method depending on the transport).
>+if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue
>+token is the virtqueue number to be notified.
>+if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token
>+is the queue notification data of the virtqueue to be notified.
>
> However, some devices benefit from the ability to find out the  amount of available data in the queue
>without accessing the virtqueue in memory:
>@@ -349,7 +353,7 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}  the
>following information:
>
> \begin{description}
>-\item [vqn] VQ number to be notified.
>+\item [vq_tok] Virtqueue token
> \item [next_off] Offset
>       within the ring where the next available ring entry
>       will be written.
>@@ -906,14 +910,14 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio
>Transport
>
> \item[\field{queue_notify_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
>+        The driver will use this value to put it in the 'virtqueue
>+ token' field
>         in the available buffer notification structure.
>         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
>+        In a trivial case the device can set \field{queue_notify_data}=virtqueue number.
>+        Some devices may benefit from providing another value, for
>+ example an internal virtqueue
>         identifier, or an internal offset related to the virtqueue number.
>         \end{note}
> \end{description}
>@@ -1531,8 +1535,7 @@ \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.
>+the 16-bit virtqueue token to the Queue Notify address.
>
> When VIRTIO_F_NOTIFICATION_DATA has been negotiated,  the driver sends an available buffer
>notification to the device by writing @@ -1546,13 +1549,8 @@ \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:
>-\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.
>-\end{itemize}
>+If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the driver MUST use
>+the \field{queue_notify_data} value as a queue notification data.
>
> \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus /
>PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>
>@@ -6579,7 +6577,7 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>   See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
>
>   \item[VIRTIO_F_NOTIF_CONFIG_DATA(39)] This feature indicates that the driver
>-  uses the data provided by the device as a virtqueue identifier in available
>+  uses the data provided by the device as a virtqueue token in
>+ available
>   buffer notifications.
>   As mentioned in section \ref{sec:Virtqueues / Driver notifications}, when the
>   driver is required to send an available buffer notification to the device, it diff --git a/notifications-be.c
>b/notifications-be.c index 5be947e..6d36ee5 100644
>--- a/notifications-be.c
>+++ b/notifications-be.c
>@@ -1,5 +1,5 @@
> be32 {
>-	vqn : 16;
>+	vq_tok : 16;
> 	next_off : 15;
> 	next_wrap : 1;
> };
>diff --git a/notifications-le.c b/notifications-le.c index fe51267..b3fc284 100644
>--- a/notifications-le.c
>+++ b/notifications-le.c
>@@ -1,5 +1,5 @@
> le32 {
>-	vqn : 16;
>+	vq_tok : 16;
> 	next_off : 15;
> 	next_wrap : 1;
> };
>--
>
>
>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: virtio-comment-subscribe@lists.oasis-open.org
>Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>List help: virtio-comment-help@lists.oasis-open.org
>List archive: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.oasis-
>2Dopen.org_archives_virtio-
>2Dcomment_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4
>A_NO7xgI&m=YbUmR4uN5R9WCKqie8LhS15h2p2SuzbyWeizpJF3-7o&s=__l6Bm-
>zG_WSWuKbY0Jd7OC2Uy7GBbjXWqKmbvc7YiI&e=
>Feedback License: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-
>2Dopen.org_who_ipr_feedback-
>5Flicense.pdf&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4
>A_NO7xgI&m=YbUmR4uN5R9WCKqie8LhS15h2p2SuzbyWeizpJF3-
>7o&s=yqx6MgT05HzEg81pnO0A_Fx5y_FMmQdoe1vuYnTWxYU&e=
>List Guidelines: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-
>2Dopen.org_policies-2Dguidelines_mailing-
>2Dlists&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQAw4A_NO7
>xgI&m=YbUmR4uN5R9WCKqie8LhS15h2p2SuzbyWeizpJF3-
>7o&s=RFX2pm1IxfEzBra8L6qRDBK1QPTUdVIg8JQ_6RHmlmY&e=
>Committee: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-
>2Dopen.org_committees_virtio_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgF
>Rdcevq01tbLQAw4A_NO7xgI&m=YbUmR4uN5R9WCKqie8LhS15h2p2SuzbyWeizpJF3-
>7o&s=IjhDXVnx0roSwHBYk4mbRLEQLhF0MTVq8sM84uwznRQ&e=
>Join OASIS: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.oasis-
>2Dopen.org_join_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=lDHJ2FW52oJ3lqqsArgFRdcevq01tbLQ
>Aw4A_NO7xgI&m=YbUmR4uN5R9WCKqie8LhS15h2p2SuzbyWeizpJF3-7o&s=_CVScryuit_J0cJp6h-
>prVZll3o4raKZntvghQMwO38&e=


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

Re: [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Halil Pasic]

On Thu, 7 Jan 2021 17:39:48 +0000
Vitaly Mireyno <vmireyno@marvell.com> wrote:

> Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification structure".
> Made Driver Notifications description more consistent throughout the document wrt VIRTIO_F_NOTIF_CONFIG_DATA.
> 
> Changes from v2:
>  * Reworked 'vqn' name and definition
> 

First sorry for being late.

> 
> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> ---
>  content.tex        | 30 ++++++++++++++----------------
>  notifications-be.c |  2 +-
>  notifications-le.c |  2 +-
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 00bc050..19f09d9 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
>  notification to the 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).
> +this notification involves sending to the device the virtqueue token
> +(method depending on the transport).

I don't think changing the word order is a good idea. I would prefer
'this notification involves sending the virtqueue token to the device'

> +if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue token is

This is a new sentence. You should start it with a capital letter.

> +the virtqueue number to be notified

To be notified is off here. I believe it can be omitted, or we need
something like 'the virtqueue number of the virtqueue to be notified'

> +if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token is

This is probably supposed to be a new sentence as well, but you lack
both the period and the capital initial letter.

> +the queue notification data of the virtqueue to be notified.
>  

[..]

I will have a look at the rest tomorrow.

Regards,
Halil

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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

Re: [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Cornelia Huck]

On Wed, 3 Feb 2021 01:30:08 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 7 Jan 2021 17:39:48 +0000
> Vitaly Mireyno <vmireyno@marvell.com> wrote:
> 
> > Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification structure".
> > Made Driver Notifications description more consistent throughout the document wrt VIRTIO_F_NOTIF_CONFIG_DATA.
> > 
> > Changes from v2:
> >  * Reworked 'vqn' name and definition
> >   
> 
> First sorry for being late.

This had somehow fallen through the cracks on my side as well.

> 
> > 
> > Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> > ---
> >  content.tex        | 30 ++++++++++++++----------------
> >  notifications-be.c |  2 +-
> >  notifications-le.c |  2 +-
> >  3 files changed, 16 insertions(+), 18 deletions(-)
> > 
> > diff --git a/content.tex b/content.tex
> > index 00bc050..19f09d9 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
> >  notification to the 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).
> > +this notification involves sending to the device the virtqueue token
> > +(method depending on the transport).  
> 
> I don't think changing the word order is a good idea. I would prefer
> 'this notification involves sending the virtqueue token to the device'

Agreed.

> 
> > +if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue token is  
> 
> This is a new sentence. You should start it with a capital letter.

Nod.

> 
> > +the virtqueue number to be notified  
> 
> To be notified is off here. I believe it can be omitted, or we need
> something like 'the virtqueue number of the virtqueue to be notified'

"the number of the virtqueue to be notified" ?

> 
> > +if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token is  
> 
> This is probably supposed to be a new sentence as well, but you lack
> both the period and the capital initial letter.

Nod.

> 
> > +the queue notification data of the virtqueue to be notified.
> >    


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

Re: [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Michael S. Tsirkin]

On Wed, Feb 03, 2021 at 11:38:27AM +0100, Cornelia Huck wrote:
> On Wed, 3 Feb 2021 01:30:08 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Thu, 7 Jan 2021 17:39:48 +0000
> > Vitaly Mireyno <vmireyno@marvell.com> wrote:
> > 
> > > Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification structure".
> > > Made Driver Notifications description more consistent throughout the document wrt VIRTIO_F_NOTIF_CONFIG_DATA.
> > > 
> > > Changes from v2:
> > >  * Reworked 'vqn' name and definition
> > >   
> > 
> > First sorry for being late.
> 
> This had somehow fallen through the cracks on my side as well.

That's why we have the voting process :)

> > 
> > > 
> > > Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> > > ---
> > >  content.tex        | 30 ++++++++++++++----------------
> > >  notifications-be.c |  2 +-
> > >  notifications-le.c |  2 +-
> > >  3 files changed, 16 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 00bc050..19f09d9 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
> > >  notification to the 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).
> > > +this notification involves sending to the device the virtqueue token
> > > +(method depending on the transport).  
> > 
> > I don't think changing the word order is a good idea. I would prefer
> > 'this notification involves sending the virtqueue token to the device'
> 
> Agreed.
> 
> > 
> > > +if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue token is  
> > 
> > This is a new sentence. You should start it with a capital letter.
> 
> Nod.
> 
> > 
> > > +the virtqueue number to be notified  
> > 
> > To be notified is off here. I believe it can be omitted, or we need
> > something like 'the virtqueue number of the virtqueue to be notified'
> 
> "the number of the virtqueue to be notified" ?
> 
> > 
> > > +if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token is  
> > 
> > This is probably supposed to be a new sentence as well, but you lack
> > both the period and the capital initial letter.
> 
> Nod.
> 
> > 
> > > +the queue notification data of the virtqueue to be notified.
> > >    



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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

Re: [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Halil Pasic]

On Thu, 7 Jan 2021 17:39:48 +0000
Vitaly Mireyno <vmireyno@marvell.com> wrote:

> Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification structure".
> Made Driver Notifications description more consistent throughout the document wrt VIRTIO_F_NOTIF_CONFIG_DATA.
> 
> Changes from v2:
>  * Reworked 'vqn' name and definition
> 
> 
> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> ---
>  content.tex        | 30 ++++++++++++++----------------
>  notifications-be.c |  2 +-
>  notifications-le.c |  2 +-
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 00bc050..19f09d9 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
>  notification to the 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).

We should probably replace almost all, if not all occurrences of
'virtqueue number' with virtqueue index. Before 396b195
("VIRTIO_F_NOTIFICATION_DATA: extra data to devices") we had a single
occurrence of 'virtqueue number' (in the ccw transport) and 8 occurrences
of 'virtqueue index'. With commit 396b195 we tarted using virtqueue
number for virtqueue index. I find virtqueue index better, because I
think one could mistake virtqueue number for the number of virtqueues.

> +this notification involves sending to the device the virtqueue token
> +(method depending on the transport).
> +if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue token is
> +the virtqueue number to be notified.
> +if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token is
> +the queue notification data of the virtqueue to be notified.

The virtqueue token is confined to notifications, in a sense, that
for purposes other than notifications, we keep identifying the virtqueue
with it's virtqueue index, so maybe a 'virtqueue notification token'
would be an even better name.

Also how about rewording the above as:
The virtqueue [notification] token is defined as the virtqueue index if
VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, and as the value of the
\field{queue_notify_data} field if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated.

>  
>  However, some devices benefit from the ability to find out the
>  amount of available data in the queue without accessing the virtqueue in memory:
> @@ -349,7 +353,7 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
>  the following information:
>  
>  \begin{description}
> -\item [vqn] VQ number to be notified.
> +\item [vq_tok] Virtqueue token
>  \item [next_off] Offset
>        within the ring where the next available ring entry
>        will be written.
> @@ -906,14 +910,14 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  
>  \item[\field{queue_notify_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
> +        The driver will use this value to put it in the 'virtqueue token' field
>          in the available buffer notification structure.
>          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
> +        In a trivial case the device can set \field{queue_notify_data}=virtqueue number.
> +        Some devices may benefit from providing another value, for example an internal virtqueue
>          identifier, or an internal offset related to the virtqueue number.
>          \end{note}
>  \end{description}
> @@ -1531,8 +1535,7 @@ \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.
> +the 16-bit virtqueue token to the Queue Notify address.
>  
>  When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
>  the driver sends an available buffer notification to the device by writing

This stuff should IMHO be normative as well.

> @@ -1546,13 +1549,8 @@ \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:
> -\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.
> -\end{itemize}
> +If VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the driver MUST use the
> +\field{queue_notify_data} value as a queue notification data.
>  

With the virtqueue [notification] token, defined as is (i.e. it is
always defined, regardless of whether any of VIRTIO_F_NOTIF_CONFIG_DATA
and VIRTIO_F_NOTIFICATION_DATA was negotiated, the driver MUST always use
the virtqueue [notification] token. Or am I wrong?

Besides this being the only driver normative, does not make sense. The
driver normative should also contain the extra requirements that
correspond to  VIRTIO_F_NOTIFICATION_DATA. Probably even more. I would
have to re-read the other normative sections to have a founded opinion.

Regards,
Halil

>  \subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>  
> @@ -6579,7 +6577,7 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
>  
>    \item[VIRTIO_F_NOTIF_CONFIG_DATA(39)] This feature indicates that the driver
> -  uses the data provided by the device as a virtqueue identifier in available
> +  uses the data provided by the device as a virtqueue token in available
>    buffer notifications.
>    As mentioned in section \ref{sec:Virtqueues / Driver notifications}, when the
>    driver is required to send an available buffer notification to the device, it
> diff --git a/notifications-be.c b/notifications-be.c
> index 5be947e..6d36ee5 100644
> --- a/notifications-be.c
> +++ b/notifications-be.c
> @@ -1,5 +1,5 @@
>  be32 {
> -	vqn : 16;
> +	vq_tok : 16;
>  	next_off : 15;
>  	next_wrap : 1;
>  };
> diff --git a/notifications-le.c b/notifications-le.c
> index fe51267..b3fc284 100644
> --- a/notifications-le.c
> +++ b/notifications-le.c
> @@ -1,5 +1,5 @@
>  le32 {
> -	vqn : 16;
> +	vq_tok : 16;
>  	next_off : 15;
>  	next_wrap : 1;
>  };
> --
> 
> 
> 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: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> 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/
> 


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/