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

[Halil Pasic <pasic@linux.ibm.com>]

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}