[virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Michael S. Tsirkin]

Some devices benefit from ability to find out the number of available
descriptors in the ring: for efficiency or as a debugging aid.

To help with these optimizations, add a new feature:
VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
device include this extra information.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

v11 -> v12
	add missing 'the' article
	drop duplicate text sections
	use separate listing files for BE and LE, making
	format explicit.

v10 -> v11:
        drop mention of interrupts: current proposal does not include
        this interrupt related information
        drop unrelated introduction sections



 content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 notifications-be.c |   5 +++
 notifications-le.c |   5 +++
 3 files changed, 113 insertions(+), 6 deletions(-)
 create mode 100644 notifications-be.c
 create mode 100644 notifications-le.c

diff --git a/content.tex b/content.tex
index 7a92cb1..ae5fa4c 100644
--- a/content.tex
+++ b/content.tex
@@ -283,9 +283,49 @@ Packed Virtqueues}).
 Every driver and device supports either the Packed or the Split
 Virtqueue format, or both.
 
+\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}
+The driver is sometimes required to notify the device after
+making changes to the virtqueue.
+
+When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
+this notification involves sending the
+virtqueue number to the device (method depending on the transport).
+
+However, some devices benefit from the ability to find out the number of
+available descriptors in the ring without accessing the virtqueue in memory:
+for efficiency or as a debugging aid.
+
+To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
+has been negotiated, driver notifications to the device include
+the following information:
+
+\begin{description}
+\item [VQ number]
+\item [Offset]
+      Within the ring where the next available ring entry
+      will be written.
+      Without VIRTIO_F_RING_PACKED this refers to the
+      15 least significant bits of the available index.
+      With VIRTIO_F_RING_PACKED this refers to the offset
+      (in units of descriptor entries)
+      within the descriptor ring where the next available
+      descriptor will be written.
+\item [Wrap Counter]
+      With VIRTIO_F_RING_PACKED this is the wrap counter
+      referring to the next available descriptor.
+      Without VIRTIO_F_RING_PACKED this is the most significant bit
+      of the available index.
+\end{description}
+
+Note that the driver can trigger multiple notifications even without
+making any more changes to the ring. When VIRTIO_F_NOTIFICATION_DATA
+has been negotiated, these notifications would then have
+identical \field{Offset} and \field{Wrap Counter} values.
+
 \input{split-ring.tex}
 
 \input{packed-ring.tex}
+
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
 
 We start with an overview of device initialization, then expand on the
@@ -862,7 +902,9 @@ the same Queue Notify address for all queues.
 \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
 The device MUST present at least one notification capability.
 
-The \field{cap.offset} MUST be 2-byte aligned.  
+For devices not offering VIRTIO_F_NOTIFICATION_DATA:
+
+The \field{cap.offset} MUST be 2-byte aligned.
 
 The device MUST either present \field{notify_off_multiplier} as an even power of 2,
 or present \field{notify_off_multiplier} as 0.
@@ -876,6 +918,23 @@ For all queues, the value \field{cap.length} presented by the device MUST satisf
 cap.length >= queue_notify_off * notify_off_multiplier + 2
 \end{lstlisting}
 
+For devices offering VIRTIO_F_NOTIFICATION_DATA:
+
+The device MUST either present \field{notify_off_multiplier} as a
+number that is a power of 2 that is also a multiple 4,
+or present \field{notify_off_multiplier} as 0.
+
+The \field{cap.offset} MUST be 4-byte aligned.
+
+The value \field{cap.length} presented by the device MUST be at least 4
+and MUST be large enough to support queue notification offsets
+for all supported queues in all possible configurations.
+
+For all queues, the value \field{cap.length} presented by the device MUST satisfy:
+\begin{lstlisting}
+cap.length >= queue_notify_off * notify_off_multiplier + 4
+\end{lstlisting}
+
 \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
 
 The VIRTIO_PCI_CAP_ISR_CFG capability
@@ -1268,8 +1327,21 @@ separate cache lines.
 
 \subsubsection{Notifying The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notifying The Device}
 
-The driver notifies the device by writing the 16-bit virtqueue index
-of this virtqueue to the Queue Notify address.  See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} for how to calculate this address.
+When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
+the driver notifies the device by writing the 16-bit virtqueue index
+of this virtqueue (in little-endian byte order format)
+to the Queue Notify address.
+
+When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
+the driver notifies 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.
 
 \subsubsection{Virtqueue Interrupts From The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Virtqueue Interrupts From The Device}
 
@@ -1500,8 +1572,19 @@ All register values are organized as Little Endian.
   }
   \hline 
   \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
-    Writing a queue index to this register notifies the device that
-    there are new buffers to process in the queue.
+    Writing a value this register notifies the device that
+    there are new buffers to process in a queue.
+
+    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
+    the value written is the queue index.
+
+    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
+    the value has the following format:
+
+    \lstinputlisting{notifications-le.c}
+
+    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
+    for the definition of the components.
   }
   \hline 
   \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
@@ -2340,12 +2423,22 @@ GPR  &   Input Value     & Output Value \\
 \hline
   2   &  Subchannel ID    & Host Cookie  \\
 \hline
-  3   & Virtqueue number  &              \\
+  3   & Notification data &              \\
 \hline
   4   &   Host Cookie     &              \\
 \hline
 \end{tabular}
 
+When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
+the \field{Notification data} contains the Virtqueue number.
+
+When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
+the value has the following format:
+\lstinputlisting{notifications-be.c}
+
+See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
+for the definition of the components.
+
 \devicenormative{\paragraph}{Guest->Host Notification}{Virtio Transport Options / Virtio over channel I/O / Device Operation / Guest->Host Notification}
 The device MUST ignore bits 0-31 (counting from the left) of GPR2.
 This aligns passing the subchannel ID with the way it is passed
@@ -5348,6 +5441,10 @@ Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Supp
   \item[VIRTIO_F_IN_ORDER(35)] This feature indicates
   that all buffers are used by the device in the same
   order in which they have been made available.
+  \item[VIRTIO_F_NOTIFICATION_DATA(36)] This feature indicates
+  that drivers pass extra data (besides identifying the Virtqueue)
+  in their device notifications.
+  See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
diff --git a/notifications-be.c b/notifications-be.c
new file mode 100644
index 0000000..5be947e
--- /dev/null
+++ b/notifications-be.c
@@ -0,0 +1,5 @@
+be32 {
+	vqn : 16;
+	next_off : 15;
+	next_wrap : 1;
+};
diff --git a/notifications-le.c b/notifications-le.c
new file mode 100644
index 0000000..fe51267
--- /dev/null
+++ b/notifications-le.c
@@ -0,0 +1,5 @@
+le32 {
+	vqn : 16;
+	next_off : 15;
+	next_wrap : 1;
+};
-- 
MST

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Halil Pasic]

On 03/27/2018 06:37 PM, Michael S. Tsirkin wrote:
> Some devices benefit from ability to find out the number of available
> descriptors in the ring: for efficiency or as a debugging aid.
> 
> To help with these optimizations, add a new feature:
> VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
> device include this extra information.

I'm pondering about symmetry and about the nature of these
optimizations. By symmetry I mean this only works driver->device.
Why do we want this the one but not the other way around?

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> v11 -> v12
> 	add missing 'the' article
> 	drop duplicate text sections
> 	use separate listing files for BE and LE, making
> 	format explicit.
> 
> v10 -> v11:
>         drop mention of interrupts: current proposal does not include
>         this interrupt related information
>         drop unrelated introduction sections
> 
> 
> 
>  content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  notifications-be.c |   5 +++
>  notifications-le.c |   5 +++
>  3 files changed, 113 insertions(+), 6 deletions(-)
>  create mode 100644 notifications-be.c
>  create mode 100644 notifications-le.c
> 
> diff --git a/content.tex b/content.tex
> index 7a92cb1..ae5fa4c 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -283,9 +283,49 @@ Packed Virtqueues}).
>  Every driver and device supports either the Packed or the Split
>  Virtqueue format, or both.
> 
> +\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}

Don't we call the same 'device notification' in 2.5.12.4 Notifying The Device.
The wordings used to denote the different notifications seems to be
chaotic and maybe somewhat confusing overall.

Let me provide some examples:
For *virtqueue notification sent by the driver to the device* we use:
   * 'driver notifications' (here)
   * 'device notification' in 2.5.12.4 Notifying The Device)
   * 'virtqueue notification' (2.5.9 Virtqueue Notification Suppression)
   * 'guest -> host notification' (4.3.3.2 Guest->Host Notification)
The notifications *sent by the device to the driver (virtqueue or
configuration change)* are often referred to as *interrupts* but occasionally
also as notifications (e.g. 'host->guest notification').



> +The driver is sometimes required to notify the device after
> +making changes to the virtqueue.
> +

I don't like sometimes. I would prefer something like
'Under certain circumstances the driver is required issue
a virtqueue notification to the device. The method is transport
defined.'

> +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> +this notification involves sending the
> +virtqueue number to the device (method depending on the transport).

I don't like 'involves' here. I think it's supposed to tell
us that notification is a vitqueue level operation. Moreover
the point is  when VIRTIO_F_NOTIFICATION_DATA has not been negotiated
it's a virtueue notification and nothing more.

> +
> +However, some devices benefit from the ability to find out the number of
> +available descriptors in the ring without accessing the virtqueue in memory:

What is 'the ring'? (probably descriptor ring or available ring depending
on what do we have)

> +for efficiency or as a debugging aid.
> +
> +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
> +has been negotiated, driver notifications to the device include
> +the following information:
> +
> +\begin{description}
> +\item [VQ number]
> +\item [Offset]
> +      Within the ring where the next available ring entry
> +      will be written.
> +      Without VIRTIO_F_RING_PACKED this refers to the
> +      15 least significant bits of the available index.
> +      With VIRTIO_F_RING_PACKED this refers to the offset
> +      (in units of descriptor entries)

What does '(in units of descriptor entries)' mean? Is it a
mod ring size index? If it is, why not call it index consequently
(instead of this offset-index-offset thing)?

> +      within the descriptor ring where the next available
> +      descriptor will be written.
> +\item [Wrap Counter]
> +      With VIRTIO_F_RING_PACKED this is the wrap counter
> +      referring to the next available descriptor.
> +      Without VIRTIO_F_RING_PACKED this is the most significant bit
> +      of the available index.
> +\end{description}
> +
> +Note that the driver can trigger multiple notifications even without
> +making any more changes to the ring. When VIRTIO_F_NOTIFICATION_DATA
> +has been negotiated, these notifications would then have
> +identical \field{Offset} and \field{Wrap Counter} values.
> +
>  \input{split-ring.tex}
> 
>  \input{packed-ring.tex}
> +
>  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> 
>  We start with an overview of device initialization, then expand on the
> @@ -862,7 +902,9 @@ the same Queue Notify address for all queues.
>  \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
>  The device MUST present at least one notification capability.
> 
> -The \field{cap.offset} MUST be 2-byte aligned.  
> +For devices not offering VIRTIO_F_NOTIFICATION_DATA:
> +
> +The \field{cap.offset} MUST be 2-byte aligned.
> 
>  The device MUST either present \field{notify_off_multiplier} as an even power of 2,
>  or present \field{notify_off_multiplier} as 0.
> @@ -876,6 +918,23 @@ For all queues, the value \field{cap.length} presented by the device MUST satisf
>  cap.length >= queue_notify_off * notify_off_multiplier + 2
>  \end{lstlisting}
> 
> +For devices offering VIRTIO_F_NOTIFICATION_DATA:
> +
> +The device MUST either present \field{notify_off_multiplier} as a
> +number that is a power of 2 that is also a multiple 4,
> +or present \field{notify_off_multiplier} as 0.
> +
> +The \field{cap.offset} MUST be 4-byte aligned.
> +
> +The value \field{cap.length} presented by the device MUST be at least 4
> +and MUST be large enough to support queue notification offsets
> +for all supported queues in all possible configurations.
> +
> +For all queues, the value \field{cap.length} presented by the device MUST satisfy:
> +\begin{lstlisting}
> +cap.length >= queue_notify_off * notify_off_multiplier + 4
> +\end{lstlisting}
> +
>  \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
> 
>  The VIRTIO_PCI_CAP_ISR_CFG capability
> @@ -1268,8 +1327,21 @@ separate cache lines.
> 
>  \subsubsection{Notifying The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notifying The Device}
> 
> -The driver notifies the device by writing the 16-bit virtqueue index
> -of this virtqueue to the Queue Notify address.  See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} for how to calculate this address.
> +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> +the driver notifies the device by writing the 16-bit virtqueue index
> +of this virtqueue (in little-endian byte order format)
> +to the Queue Notify address.
> +
> +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> +the driver notifies 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.
> 
>  \subsubsection{Virtqueue Interrupts From The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Virtqueue Interrupts From The Device}
> 
> @@ -1500,8 +1572,19 @@ All register values are organized as Little Endian.
>    }
>    \hline 
>    \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
> -    Writing a queue index to this register notifies the device that
> -    there are new buffers to process in the queue.
> +    Writing a value this register notifies the device that
> +    there are new buffers to process in a queue.
> +
> +    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> +    the value written is the queue index.
> +
> +    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> +    the value has the following format:
> +
> +    \lstinputlisting{notifications-le.c}
> +
> +    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> +    for the definition of the components.
>    }
>    \hline 
>    \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
> @@ -2340,12 +2423,22 @@ GPR  &   Input Value     & Output Value \\
>  \hline
>    2   &  Subchannel ID    & Host Cookie  \\
>  \hline
> -  3   & Virtqueue number  &              \\
> +  3   & Notification data &              \\
>  \hline
>    4   &   Host Cookie     &              \\
>  \hline
>  \end{tabular}
> 
> +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> +the \field{Notification data} contains the Virtqueue number.
> +
> +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> +the value has the following format:
> +\lstinputlisting{notifications-be.c}
> +
> +See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> +for the definition of the components.
> +
>  \devicenormative{\paragraph}{Guest->Host Notification}{Virtio Transport Options / Virtio over channel I/O / Device Operation / Guest->Host Notification}
>  The device MUST ignore bits 0-31 (counting from the left) of GPR2.
>  This aligns passing the subchannel ID with the way it is passed
> @@ -5348,6 +5441,10 @@ Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Supp
>    \item[VIRTIO_F_IN_ORDER(35)] This feature indicates
>    that all buffers are used by the device in the same
>    order in which they have been made available.
> +  \item[VIRTIO_F_NOTIFICATION_DATA(36)] This feature indicates
> +  that drivers pass extra data (besides identifying the Virtqueue)

s/drivers/driver/ plural seems inappropriate

> +  in their device notifications.

I'm not really satisfied with this description but have nothing
better to offer, so I'm willing to accept.


> +  See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
>  \end{description}
> 
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> diff --git a/notifications-be.c b/notifications-be.c
> new file mode 100644
> index 0000000..5be947e
> --- /dev/null
> +++ b/notifications-be.c
> @@ -0,0 +1,5 @@
> +be32 {
> +	vqn : 16;
> +	next_off : 15;
> +	next_wrap : 1;

Would it make sense to switch next_wrap and next_off for be.
I mean that way we should be able to read the avail_idx at
once (I think) instead of having to compute it first from
next_off and next_wrap.

Again, I can't really tell what is behind yet. Thus it may
be completely irrelevant.


Regards,
Halil

> +};
> diff --git a/notifications-le.c b/notifications-le.c
> new file mode 100644
> index 0000000..fe51267
> --- /dev/null
> +++ b/notifications-le.c
> @@ -0,0 +1,5 @@
> +le32 {
> +	vqn : 16;
> +	next_off : 15;
> +	next_wrap : 1;
> +};
> 


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Michael S. Tsirkin]

On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:
> 
> 
> On 03/27/2018 06:37 PM, Michael S. Tsirkin wrote:
> > Some devices benefit from ability to find out the number of available
> > descriptors in the ring: for efficiency or as a debugging aid.
> > 
> > To help with these optimizations, add a new feature:
> > VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
> > device include this extra information.
> 
> I'm pondering about symmetry and about the nature of these
> optimizations. By symmetry I mean this only works driver->device.
> Why do we want this the one but not the other way around?

Because
- memory is already closer to CPU than to devices
- there is no way to pass extra data with an interrupt


> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > v11 -> v12
> > 	add missing 'the' article
> > 	drop duplicate text sections
> > 	use separate listing files for BE and LE, making
> > 	format explicit.
> > 
> > v10 -> v11:
> >         drop mention of interrupts: current proposal does not include
> >         this interrupt related information
> >         drop unrelated introduction sections
> > 
> > 
> > 
> >  content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  notifications-be.c |   5 +++
> >  notifications-le.c |   5 +++
> >  3 files changed, 113 insertions(+), 6 deletions(-)
> >  create mode 100644 notifications-be.c
> >  create mode 100644 notifications-le.c
> > 
> > diff --git a/content.tex b/content.tex
> > index 7a92cb1..ae5fa4c 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -283,9 +283,49 @@ Packed Virtqueues}).
> >  Every driver and device supports either the Packed or the Split
> >  Virtqueue format, or both.
> > 
> > +\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}
> 
> Don't we call the same 'device notification' in 2.5.12.4 Notifying The Device.

Oh right.

> The wordings used to denote the different notifications seems to be
> chaotic and maybe somewhat confusing overall.
> 
> Let me provide some examples:
> For *virtqueue notification sent by the driver to the device* we use:
>    * 'driver notifications' (here)

We should fix this I think.

>    * 'device notification' in 2.5.12.4 Notifying The Device)
>    * 'virtqueue notification' (2.5.9 Virtqueue Notification Suppression)

Let's change these to 'virtqueue device notifications'?
It's a separate issue though, not introduced by this patch.

>    * 'guest -> host notification' (4.3.3.2 Guest->Host Notification)

That's a transport thing specific to virtio over MMIO.

> The notifications *sent by the device to the driver (virtqueue or
> configuration change)* are often referred to as *interrupts* but occasionally
> also as notifications (e.g. 'host->guest notification').
> 
> 
> 
> > +The driver is sometimes required to notify the device after
> > +making changes to the virtqueue.
> > +
> 
> I don't like sometimes. I would prefer something like
> 'Under certain circumstances the driver is required issue
> a virtqueue notification to the device.


Under certain circumstances just says sometimes in 3 words.
Can't we find a way to say it that does not bump the word count x3?

> The method is transport
> defined.'

That's good to add.

> > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > +this notification involves sending the
> > +virtqueue number to the device (method depending on the transport).
> 
> I don't like 'involves' here. I think it's supposed to tell
> us that notification is a vitqueue level operation. Moreover
> the point is  when VIRTIO_F_NOTIFICATION_DATA has not been negotiated
> it's a virtueue notification and nothing more.
> 
> > +
> > +However, some devices benefit from the ability to find out the number of
> > +available descriptors in the ring without accessing the virtqueue in memory:
> 
> What is 'the ring'? (probably descriptor ring or available ring depending
> on what do we have)

Maybe buffers in the virtqueue?

> > +for efficiency or as a debugging aid.
> > +
> > +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
> > +has been negotiated, driver notifications to the device include
> > +the following information:
> > +
> > +\begin{description}
> > +\item [VQ number]
> > +\item [Offset]
> > +      Within the ring where the next available ring entry
> > +      will be written.
> > +      Without VIRTIO_F_RING_PACKED this refers to the
> > +      15 least significant bits of the available index.
> > +      With VIRTIO_F_RING_PACKED this refers to the offset
> > +      (in units of descriptor entries)
> 
> What does '(in units of descriptor entries)' mean?

That it's not an address in bytes.
Offset is a distance right?
Within ring implies from start of ring.
What is left is to tell what are the units.

> Is it a
> mod ring size index?

I don't see how specifying units can be taken
to mean it's modulo some value :(

> If it is, why not call it index consequently
> (instead of this offset-index-offset thing)?

In fact index in many places is *not* mod ring size.
For me offset means where it is. Just need to specify
the units.


> > +      within the descriptor ring where the next available
> > +      descriptor will be written.
> > +\item [Wrap Counter]
> > +      With VIRTIO_F_RING_PACKED this is the wrap counter
> > +      referring to the next available descriptor.
> > +      Without VIRTIO_F_RING_PACKED this is the most significant bit
> > +      of the available index.
> > +\end{description}
> > +
> > +Note that the driver can trigger multiple notifications even without
> > +making any more changes to the ring. When VIRTIO_F_NOTIFICATION_DATA
> > +has been negotiated, these notifications would then have
> > +identical \field{Offset} and \field{Wrap Counter} values.
> > +
> >  \input{split-ring.tex}
> > 
> >  \input{packed-ring.tex}
> > +
> >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > 
> >  We start with an overview of device initialization, then expand on the
> > @@ -862,7 +902,9 @@ the same Queue Notify address for all queues.
> >  \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
> >  The device MUST present at least one notification capability.
> > 
> > -The \field{cap.offset} MUST be 2-byte aligned.  
> > +For devices not offering VIRTIO_F_NOTIFICATION_DATA:
> > +
> > +The \field{cap.offset} MUST be 2-byte aligned.
> > 
> >  The device MUST either present \field{notify_off_multiplier} as an even power of 2,
> >  or present \field{notify_off_multiplier} as 0.
> > @@ -876,6 +918,23 @@ For all queues, the value \field{cap.length} presented by the device MUST satisf
> >  cap.length >= queue_notify_off * notify_off_multiplier + 2
> >  \end{lstlisting}
> > 
> > +For devices offering VIRTIO_F_NOTIFICATION_DATA:
> > +
> > +The device MUST either present \field{notify_off_multiplier} as a
> > +number that is a power of 2 that is also a multiple 4,
> > +or present \field{notify_off_multiplier} as 0.
> > +
> > +The \field{cap.offset} MUST be 4-byte aligned.
> > +
> > +The value \field{cap.length} presented by the device MUST be at least 4
> > +and MUST be large enough to support queue notification offsets
> > +for all supported queues in all possible configurations.
> > +
> > +For all queues, the value \field{cap.length} presented by the device MUST satisfy:
> > +\begin{lstlisting}
> > +cap.length >= queue_notify_off * notify_off_multiplier + 4
> > +\end{lstlisting}
> > +
> >  \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
> > 
> >  The VIRTIO_PCI_CAP_ISR_CFG capability
> > @@ -1268,8 +1327,21 @@ separate cache lines.
> > 
> >  \subsubsection{Notifying The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notifying The Device}
> > 
> > -The driver notifies the device by writing the 16-bit virtqueue index
> > -of this virtqueue to the Queue Notify address.  See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} for how to calculate this address.
> > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > +the driver notifies the device by writing the 16-bit virtqueue index
> > +of this virtqueue (in little-endian byte order format)
> > +to the Queue Notify address.
> > +
> > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > +the driver notifies 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.
> > 
> >  \subsubsection{Virtqueue Interrupts From The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Virtqueue Interrupts From The Device}
> > 
> > @@ -1500,8 +1572,19 @@ All register values are organized as Little Endian.
> >    }
> >    \hline 
> >    \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
> > -    Writing a queue index to this register notifies the device that
> > -    there are new buffers to process in the queue.
> > +    Writing a value this register notifies the device that
> > +    there are new buffers to process in a queue.
> > +
> > +    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > +    the value written is the queue index.
> > +
> > +    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > +    the value has the following format:
> > +
> > +    \lstinputlisting{notifications-le.c}
> > +
> > +    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> > +    for the definition of the components.
> >    }
> >    \hline 
> >    \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
> > @@ -2340,12 +2423,22 @@ GPR  &   Input Value     & Output Value \\
> >  \hline
> >    2   &  Subchannel ID    & Host Cookie  \\
> >  \hline
> > -  3   & Virtqueue number  &              \\
> > +  3   & Notification data &              \\
> >  \hline
> >    4   &   Host Cookie     &              \\
> >  \hline
> >  \end{tabular}
> > 
> > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > +the \field{Notification data} contains the Virtqueue number.
> > +
> > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > +the value has the following format:
> > +\lstinputlisting{notifications-be.c}
> > +
> > +See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> > +for the definition of the components.
> > +
> >  \devicenormative{\paragraph}{Guest->Host Notification}{Virtio Transport Options / Virtio over channel I/O / Device Operation / Guest->Host Notification}
> >  The device MUST ignore bits 0-31 (counting from the left) of GPR2.
> >  This aligns passing the subchannel ID with the way it is passed
> > @@ -5348,6 +5441,10 @@ Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Supp
> >    \item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> >    that all buffers are used by the device in the same
> >    order in which they have been made available.
> > +  \item[VIRTIO_F_NOTIFICATION_DATA(36)] This feature indicates
> > +  that drivers pass extra data (besides identifying the Virtqueue)
> 
> s/drivers/driver/ plural seems inappropriate
> 
> > +  in their device notifications.
> 
> I'm not really satisfied with this description but have nothing
> better to offer, so I'm willing to accept.
> 
> 
> > +  See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
> >  \end{description}
> > 
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > diff --git a/notifications-be.c b/notifications-be.c
> > new file mode 100644
> > index 0000000..5be947e
> > --- /dev/null
> > +++ b/notifications-be.c
> > @@ -0,0 +1,5 @@
> > +be32 {
> > +	vqn : 16;
> > +	next_off : 15;
> > +	next_wrap : 1;
> 
> Would it make sense to switch next_wrap and next_off for be.
> I mean that way we should be able to read the avail_idx at
> once (I think) instead of having to compute it first from
> next_off and next_wrap.

Makes sense.

> Again, I can't really tell what is behind yet. Thus it may
> be completely irrelevant.
> 
> 
> Regards,
> Halil
> 
> > +};
> > diff --git a/notifications-le.c b/notifications-le.c
> > new file mode 100644
> > index 0000000..fe51267
> > --- /dev/null
> > +++ b/notifications-le.c
> > @@ -0,0 +1,5 @@
> > +le32 {
> > +	vqn : 16;
> > +	next_off : 15;
> > +	next_wrap : 1;
> > +};
> > 

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio] on spec clarifications

[Michael S. Tsirkin]

Halil, 
in the past you mentioned that you see some parts of the specification
as unclear.

Will you have the time to look into this and send patches, or even just
open github issues?

I'd like us to start looking at wrapping up the next spec version.

-- 
MST

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Cornelia Huck]

On Wed, 28 Mar 2018 20:21:47 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:
> > 
> > 
> > On 03/27/2018 06:37 PM, Michael S. Tsirkin wrote:  
> > > Some devices benefit from ability to find out the number of available
> > > descriptors in the ring: for efficiency or as a debugging aid.
> > > 
> > > To help with these optimizations, add a new feature:
> > > VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
> > > device include this extra information.  
> > 
> > I'm pondering about symmetry and about the nature of these
> > optimizations. By symmetry I mean this only works driver->device.
> > Why do we want this the one but not the other way around?  
> 
> Because
> - memory is already closer to CPU than to devices
> - there is no way to pass extra data with an interrupt
> 
> 
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > 
> > > v11 -> v12
> > > 	add missing 'the' article
> > > 	drop duplicate text sections
> > > 	use separate listing files for BE and LE, making
> > > 	format explicit.
> > > 
> > > v10 -> v11:
> > >         drop mention of interrupts: current proposal does not include
> > >         this interrupt related information
> > >         drop unrelated introduction sections
> > > 
> > > 
> > > 
> > >  content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  notifications-be.c |   5 +++
> > >  notifications-le.c |   5 +++
> > >  3 files changed, 113 insertions(+), 6 deletions(-)
> > >  create mode 100644 notifications-be.c
> > >  create mode 100644 notifications-le.c
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 7a92cb1..ae5fa4c 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -283,9 +283,49 @@ Packed Virtqueues}).
> > >  Every driver and device supports either the Packed or the Split
> > >  Virtqueue format, or both.
> > > 
> > > +\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}  
> > 
> > Don't we call the same 'device notification' in 2.5.12.4 Notifying The Device.  
> 
> Oh right.
> 
> > The wordings used to denote the different notifications seems to be
> > chaotic and maybe somewhat confusing overall.
> > 
> > Let me provide some examples:
> > For *virtqueue notification sent by the driver to the device* we use:
> >    * 'driver notifications' (here)  
> 
> We should fix this I think.
> 
> >    * 'device notification' in 2.5.12.4 Notifying The Device)
> >    * 'virtqueue notification' (2.5.9 Virtqueue Notification Suppression)  
> 
> Let's change these to 'virtqueue device notifications'?
> It's a separate issue though, not introduced by this patch.
> 
> >    * 'guest -> host notification' (4.3.3.2 Guest->Host Notification)  
> 
> That's a transport thing specific to virtio over MMIO.

ccw, right?

I called these 'guest->host' (and 'host->guest') because it was more
easily understandable at the time. It would be better to call them
'device notification' and 'driver notification' to align with the
wording elsewhere. (As a separate patch.)

> 
> > The notifications *sent by the device to the driver (virtqueue or
> > configuration change)* are often referred to as *interrupts* but occasionally
> > also as notifications (e.g. 'host->guest notification').

I think 'notifications' is a better term for these. I'm thinking of a
driver polling for outstanding notifications from the device, no
interrupts involved.

But we can discuss that later.

> > 
> > 
> >   
> > > +The driver is sometimes required to notify the device after
> > > +making changes to the virtqueue.
> > > +  
> > 
> > I don't like sometimes. I would prefer something like
> > 'Under certain circumstances the driver is required issue
> > a virtqueue notification to the device.  
> 
> 
> Under certain circumstances just says sometimes in 3 words.
> Can't we find a way to say it that does not bump the word count x3?

What's wrong with "Sometimes, the driver is required to issue..."?

> 
> > The method is transport
> > defined.'  
> 
> That's good to add.

Agreed.

> 
> > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > +this notification involves sending the
> > > +virtqueue number to the device (method depending on the transport).  
> > 
> > I don't like 'involves' here. I think it's supposed to tell
> > us that notification is a vitqueue level operation. Moreover
> > the point is  when VIRTIO_F_NOTIFICATION_DATA has not been negotiated
> > it's a virtueue notification and nothing more.

What about

"When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver
notifies the device about the affected virtqueue only."

?

> >   
> > > +
> > > +However, some devices benefit from the ability to find out the number of
> > > +available descriptors in the ring without accessing the virtqueue in memory:  
> > 
> > What is 'the ring'? (probably descriptor ring or available ring depending
> > on what do we have)  
> 
> Maybe buffers in the virtqueue?

Works for me.

> 
> > > +for efficiency or as a debugging aid.
> > > +
> > > +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
> > > +has been negotiated, driver notifications to the device include
> > > +the following information:
> > > +
> > > +\begin{description}
> > > +\item [VQ number]
> > > +\item [Offset]
> > > +      Within the ring where the next available ring entry
> > > +      will be written.
> > > +      Without VIRTIO_F_RING_PACKED this refers to the
> > > +      15 least significant bits of the available index.
> > > +      With VIRTIO_F_RING_PACKED this refers to the offset
> > > +      (in units of descriptor entries)  
> > 
> > What does '(in units of descriptor entries)' mean?  
> 
> That it's not an address in bytes.
> Offset is a distance right?
> Within ring implies from start of ring.
> What is left is to tell what are the units.
> 
> > Is it a
> > mod ring size index?  
> 
> I don't see how specifying units can be taken
> to mean it's modulo some value :(
> 
> > If it is, why not call it index consequently
> > (instead of this offset-index-offset thing)?  
> 
> In fact index in many places is *not* mod ring size.
> For me offset means where it is. Just need to specify
> the units.

I agree, and I think the wording is fine here.

> 
> 
> > > +      within the descriptor ring where the next available
> > > +      descriptor will be written.
> > > +\item [Wrap Counter]
> > > +      With VIRTIO_F_RING_PACKED this is the wrap counter
> > > +      referring to the next available descriptor.
> > > +      Without VIRTIO_F_RING_PACKED this is the most significant bit
> > > +      of the available index.
> > > +\end{description}
> > > +
> > > +Note that the driver can trigger multiple notifications even without
> > > +making any more changes to the ring. When VIRTIO_F_NOTIFICATION_DATA
> > > +has been negotiated, these notifications would then have
> > > +identical \field{Offset} and \field{Wrap Counter} values.
> > > +
> > >  \input{split-ring.tex}
> > > 
> > >  \input{packed-ring.tex}
> > > +
> > >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > > 
> > >  We start with an overview of device initialization, then expand on the
> > > @@ -862,7 +902,9 @@ the same Queue Notify address for all queues.
> > >  \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
> > >  The device MUST present at least one notification capability.
> > > 
> > > -The \field{cap.offset} MUST be 2-byte aligned.  
> > > +For devices not offering VIRTIO_F_NOTIFICATION_DATA:
> > > +
> > > +The \field{cap.offset} MUST be 2-byte aligned.
> > > 
> > >  The device MUST either present \field{notify_off_multiplier} as an even power of 2,
> > >  or present \field{notify_off_multiplier} as 0.
> > > @@ -876,6 +918,23 @@ For all queues, the value \field{cap.length} presented by the device MUST satisf
> > >  cap.length >= queue_notify_off * notify_off_multiplier + 2
> > >  \end{lstlisting}
> > > 
> > > +For devices offering VIRTIO_F_NOTIFICATION_DATA:
> > > +
> > > +The device MUST either present \field{notify_off_multiplier} as a
> > > +number that is a power of 2 that is also a multiple 4,
> > > +or present \field{notify_off_multiplier} as 0.
> > > +
> > > +The \field{cap.offset} MUST be 4-byte aligned.
> > > +
> > > +The value \field{cap.length} presented by the device MUST be at least 4
> > > +and MUST be large enough to support queue notification offsets
> > > +for all supported queues in all possible configurations.
> > > +
> > > +For all queues, the value \field{cap.length} presented by the device MUST satisfy:
> > > +\begin{lstlisting}
> > > +cap.length >= queue_notify_off * notify_off_multiplier + 4
> > > +\end{lstlisting}
> > > +
> > >  \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
> > > 
> > >  The VIRTIO_PCI_CAP_ISR_CFG capability
> > > @@ -1268,8 +1327,21 @@ separate cache lines.
> > > 
> > >  \subsubsection{Notifying The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notifying The Device}
> > > 
> > > -The driver notifies the device by writing the 16-bit virtqueue index
> > > -of this virtqueue to the Queue Notify address.  See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} for how to calculate this address.
> > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > +the driver notifies the device by writing the 16-bit virtqueue index
> > > +of this virtqueue (in little-endian byte order format)
> > > +to the Queue Notify address.
> > > +
> > > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > +the driver notifies 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.
> > > 
> > >  \subsubsection{Virtqueue Interrupts From The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Virtqueue Interrupts From The Device}
> > > 
> > > @@ -1500,8 +1572,19 @@ All register values are organized as Little Endian.
> > >    }
> > >    \hline 
> > >    \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
> > > -    Writing a queue index to this register notifies the device that
> > > -    there are new buffers to process in the queue.
> > > +    Writing a value this register notifies the device that
> > > +    there are new buffers to process in a queue.
> > > +
> > > +    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > +    the value written is the queue index.
> > > +
> > > +    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > +    the value has the following format:
> > > +
> > > +    \lstinputlisting{notifications-le.c}
> > > +
> > > +    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> > > +    for the definition of the components.
> > >    }
> > >    \hline 
> > >    \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
> > > @@ -2340,12 +2423,22 @@ GPR  &   Input Value     & Output Value \\
> > >  \hline
> > >    2   &  Subchannel ID    & Host Cookie  \\
> > >  \hline
> > > -  3   & Virtqueue number  &              \\
> > > +  3   & Notification data &              \\
> > >  \hline
> > >    4   &   Host Cookie     &              \\
> > >  \hline
> > >  \end{tabular}
> > > 
> > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > +the \field{Notification data} contains the Virtqueue number.
> > > +
> > > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > +the value has the following format:
> > > +\lstinputlisting{notifications-be.c}
> > > +
> > > +See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> > > +for the definition of the components.
> > > +
> > >  \devicenormative{\paragraph}{Guest->Host Notification}{Virtio Transport Options / Virtio over channel I/O / Device Operation / Guest->Host Notification}
> > >  The device MUST ignore bits 0-31 (counting from the left) of GPR2.
> > >  This aligns passing the subchannel ID with the way it is passed
> > > @@ -5348,6 +5441,10 @@ Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Supp
> > >    \item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> > >    that all buffers are used by the device in the same
> > >    order in which they have been made available.
> > > +  \item[VIRTIO_F_NOTIFICATION_DATA(36)] This feature indicates
> > > +  that drivers pass extra data (besides identifying the Virtqueue)  
> > 
> > s/drivers/driver/ plural seems inappropriate
> >   
> > > +  in their device notifications.  

As a feature is negotiated per-device anyway, agreed.

"This feature indicates that the driver passes extra data (besides
identifying the virtqueue) in its device notifications."

> > 
> > I'm not really satisfied with this description but have nothing
> > better to offer, so I'm willing to accept.
> > 
> >   
> > > +  See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
> > >  \end{description}
> > > 
> > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > > diff --git a/notifications-be.c b/notifications-be.c
> > > new file mode 100644
> > > index 0000000..5be947e
> > > --- /dev/null
> > > +++ b/notifications-be.c
> > > @@ -0,0 +1,5 @@
> > > +be32 {
> > > +	vqn : 16;
> > > +	next_off : 15;
> > > +	next_wrap : 1;  
> > 
> > Would it make sense to switch next_wrap and next_off for be.
> > I mean that way we should be able to read the avail_idx at
> > once (I think) instead of having to compute it first from
> > next_off and next_wrap.  
> 
> Makes sense.

Nod.

> 
> > Again, I can't really tell what is behind yet. Thus it may
> > be completely irrelevant.
> > 
> > 
> > Regards,
> > Halil
> >   
> > > +};
> > > diff --git a/notifications-le.c b/notifications-le.c
> > > new file mode 100644
> > > index 0000000..fe51267
> > > --- /dev/null
> > > +++ b/notifications-le.c
> > > @@ -0,0 +1,5 @@
> > > +le32 {
> > > +	vqn : 16;
> > > +	next_off : 15;
> > > +	next_wrap : 1;
> > > +};
> > >   


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Michael S. Tsirkin]

On Thu, Mar 29, 2018 at 11:05:55AM +0200, Cornelia Huck wrote:
> On Wed, 28 Mar 2018 20:21:47 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:
> > > 
> > > 
> > > On 03/27/2018 06:37 PM, Michael S. Tsirkin wrote:  
> > > > Some devices benefit from ability to find out the number of available
> > > > descriptors in the ring: for efficiency or as a debugging aid.
> > > > 
> > > > To help with these optimizations, add a new feature:
> > > > VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
> > > > device include this extra information.  
> > > 
> > > I'm pondering about symmetry and about the nature of these
> > > optimizations. By symmetry I mean this only works driver->device.
> > > Why do we want this the one but not the other way around?  
> > 
> > Because
> > - memory is already closer to CPU than to devices
> > - there is no way to pass extra data with an interrupt
> > 
> > 
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > 
> > > > v11 -> v12
> > > > 	add missing 'the' article
> > > > 	drop duplicate text sections
> > > > 	use separate listing files for BE and LE, making
> > > > 	format explicit.
> > > > 
> > > > v10 -> v11:
> > > >         drop mention of interrupts: current proposal does not include
> > > >         this interrupt related information
> > > >         drop unrelated introduction sections
> > > > 
> > > > 
> > > > 
> > > >  content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > >  notifications-be.c |   5 +++
> > > >  notifications-le.c |   5 +++
> > > >  3 files changed, 113 insertions(+), 6 deletions(-)
> > > >  create mode 100644 notifications-be.c
> > > >  create mode 100644 notifications-le.c
> > > > 
> > > > diff --git a/content.tex b/content.tex
> > > > index 7a92cb1..ae5fa4c 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -283,9 +283,49 @@ Packed Virtqueues}).
> > > >  Every driver and device supports either the Packed or the Split
> > > >  Virtqueue format, or both.
> > > > 
> > > > +\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}  
> > > 
> > > Don't we call the same 'device notification' in 2.5.12.4 Notifying The Device.  
> > 
> > Oh right.
> > 
> > > The wordings used to denote the different notifications seems to be
> > > chaotic and maybe somewhat confusing overall.
> > > 
> > > Let me provide some examples:
> > > For *virtqueue notification sent by the driver to the device* we use:
> > >    * 'driver notifications' (here)  
> > 
> > We should fix this I think.
> > 
> > >    * 'device notification' in 2.5.12.4 Notifying The Device)
> > >    * 'virtqueue notification' (2.5.9 Virtqueue Notification Suppression)  
> > 
> > Let's change these to 'virtqueue device notifications'?
> > It's a separate issue though, not introduced by this patch.
> > 
> > >    * 'guest -> host notification' (4.3.3.2 Guest->Host Notification)  
> > 
> > That's a transport thing specific to virtio over MMIO.
> 
> ccw, right?
> 
> I called these 'guest->host' (and 'host->guest') because it was more
> easily understandable at the time. It would be better to call them
> 'device notification' and 'driver notification' to align with the
> wording elsewhere. (As a separate patch.)

ccw is different is that there's always a host, so there is
some sense in using host and guest terminology.

> > 
> > > The notifications *sent by the device to the driver (virtqueue or
> > > configuration change)* are often referred to as *interrupts* but occasionally
> > > also as notifications (e.g. 'host->guest notification').
> 
> I think 'notifications' is a better term for these. I'm thinking of a
> driver polling for outstanding notifications from the device, no
> interrupts involved.

In this context when polling there are no notifications - rememeber we are
talking about notification suppression.

> But we can discuss that later.
> 
> > > 
> > > 
> > >   
> > > > +The driver is sometimes required to notify the device after
> > > > +making changes to the virtqueue.
> > > > +  
> > > 
> > > I don't like sometimes. I would prefer something like
> > > 'Under certain circumstances the driver is required issue
> > > a virtqueue notification to the device.  
> > 
> > 
> > Under certain circumstances just says sometimes in 3 words.
> > Can't we find a way to say it that does not bump the word count x3?
> 
> What's wrong with "Sometimes, the driver is required to issue..."?

Fine.

> > 
> > > The method is transport
> > > defined.'  
> > 
> > That's good to add.
> 
> Agreed.
> 
> > 
> > > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > > +this notification involves sending the
> > > > +virtqueue number to the device (method depending on the transport).  
> > > 
> > > I don't like 'involves' here. I think it's supposed to tell
> > > us that notification is a vitqueue level operation. Moreover
> > > the point is  when VIRTIO_F_NOTIFICATION_DATA has not been negotiated
> > > it's a virtueue notification and nothing more.
> 
> What about
> 
> "When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver
> notifies the device about the affected virtqueue only."
> 
> ?
> 
> > >   
> > > > +
> > > > +However, some devices benefit from the ability to find out the number of
> > > > +available descriptors in the ring without accessing the virtqueue in memory:  
> > > 
> > > What is 'the ring'? (probably descriptor ring or available ring depending
> > > on what do we have)  
> > 
> > Maybe buffers in the virtqueue?
> 
> Works for me.
> 
> > 
> > > > +for efficiency or as a debugging aid.
> > > > +
> > > > +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
> > > > +has been negotiated, driver notifications to the device include
> > > > +the following information:
> > > > +
> > > > +\begin{description}
> > > > +\item [VQ number]
> > > > +\item [Offset]
> > > > +      Within the ring where the next available ring entry
> > > > +      will be written.
> > > > +      Without VIRTIO_F_RING_PACKED this refers to the
> > > > +      15 least significant bits of the available index.
> > > > +      With VIRTIO_F_RING_PACKED this refers to the offset
> > > > +      (in units of descriptor entries)  
> > > 
> > > What does '(in units of descriptor entries)' mean?  
> > 
> > That it's not an address in bytes.
> > Offset is a distance right?
> > Within ring implies from start of ring.
> > What is left is to tell what are the units.
> > 
> > > Is it a
> > > mod ring size index?  
> > 
> > I don't see how specifying units can be taken
> > to mean it's modulo some value :(
> > 
> > > If it is, why not call it index consequently
> > > (instead of this offset-index-offset thing)?  
> > 
> > In fact index in many places is *not* mod ring size.
> > For me offset means where it is. Just need to specify
> > the units.
> 
> I agree, and I think the wording is fine here.
> 
> > 
> > 
> > > > +      within the descriptor ring where the next available
> > > > +      descriptor will be written.
> > > > +\item [Wrap Counter]
> > > > +      With VIRTIO_F_RING_PACKED this is the wrap counter
> > > > +      referring to the next available descriptor.
> > > > +      Without VIRTIO_F_RING_PACKED this is the most significant bit
> > > > +      of the available index.
> > > > +\end{description}
> > > > +
> > > > +Note that the driver can trigger multiple notifications even without
> > > > +making any more changes to the ring. When VIRTIO_F_NOTIFICATION_DATA
> > > > +has been negotiated, these notifications would then have
> > > > +identical \field{Offset} and \field{Wrap Counter} values.
> > > > +
> > > >  \input{split-ring.tex}
> > > > 
> > > >  \input{packed-ring.tex}
> > > > +
> > > >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > > > 
> > > >  We start with an overview of device initialization, then expand on the
> > > > @@ -862,7 +902,9 @@ the same Queue Notify address for all queues.
> > > >  \devicenormative{\paragraph}{Notification capability}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
> > > >  The device MUST present at least one notification capability.
> > > > 
> > > > -The \field{cap.offset} MUST be 2-byte aligned.  
> > > > +For devices not offering VIRTIO_F_NOTIFICATION_DATA:
> > > > +
> > > > +The \field{cap.offset} MUST be 2-byte aligned.
> > > > 
> > > >  The device MUST either present \field{notify_off_multiplier} as an even power of 2,
> > > >  or present \field{notify_off_multiplier} as 0.
> > > > @@ -876,6 +918,23 @@ For all queues, the value \field{cap.length} presented by the device MUST satisf
> > > >  cap.length >= queue_notify_off * notify_off_multiplier + 2
> > > >  \end{lstlisting}
> > > > 
> > > > +For devices offering VIRTIO_F_NOTIFICATION_DATA:
> > > > +
> > > > +The device MUST either present \field{notify_off_multiplier} as a
> > > > +number that is a power of 2 that is also a multiple 4,
> > > > +or present \field{notify_off_multiplier} as 0.
> > > > +
> > > > +The \field{cap.offset} MUST be 4-byte aligned.
> > > > +
> > > > +The value \field{cap.length} presented by the device MUST be at least 4
> > > > +and MUST be large enough to support queue notification offsets
> > > > +for all supported queues in all possible configurations.
> > > > +
> > > > +For all queues, the value \field{cap.length} presented by the device MUST satisfy:
> > > > +\begin{lstlisting}
> > > > +cap.length >= queue_notify_off * notify_off_multiplier + 4
> > > > +\end{lstlisting}
> > > > +
> > > >  \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
> > > > 
> > > >  The VIRTIO_PCI_CAP_ISR_CFG capability
> > > > @@ -1268,8 +1327,21 @@ separate cache lines.
> > > > 
> > > >  \subsubsection{Notifying The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notifying The Device}
> > > > 
> > > > -The driver notifies the device by writing the 16-bit virtqueue index
> > > > -of this virtqueue to the Queue Notify address.  See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} for how to calculate this address.
> > > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > > +the driver notifies the device by writing the 16-bit virtqueue index
> > > > +of this virtqueue (in little-endian byte order format)
> > > > +to the Queue Notify address.
> > > > +
> > > > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > > +the driver notifies 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.
> > > > 
> > > >  \subsubsection{Virtqueue Interrupts From The Device}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Virtqueue Interrupts From The Device}
> > > > 
> > > > @@ -1500,8 +1572,19 @@ All register values are organized as Little Endian.
> > > >    }
> > > >    \hline 
> > > >    \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
> > > > -    Writing a queue index to this register notifies the device that
> > > > -    there are new buffers to process in the queue.
> > > > +    Writing a value this register notifies the device that
> > > > +    there are new buffers to process in a queue.
> > > > +
> > > > +    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > > +    the value written is the queue index.
> > > > +
> > > > +    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > > +    the value has the following format:
> > > > +
> > > > +    \lstinputlisting{notifications-le.c}
> > > > +
> > > > +    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> > > > +    for the definition of the components.
> > > >    }
> > > >    \hline 
> > > >    \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
> > > > @@ -2340,12 +2423,22 @@ GPR  &   Input Value     & Output Value \\
> > > >  \hline
> > > >    2   &  Subchannel ID    & Host Cookie  \\
> > > >  \hline
> > > > -  3   & Virtqueue number  &              \\
> > > > +  3   & Notification data &              \\
> > > >  \hline
> > > >    4   &   Host Cookie     &              \\
> > > >  \hline
> > > >  \end{tabular}
> > > > 
> > > > +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > > > +the \field{Notification data} contains the Virtqueue number.
> > > > +
> > > > +When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
> > > > +the value has the following format:
> > > > +\lstinputlisting{notifications-be.c}
> > > > +
> > > > +See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
> > > > +for the definition of the components.
> > > > +
> > > >  \devicenormative{\paragraph}{Guest->Host Notification}{Virtio Transport Options / Virtio over channel I/O / Device Operation / Guest->Host Notification}
> > > >  The device MUST ignore bits 0-31 (counting from the left) of GPR2.
> > > >  This aligns passing the subchannel ID with the way it is passed
> > > > @@ -5348,6 +5441,10 @@ Descriptors} and \ref{sec:Packed Virtqueues / Indirect Flag: Scatter-Gather Supp
> > > >    \item[VIRTIO_F_IN_ORDER(35)] This feature indicates
> > > >    that all buffers are used by the device in the same
> > > >    order in which they have been made available.
> > > > +  \item[VIRTIO_F_NOTIFICATION_DATA(36)] This feature indicates
> > > > +  that drivers pass extra data (besides identifying the Virtqueue)  
> > > 
> > > s/drivers/driver/ plural seems inappropriate
> > >   
> > > > +  in their device notifications.  
> 
> As a feature is negotiated per-device anyway, agreed.
> 
> "This feature indicates that the driver passes extra data (besides
> identifying the virtqueue) in its device notifications."
> 
> > > 
> > > I'm not really satisfied with this description but have nothing
> > > better to offer, so I'm willing to accept.
> > > 
> > >   
> > > > +  See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}.
> > > >  \end{description}
> > > > 
> > > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > > > diff --git a/notifications-be.c b/notifications-be.c
> > > > new file mode 100644
> > > > index 0000000..5be947e
> > > > --- /dev/null
> > > > +++ b/notifications-be.c
> > > > @@ -0,0 +1,5 @@
> > > > +be32 {
> > > > +	vqn : 16;
> > > > +	next_off : 15;
> > > > +	next_wrap : 1;  
> > > 
> > > Would it make sense to switch next_wrap and next_off for be.
> > > I mean that way we should be able to read the avail_idx at
> > > once (I think) instead of having to compute it first from
> > > next_off and next_wrap.  
> > 
> > Makes sense.
> 
> Nod.
> 
> > 
> > > Again, I can't really tell what is behind yet. Thus it may
> > > be completely irrelevant.
> > > 
> > > 
> > > Regards,
> > > Halil
> > >   
> > > > +};
> > > > diff --git a/notifications-le.c b/notifications-le.c
> > > > new file mode 100644
> > > > index 0000000..fe51267
> > > > --- /dev/null
> > > > +++ b/notifications-le.c
> > > > @@ -0,0 +1,5 @@
> > > > +le32 {
> > > > +	vqn : 16;
> > > > +	next_off : 15;
> > > > +	next_wrap : 1;
> > > > +};
> > > >   

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Cornelia Huck]

On Thu, 29 Mar 2018 15:52:34 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Mar 29, 2018 at 11:05:55AM +0200, Cornelia Huck wrote:
> > On Wed, 28 Mar 2018 20:21:47 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:  

> > > > The notifications *sent by the device to the driver (virtqueue or
> > > > configuration change)* are often referred to as *interrupts* but occasionally
> > > > also as notifications (e.g. 'host->guest notification').  
> > 
> > I think 'notifications' is a better term for these. I'm thinking of a
> > driver polling for outstanding notifications from the device, no
> > interrupts involved.  
> 
> In this context when polling there are no notifications - rememeber we are
> talking about notification suppression.

OK, that's my s390 background again. We can easily make notification
status available (setting indicators, making a subchannel status
pending), but not deliver an interrupt because the guest has not
enabled interrupts and polls instead. (In fact, such an operation mode
is not uncommon for the traditional s390 OSs.)

But we can also just keep this as-is, it's probably not confusing for
most people anyway.

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Michael S. Tsirkin]

On Thu, Mar 29, 2018 at 03:04:55PM +0200, Cornelia Huck wrote:
> On Thu, 29 Mar 2018 15:52:34 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Mar 29, 2018 at 11:05:55AM +0200, Cornelia Huck wrote:
> > > On Wed, 28 Mar 2018 20:21:47 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:  
> 
> > > > > The notifications *sent by the device to the driver (virtqueue or
> > > > > configuration change)* are often referred to as *interrupts* but occasionally
> > > > > also as notifications (e.g. 'host->guest notification').  
> > > 
> > > I think 'notifications' is a better term for these. I'm thinking of a
> > > driver polling for outstanding notifications from the device, no
> > > interrupts involved.  
> > 
> > In this context when polling there are no notifications - rememeber we are
> > talking about notification suppression.
> 
> OK, that's my s390 background again. We can easily make notification
> status available (setting indicators, making a subchannel status
> pending), but not deliver an interrupt because the guest has not
> enabled interrupts and polls instead. (In fact, such an operation mode
> is not uncommon for the traditional s390 OSs.)
> 
> But we can also just keep this as-is, it's probably not confusing for
> most people anyway.

Oh I see. In theory MSI also has on-device bits to signal events. They
are still called "interrupt pending" there.

It's up to you guys then.

-- 
MST

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Halil Pasic]

On 03/29/2018 11:05 AM, Cornelia Huck wrote:
> On Wed, 28 Mar 2018 20:21:47 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:
>>>
>>>
>>> On 03/27/2018 06:37 PM, Michael S. Tsirkin wrote:  
>>>> Some devices benefit from ability to find out the number of available
>>>> descriptors in the ring: for efficiency or as a debugging aid.
>>>>
>>>> To help with these optimizations, add a new feature:
>>>> VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
>>>> device include this extra information.  
>>>
>>> I'm pondering about symmetry and about the nature of these
>>> optimizations. By symmetry I mean this only works driver->device.
>>> Why do we want this the one but not the other way around?  
>>
>> Because
>> - memory is already closer to CPU than to devices
>> - there is no way to pass extra data with an interrupt
>>
>>

Thanks for the explanation!

>>>>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>
>>>> v11 -> v12
>>>> 	add missing 'the' article
>>>> 	drop duplicate text sections
>>>> 	use separate listing files for BE and LE, making
>>>> 	format explicit.
>>>>
>>>> v10 -> v11:
>>>>         drop mention of interrupts: current proposal does not include
>>>>         this interrupt related information
>>>>         drop unrelated introduction sections
>>>>
>>>>
>>>>
>>>>  content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>  notifications-be.c |   5 +++
>>>>  notifications-le.c |   5 +++
>>>>  3 files changed, 113 insertions(+), 6 deletions(-)
>>>>  create mode 100644 notifications-be.c
>>>>  create mode 100644 notifications-le.c
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 7a92cb1..ae5fa4c 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -283,9 +283,49 @@ Packed Virtqueues}).
>>>>  Every driver and device supports either the Packed or the Split
>>>>  Virtqueue format, or both.
>>>>
>>>> +\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}  
>>>
>>> Don't we call the same 'device notification' in 2.5.12.4 Notifying The Device.  
>>
>> Oh right.
>>
>>> The wordings used to denote the different notifications seems to be
>>> chaotic and maybe somewhat confusing overall.
>>>
>>> Let me provide some examples:
>>> For *virtqueue notification sent by the driver to the device* we use:
>>>    * 'driver notifications' (here)  
>>
>> We should fix this I think.
>>
>>>    * 'device notification' in 2.5.12.4 Notifying The Device)
>>>    * 'virtqueue notification' (2.5.9 Virtqueue Notification Suppression)  
>>
>> Let's change these to 'virtqueue device notifications'?
>> It's a separate issue though, not introduced by this patch.
>>
>>>    * 'guest -> host notification' (4.3.3.2 Guest->Host Notification)  
>>
>> That's a transport thing specific to virtio over MMIO.
> 
> ccw, right?
> 

Yes.

> I called these 'guest->host' (and 'host->guest') because it was more
> easily understandable at the time. It would be better to call them
> 'device notification' and 'driver notification' to align with the
> wording elsewhere. (As a separate patch.)
> 

I'm definitely in favor of aligned wording. I'm also in favor of a
separate patch, as long as this one does not introduce conflicts,
which is my primary concern.

And the 'guest->host' indeed came across as one of the easy to
understand when I was first exposed to the spec. About 'call them
device notification and driver notification' I'm not sure. I
think the conflict, which is my primary concern, demonstrates
that it's too easy to get it wrong (and to hard to remember did
we tie the name to the recipient or to the sender). Moreover
the device sends two types of notifications (virtqueue and confing).

I don't have a good solution, otherwise I would have proposed already.
Maybe something like 'available notification' and 'used notification' is
worth considering. I don't like that one can read available and used
as normal English (that is a notification that is available and not
a notification that buffers were made available at the given virtqueue).
Yet the alternatives I had in mind like 'driver to device virtqueue
notification' are too verbose.


>>
>>> The notifications *sent by the device to the driver (virtqueue or
>>> configuration change)* are often referred to as *interrupts* but occasionally
>>> also as notifications (e.g. 'host->guest notification').
> 
> I think 'notifications' is a better term for these. I'm thinking of a
> driver polling for outstanding notifications from the device, no
> interrupts involved.
> 

I agree that notifications is more generic (and can fit any mechanism
established by some transport) while interrupt does smell like implementation
a bit.


> But we can discuss that later.
> 

Agreed.

>>>
>>>
>>>   
>>>> +The driver is sometimes required to notify the device after
>>>> +making changes to the virtqueue.
>>>> +  
>>>
>>> I don't like sometimes. I would prefer something like
>>> 'Under certain circumstances the driver is required issue
>>> a virtqueue notification to the device.  
>>
>>
>> Under certain circumstances just says sometimes in 3 words.
>> Can't we find a way to say it that does not bump the word count x3?
> 
> What's wrong with "Sometimes, the driver is required to issue..."?
> 

No biggie, but 
sometimes: Occasionally, rather than all of the time. 
(https://en.oxforddictionaries.com/definition/sometimes)
Based on dictionary definitons and on my intuition it occurs
to me that 'sometimes' has a temporal aspect.

AFAIR the device can disable these notifications altogether
and happily poll the ring (that is avail or descriptor ring
depending on packed). 'Sometimes' would then stand for 'never'
as the 'certain circumstances' never occur.

But it ain't a big deal really.

>>
>>> The method is transport
>>> defined.'  
>>
>> That's good to add.
> 
> Agreed.
> 
>>
>>>> +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
>>>> +this notification involves sending the
>>>> +virtqueue number to the device (method depending on the transport).  
>>>
>>> I don't like 'involves' here. I think it's supposed to tell
>>> us that notification is a vitqueue level operation. Moreover
>>> the point is  when VIRTIO_F_NOTIFICATION_DATA has not been negotiated
>>> it's a virtueue notification and nothing more.
> 
> What about
> 
> "When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver
> notifies the device about the affected virtqueue only."
> 
> ?

I like it.

> 
>>>   
>>>> +
>>>> +However, some devices benefit from the ability to find out the number of
>>>> +available descriptors in the ring without accessing the virtqueue in memory:  
>>>
>>> What is 'the ring'? (probably descriptor ring or available ring depending
>>> on what do we have)  
>>
>> Maybe buffers in the virtqueue?
> 
> Works for me.
> 

Hm. I'm not sure it brings us nearer to the truth. AFAIU for non-packed
this would work like charm since the 'ring' is the available ring. So
we have one available ring entry for a buffer (that is a descriptor
chain/scatter-gather list). OTOH for packed we have one entry per descriptor
and not per descriptor chain (that is buffer/scatter-gather list).
So AFAIU if we just stared (virtqueue empty) and want to send a notification
after making one buffer consisting of three elements an not using indirect
decriptors (that is three chained descriptors in the descriptor table or
descriptor ring) for non packed we want to send offset 1 and for packed
offset 3. So for packed 'buffers in virtqueue' seems to be a poor fit.

I think a more generic wording not using 'number of descriptors' and
'the ring' would probably be a better match. But I've failed to find
a better wording so I'm fine with whatever you decide, because we have
it spelled out properly a couple of lines later.

>>
>>>> +for efficiency or as a debugging aid.
>>>> +
>>>> +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
>>>> +has been negotiated, driver notifications to the device include
>>>> +the following information:
>>>> +
>>>> +\begin{description}
>>>> +\item [VQ number]
>>>> +\item [Offset]
>>>> +      Within the ring where the next available ring entry
>>>> +      will be written.
>>>> +      Without VIRTIO_F_RING_PACKED this refers to the
>>>> +      15 least significant bits of the available index.
>>>> +      With VIRTIO_F_RING_PACKED this refers to the offset
>>>> +      (in units of descriptor entries)  
>>>
>>> What does '(in units of descriptor entries)' mean?  
>>
>> That it's not an address in bytes.
>> Offset is a distance right?
>> Within ring implies from start of ring.
>> What is left is to tell what are the units.
>>
>>> Is it a
>>> mod ring size index?  
>>
>> I don't see how specifying units can be taken
>> to mean it's modulo some value :(

The semantic of virtq_avail.idx (aka available index) is defined
as: "idx field indicates where the driver would put the next descriptor
entry in the ring (modulo the queue size). This starts at 0, and increases."
Here 'ring' is virtq_avail.ring and not the descriptor table and
'next descriptor' actually means it's index in the descriptor
table. This is where my modulo cam from.

>>
>>> If it is, why not call it index consequently
>>> (instead of this offset-index-offset thing)?  
>>
>> In fact index in many places is *not* mod ring size.
>> For me offset means where it is.

My point was that we seem to use 'offset' for offset in bytes
(search for 'offset' in the current spec:
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html),
while we seem to use idx and index for denoting what you call
offset in a ring/table. (E.g. avail.idx, used.idx descriptor table index).

>> Just need to specify
>> the units.

I find this 'descriptor entry' a bit unfortunate. If you search the spec
for 'descriptor entry' you will get the wrong stuff. Maybe 'ring elements'
or 'ring entries' is an alternative. We also have a precedence for
spelling out 'not in bytes'.

> 
> I agree, and I think the wording is fine here.
>

Overall it ain't too bad. I can live with it. Was an observation.

Regards,
Halil


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Michael S. Tsirkin]

On Thu, Mar 29, 2018 at 04:15:19PM +0200, Halil Pasic wrote:
> 
> 
> On 03/29/2018 11:05 AM, Cornelia Huck wrote:
> > On Wed, 28 Mar 2018 20:21:47 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> >> On Wed, Mar 28, 2018 at 06:56:13PM +0200, Halil Pasic wrote:
> >>>
> >>>
> >>> On 03/27/2018 06:37 PM, Michael S. Tsirkin wrote:  
> >>>> Some devices benefit from ability to find out the number of available
> >>>> descriptors in the ring: for efficiency or as a debugging aid.
> >>>>
> >>>> To help with these optimizations, add a new feature:
> >>>> VIRTIO_F_NOTIFICATION_DATA. When negotiated, driver notifications to the
> >>>> device include this extra information.  
> >>>
> >>> I'm pondering about symmetry and about the nature of these
> >>> optimizations. By symmetry I mean this only works driver->device.
> >>> Why do we want this the one but not the other way around?  
> >>
> >> Because
> >> - memory is already closer to CPU than to devices
> >> - there is no way to pass extra data with an interrupt
> >>
> >>
> 
> Thanks for the explanation!
> 
> >>>>
> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>> ---
> >>>>
> >>>> v11 -> v12
> >>>> 	add missing 'the' article
> >>>> 	drop duplicate text sections
> >>>> 	use separate listing files for BE and LE, making
> >>>> 	format explicit.
> >>>>
> >>>> v10 -> v11:
> >>>>         drop mention of interrupts: current proposal does not include
> >>>>         this interrupt related information
> >>>>         drop unrelated introduction sections
> >>>>
> >>>>
> >>>>
> >>>>  content.tex        | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>>>  notifications-be.c |   5 +++
> >>>>  notifications-le.c |   5 +++
> >>>>  3 files changed, 113 insertions(+), 6 deletions(-)
> >>>>  create mode 100644 notifications-be.c
> >>>>  create mode 100644 notifications-le.c
> >>>>
> >>>> diff --git a/content.tex b/content.tex
> >>>> index 7a92cb1..ae5fa4c 100644
> >>>> --- a/content.tex
> >>>> +++ b/content.tex
> >>>> @@ -283,9 +283,49 @@ Packed Virtqueues}).
> >>>>  Every driver and device supports either the Packed or the Split
> >>>>  Virtqueue format, or both.
> >>>>
> >>>> +\subsection{Driver notifications} \label{sec:Virtqueues / Driver notifications}  
> >>>
> >>> Don't we call the same 'device notification' in 2.5.12.4 Notifying The Device.  
> >>
> >> Oh right.
> >>
> >>> The wordings used to denote the different notifications seems to be
> >>> chaotic and maybe somewhat confusing overall.
> >>>
> >>> Let me provide some examples:
> >>> For *virtqueue notification sent by the driver to the device* we use:
> >>>    * 'driver notifications' (here)  
> >>
> >> We should fix this I think.
> >>
> >>>    * 'device notification' in 2.5.12.4 Notifying The Device)
> >>>    * 'virtqueue notification' (2.5.9 Virtqueue Notification Suppression)  
> >>
> >> Let's change these to 'virtqueue device notifications'?
> >> It's a separate issue though, not introduced by this patch.
> >>
> >>>    * 'guest -> host notification' (4.3.3.2 Guest->Host Notification)  
> >>
> >> That's a transport thing specific to virtio over MMIO.
> > 
> > ccw, right?
> > 
> 
> Yes.
> 
> > I called these 'guest->host' (and 'host->guest') because it was more
> > easily understandable at the time. It would be better to call them
> > 'device notification' and 'driver notification' to align with the
> > wording elsewhere. (As a separate patch.)
> > 
> 
> I'm definitely in favor of aligned wording. I'm also in favor of a
> separate patch, as long as this one does not introduce conflicts,
> which is my primary concern.
> 
> And the 'guest->host' indeed came across as one of the easy to
> understand when I was first exposed to the spec. About 'call them
> device notification and driver notification' I'm not sure. I
> think the conflict, which is my primary concern, demonstrates
> that it's too easy to get it wrong (and to hard to remember did
> we tie the name to the recipient or to the sender). Moreover
> the device sends two types of notifications (virtqueue and confing).
> 
> I don't have a good solution, otherwise I would have proposed already.
> Maybe something like 'available notification' and 'used notification' is
> worth considering. I don't like that one can read available and used
> as normal English (that is a notification that is available and not
> a notification that buffers were made available at the given virtqueue).

I think I like this.

Available buffer / used buffer notifications?


We'd have to make this change all over the spec,
if you like it pls open a github issue and post
a patch.

> Yet the alternatives I had in mind like 'driver to device virtqueue
> notification' are too verbose.
> 
> >>
> >>> The notifications *sent by the device to the driver (virtqueue or
> >>> configuration change)* are often referred to as *interrupts* but occasionally
> >>> also as notifications (e.g. 'host->guest notification').
> > 
> > I think 'notifications' is a better term for these. I'm thinking of a
> > driver polling for outstanding notifications from the device, no
> > interrupts involved.
> > 
> 
> I agree that notifications is more generic (and can fit any mechanism
> established by some transport) while interrupt does smell like implementation
> a bit.
> 
> 
> > But we can discuss that later.
> > 
> 
> Agreed.
> 
> >>>
> >>>
> >>>   
> >>>> +The driver is sometimes required to notify the device after
> >>>> +making changes to the virtqueue.
> >>>> +  
> >>>
> >>> I don't like sometimes. I would prefer something like
> >>> 'Under certain circumstances the driver is required issue
> >>> a virtqueue notification to the device.  
> >>
> >>
> >> Under certain circumstances just says sometimes in 3 words.
> >> Can't we find a way to say it that does not bump the word count x3?
> > 
> > What's wrong with "Sometimes, the driver is required to issue..."?
> > 
> 
> No biggie, but 
> sometimes: Occasionally, rather than all of the time. 
> (https://en.oxforddictionaries.com/definition/sometimes)
> Based on dictionary definitons and on my intuition it occurs
> to me that 'sometimes' has a temporal aspect.
> 
> AFAIR the device can disable these notifications altogether
> and happily poll the ring (that is avail or descriptor ring
> depending on packed). 'Sometimes' would then stand for 'never'
> as the 'certain circumstances' never occur.
> 
> But it ain't a big deal really.
> 
> >>
> >>> The method is transport
> >>> defined.'  
> >>
> >> That's good to add.
> > 
> > Agreed.
> > 
> >>
> >>>> +When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> >>>> +this notification involves sending the
> >>>> +virtqueue number to the device (method depending on the transport).  
> >>>
> >>> I don't like 'involves' here. I think it's supposed to tell
> >>> us that notification is a vitqueue level operation. Moreover
> >>> the point is  when VIRTIO_F_NOTIFICATION_DATA has not been negotiated
> >>> it's a virtueue notification and nothing more.
> > 
> > What about
> > 
> > "When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver
> > notifies the device about the affected virtqueue only."
> > 
> > ?
> 
> I like it.
> 
> > 
> >>>   
> >>>> +
> >>>> +However, some devices benefit from the ability to find out the number of
> >>>> +available descriptors in the ring without accessing the virtqueue in memory:  
> >>>
> >>> What is 'the ring'? (probably descriptor ring or available ring depending
> >>> on what do we have)  
> >>
> >> Maybe buffers in the virtqueue?
> > 
> > Works for me.
> > 
> 
> Hm. I'm not sure it brings us nearer to the truth. AFAIU for non-packed
> this would work like charm since the 'ring' is the available ring. So
> we have one available ring entry for a buffer (that is a descriptor
> chain/scatter-gather list). OTOH for packed we have one entry per descriptor
> and not per descriptor chain (that is buffer/scatter-gather list).
> So AFAIU if we just stared (virtqueue empty) and want to send a notification
> after making one buffer consisting of three elements an not using indirect
> decriptors (that is three chained descriptors in the descriptor table or
> descriptor ring) for non packed we want to send offset 1 and for packed
> offset 3. So for packed 'buffers in virtqueue' seems to be a poor fit.
> 
> I think a more generic wording not using 'number of descriptors' and
> 'the ring' would probably be a better match. But I've failed to find
> a better wording so I'm fine with whatever you decide, because we have
> it spelled out properly a couple of lines later.
> 

... to find out about available buffers in the virtqueue ... ?

> >>
> >>>> +for efficiency or as a debugging aid.
> >>>> +
> >>>> +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
> >>>> +has been negotiated, driver notifications to the device include
> >>>> +the following information:
> >>>> +
> >>>> +\begin{description}
> >>>> +\item [VQ number]
> >>>> +\item [Offset]
> >>>> +      Within the ring where the next available ring entry
> >>>> +      will be written.
> >>>> +      Without VIRTIO_F_RING_PACKED this refers to the
> >>>> +      15 least significant bits of the available index.
> >>>> +      With VIRTIO_F_RING_PACKED this refers to the offset
> >>>> +      (in units of descriptor entries)  
> >>>
> >>> What does '(in units of descriptor entries)' mean?  
> >>
> >> That it's not an address in bytes.
> >> Offset is a distance right?
> >> Within ring implies from start of ring.
> >> What is left is to tell what are the units.
> >>
> >>> Is it a
> >>> mod ring size index?  
> >>
> >> I don't see how specifying units can be taken
> >> to mean it's modulo some value :(
> 
> The semantic of virtq_avail.idx (aka available index) is defined
> as: "idx field indicates where the driver would put the next descriptor
> entry in the ring (modulo the queue size). This starts at 0, and increases."
> Here 'ring' is virtq_avail.ring and not the descriptor table and
> 'next descriptor' actually means it's index in the descriptor
> table. This is where my modulo cam from.

Well that's a good reason not to say index since we used
modulo with an index in one place and people might assume
it's a modulo in another place, isn't it?

> >>
> >>> If it is, why not call it index consequently
> >>> (instead of this offset-index-offset thing)?  
> >>
> >> In fact index in many places is *not* mod ring size.
> >> For me offset means where it is.
> 
> My point was that we seem to use 'offset' for offset in bytes
> (search for 'offset' in the current spec:
> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html),
> while we seem to use idx and index for denoting what you call
> offset in a ring/table. (E.g. avail.idx, used.idx descriptor table index).

So for an index you need to do modulo in other places, and I
was concerned people would this it's necessary here too.
I did not find a way to say "do not do modulo" but
specifying alternative units for an offset seemed natural.

> >> Just need to specify
> >> the units.
> 
> I find this 'descriptor entry' a bit unfortunate. If you search the spec
> for 'descriptor entry' you will get the wrong stuff. Maybe 'ring elements'
> or 'ring entries' is an alternative. We also have a precedence for
> spelling out 'not in bytes'.

We can be explicit here:

.. in 16-byte descriptor entry units ...

would this address the comment?


> > 
> > I agree, and I think the wording is fine here.
> >
> 
> Overall it ain't too bad. I can live with it. Was an observation.
> 
> Regards,
> Halil

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio] Re: [virtio-dev] Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Halil Pasic]

On 03/29/2018 04:30 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 29, 2018 at 04:15:19PM +0200, Halil Pasic wrote:
>>
[..]
>>
>> I don't have a good solution, otherwise I would have proposed already.
>> Maybe something like 'available notification' and 'used notification' is
>> worth considering. I don't like that one can read available and used
>> as normal English (that is a notification that is available and not
>> a notification that buffers were made available at the given virtqueue).
> 
> I think I like this.
> 
> Available buffer / used buffer notifications?
> 

I will do it!
@Connie: if you have concerns or something even better in mind
please do tell.
[..]

>>>>>> +However, some devices benefit from the ability to find out the number of
>>>>>> +available descriptors in the ring without accessing the virtqueue in memory:  
>>>>>
>>>>> What is 'the ring'? (probably descriptor ring or available ring depending
>>>>> on what do we have)  
>>>>
>>>> Maybe buffers in the virtqueue?
>>>
>>> Works for me.
>>>
>>
>> Hm. I'm not sure it brings us nearer to the truth. AFAIU for non-packed
>> this would work like charm since the 'ring' is the available ring. So
>> we have one available ring entry for a buffer (that is a descriptor
>> chain/scatter-gather list). OTOH for packed we have one entry per descriptor
>> and not per descriptor chain (that is buffer/scatter-gather list).
>> So AFAIU if we just stared (virtqueue empty) and want to send a notification
>> after making one buffer consisting of three elements an not using indirect
>> decriptors (that is three chained descriptors in the descriptor table or
>> descriptor ring) for non packed we want to send offset 1 and for packed
>> offset 3. So for packed 'buffers in virtqueue' seems to be a poor fit.
>>
>> I think a more generic wording not using 'number of descriptors' and
>> 'the ring' would probably be a better match. But I've failed to find
>> a better wording so I'm fine with whatever you decide, because we have
>> it spelled out properly a couple of lines later.
>>
> 
> ... to find out about available buffers in the virtqueue ... ?
> 

I like this. It's intuitive and at the same time vague
enough to be correct. And we will get precise soon enough.

>>>>
>>>>>> +for efficiency or as a debugging aid.
>>>>>> +
>>>>>> +To help with these optimizations, when VIRTIO_F_NOTIFICATION_DATA
>>>>>> +has been negotiated, driver notifications to the device include
>>>>>> +the following information:
>>>>>> +
>>>>>> +\begin{description}
>>>>>> +\item [VQ number]
>>>>>> +\item [Offset]
>>>>>> +      Within the ring where the next available ring entry
>>>>>> +      will be written.
>>>>>> +      Without VIRTIO_F_RING_PACKED this refers to the
>>>>>> +      15 least significant bits of the available index.
>>>>>> +      With VIRTIO_F_RING_PACKED this refers to the offset
>>>>>> +      (in units of descriptor entries)  
>>>>>
>>>>> What does '(in units of descriptor entries)' mean?  
>>>>
>>>> That it's not an address in bytes.
>>>> Offset is a distance right?
>>>> Within ring implies from start of ring.
>>>> What is left is to tell what are the units.
>>>>
>>>>> Is it a
>>>>> mod ring size index?  
>>>>
>>>> I don't see how specifying units can be taken
>>>> to mean it's modulo some value :(
>>
>> The semantic of virtq_avail.idx (aka available index) is defined
>> as: "idx field indicates where the driver would put the next descriptor
>> entry in the ring (modulo the queue size). This starts at 0, and increases."
>> Here 'ring' is virtq_avail.ring and not the descriptor table and
>> 'next descriptor' actually means it's index in the descriptor
>> table. This is where my modulo cam from.
> 
> Well that's a good reason not to say index since we used
> modulo with an index in one place and people might assume
> it's a modulo in another place, isn't it?
> 

As you have previously stated yourself index is usually
not modulo. But offset is fine too. I was probably over-thinking
this in the first place.

>>>>
>>>>> If it is, why not call it index consequently
>>>>> (instead of this offset-index-offset thing)?  
>>>>
>>>> In fact index in many places is *not* mod ring size.
>>>> For me offset means where it is.
>>
>> My point was that we seem to use 'offset' for offset in bytes
>> (search for 'offset' in the current spec:
>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html),
>> while we seem to use idx and index for denoting what you call
>> offset in a ring/table. (E.g. avail.idx, used.idx descriptor table index).
> 
> So for an index you need to do modulo in other places, and I
> was concerned people would this it's necessary here too.
> I did not find a way to say "do not do modulo" but
> specifying alternative units for an offset seemed natural.
> 

I'm not sure offset is a good way to express "do not do modulo".  When I
hear offset I think base + offset. The tricky part is what is base (*type
but also value*). If we say base is an iterator corresponding to some
kind of cyclical data structure like a ring or a ringbuffer then it's
reasonable to assume that the iterator is cyclical in respect to the
underlying array. For an example of what I mean by iterator you can have
a look at:
http://www.boost.org/doc/libs/1_61_0/doc/html/circular_buffer.html Please
notice that this circular isn't necessarily exactly 'modulo' and that
index has similar ambiguity. But his is probably over-thinking again.

>>>> Just need to specify
>>>> the units.
>>
>> I find this 'descriptor entry' a bit unfortunate. If you search the spec
>> for 'descriptor entry' you will get the wrong stuff. Maybe 'ring elements'
>> or 'ring entries' is an alternative. We also have a precedence for
>> spelling out 'not in bytes'.
> 
> We can be explicit here:
> 
> .. in 16-byte descriptor entry units ...
> 
> would this address the comment?
> 
> 
As you probably already understand: there is not much of a concern, yet I
wouldn't be completely satisfied with that solution. But I can certainly
live with any of the proposed solutions.

For me the least ambiguous would be something like the: 'With
VIRTIO_F_RING_PACKED the value of Offset is such that the address of the
descriptor area + 16 * Offset is the address where the next available
descriptor (that is the first descriptor of the next chain) will be
written to.' but it's kinda ugly.

Again, I shouldn't have complained abut it in the first place:
it was good enough. Pick whichever solution you deem the best.

Regards,
Halil


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] on spec clarifications

[Halil Pasic]

On 03/29/2018 12:30 AM, Michael S. Tsirkin wrote:
> Halil, 
> in the past you mentioned that you see some parts of the specification
> as unclear.
> 
> Will you have the time to look into this and send patches, or even just
> open github issues?
> 
> I'd like us to start looking at wrapping up the next spec version.
> 

There are a couple of I would have preferred done differently. Yet
my estimate regarding the pain/gain ratio for fixing most of the things
I dislike tells me: careful much pain and little gain. I'm a terrible
writer, and that does not make things easier.

I intend cook up some patches for the smaller things (the notifications
terminology, and AFAIR there was some stuff I noticed while reviewing packed)
this week. 

I was also pondering about the structure and what guarantees does
the specification provide for the correct implementations (e.g. interoperability
with any other correct implementation). I'm not sure I'm willing to
to muster the effort to try to improve these aspects. If I will probably
start with an RFC summarizing the issues and some ideas on what can be done.
Anyway I don't see this block as something that should delay the next
release.

Regards,
Halil




---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio] Re: [virtio-dev] Re: [virtio] [PATCH v12] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Cornelia Huck]

On Mon, 2 Apr 2018 17:34:50 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 03/29/2018 04:30 PM, Michael S. Tsirkin wrote:
> > On Thu, Mar 29, 2018 at 04:15:19PM +0200, Halil Pasic wrote:  
> >>  
> [..]
> >>
> >> I don't have a good solution, otherwise I would have proposed already.
> >> Maybe something like 'available notification' and 'used notification' is
> >> worth considering. I don't like that one can read available and used
> >> as normal English (that is a notification that is available and not
> >> a notification that buffers were made available at the given virtqueue).  
> > 
> > I think I like this.
> > 
> > Available buffer / used buffer notifications?
> >   
> 
> I will do it!
> @Connie: if you have concerns or something even better in mind
> please do tell.

No further comments from me.

---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] on spec clarifications

[Halil Pasic]

On 04/03/2018 02:11 AM, Halil Pasic wrote:
>> Halil, 
>> in the past you mentioned that you see some parts of the specification
>> as unclear.
>>
>> Will you have the time to look into this and send patches, or even just
>> open github issues?
>>
>> I'd like us to start looking at wrapping up the next spec version.
>>
> There are a couple of I would have preferred done differently. Yet
> my estimate regarding the pain/gain ratio for fixing most of the things
> I dislike tells me: careful much pain and little gain. I'm a terrible
> writer, and that does not make things easier.
> 
> I intend cook up some patches for the smaller things (the notifications
> terminology, and AFAIR there was some stuff I noticed while reviewing packed)
> this week. 

Sorry , till the end of the week did not work out. This 'available/used
buffer notification' change turned out for me surprisingly invasive and something
came up so I could not invest all the time I was intending to. But I did manage
to some hours on the spec.

I will try to send out an incomplete RFC on 'available/used buffer notification'
to get some feedback is it really what we want before going all the way.

Regards,
Halil


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php