RE: [EXT] Re: [PATCH] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Vitaly Mireyno <vmireyno@marvell.com>]

>-----Original Message-----
>From: Michael S. Tsirkin <mst@redhat.com>
>Sent: Wednesday, 9 December, 2020 15:17
>To: Vitaly Mireyno <vmireyno@marvell.com>
>Cc: virtio-comment@lists.oasis-open.org; Ariel Elior <aelior@marvell.com>; Halil Pasic
><pasic@linux.ibm.com>; Cornelia Huck <cohuck@redhat.com>
>Subject: [EXT] Re: [PATCH] virtio-net: fix Driver Notification description related to
>VIRTIO_F_NOTIF_CONFIG_DATA
>
>External Email
>
>----------------------------------------------------------------------
>On Wed, Dec 09, 2020 at 12:57:08PM +0000, Vitaly Mireyno 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.
>>
>> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
>
>
>This is a bit repetitive.
>
>How about we replace "vqn" with "vqid", call it "virt queue identifier"
>and explain that it is either hardcoded to the virt queue index or read from config space.
>
>In config space replace queue_notify_data with queue_identifier.
>
>And maybe VIRTIO_F_NOTIF_CONFIG_DATA -> VIRTIO_F_NOTIF_CONFIG_QUEUE_ID
>

I think such definition is a little more restrictive as it suggests that the notification data is necessarily queue identifier, whereas the current definition allows a broader interpretation.

How about rephrasing "Driver Notifications" and "Available Buffer Notifications" sections to avoid repetitions? Something like:

@@ -337,9 +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 to the device either the virtqueue number
-or, if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated,
-the queue notification data (method depending on the transport).
+this notification involves sending to the device the vqn
+(method depending on the transport).
+\begin{description}
+\item [vqn] Either the virtqueue number to be notified or, if VIRTIO_F_NOTIF_CONFIG_DATA
+      has been negotiated, the queue notification data.
+\end{description}
 
 However, some devices benefit from the ability to find out the
 amount of available data in the queue without accessing the virtqueue in memory:
@@ -350,8 +353,7 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
 the following information:
 
 \begin{description}
-\item [vqn] Either the virtqueue number to be notified or, if VIRTIO_F_NOTIF_CONFIG_DATA
-      has been negotiated, the queue notification data.
+\item [vqn]
 \item [next_off] Offset
       within the ring where the next available ring entry
       will be written.
@@ -1533,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
-either the 16-bit virtqueue index of this virtqueue or, if VIRTIO_F_NOTIF_CONFIG_DATA
-has been negotiated, the 16-bit queue notification data to the Queue Notify address.
+the 16-bit vqn 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


>> ---
>>  content.tex | 21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/content.tex b/content.tex index 00bc050..9514b2d 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -337,8 +337,9 @@ \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 either the virtqueue
>> +number or, if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the
>> +queue notification data (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:
>> @@ -349,7 +350,8 @@ \section{Driver Notifications}
>> \label{sec:Virtqueues / Driver notifications}  the following information:
>>
>>  \begin{description}
>> -\item [vqn] VQ number to be notified.
>> +\item [vqn] Either the virtqueue number to be notified or, if VIRTIO_F_NOTIF_CONFIG_DATA
>> +      has been negotiated, the queue notification data.
>>  \item [next_off] Offset
>>        within the ring where the next available ring entry
>>        will be written.
>> @@ -1531,8 +1533,8 @@ \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.
>> +either the 16-bit virtqueue index of this virtqueue or, if
>> +VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the 16-bit queue notification data 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 +1548,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}
>>
>> --