[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA
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/ >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]