Re: [virtio-comment] [PATCH v9] virtio-net: Add support for the flexible driver notification structure.

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

On Thu, 5 Nov 2020 22:41:42 +0000
Vitaly Mireyno <vmireyno@marvell.com> wrote:

> When the driver is required to send an available buffer notification to the device, it sends the virtqueue number to be notified.
> With this new feature, the device can optionally provide a per-virtqueue value for the driver to use in driver notifications, instead of the virtqueue number.
> Some devices may benefit from this flexibility by providing, for example, an internal virtqueue identifier, or an internal offset related to the virtqueue number.
> 
> Changes from v8:
>  * Incorporated comments for v8:
>      - moved the feature from a network device to a global section
>      - few minor changes
> 
> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> ---
>  content.tex | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 91735e3..bf50bfd 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -824,6 +824,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 */
>  };
>  \end{lstlisting}
>  
> @@ -890,6 +891,19 @@ \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}]
> +        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.
> +        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.
> +        \end{note}
>  \end{description}
>  
>  \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
> @@ -940,7 +954,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} or \field{queue_notify_off}.
> +The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
>  
>  If VIRTIO_F_RING_PACKED has been negotiated,
>  the driver MUST NOT write the value 0 to \field{queue_size}.
> @@ -1519,6 +1533,15 @@ \subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Option
>  See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
>  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}
> +\subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}

Whith your patch applied on top of current master I get something like this

"""
\subsubsection{Available Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}

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.

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}

See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
for the definition of the components.

See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
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}

\subsubsection{Used Buffer Notifications}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
"""

The device normative section and the preceding text are IMHO in
contradiction. The former says  the driver sends a 16-bit virtqueue
index unconditionally, the later says if if VIRTIO_F_NOTIFICATION_DATA
has been negotiated then use queue_notify_data, instead (thus
referencing the non-normative content).

IMHO we need a normative section that fully describes all the available
cases.

I think we can do this on top.

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/