[virtio] [PATCH 0/5] fixes and tweaks for virtio v1.1

[virtio] [PATCH 0/5] fixes and tweaks for virtio v1.1

[Halil Pasic]

Some of these are more and some are less important. Please
be careful, there is always a chance that I misunderstood something
and that I'm trying to change for the worse.

I've identified some other weak point's too, but tried to
proceed in order the more grave and the better I think I
understand it (and thus propose a fix) the higher the priority.

Some stuff I've left for my notifiers patch set. This one
and that one will have to get merged if both turn out to
be good.

Halil Pasic (5):
  packed-ring: fix used descriptors but meant buffers
  consistency: clean up stale  wording for v1.1 terms
  packed-packed: fix supplying buffers description
  packed-ring: reposition drivernormative on driver notifications
  packed-ring: tweak driver code example

 content.tex     |   14 ++++----
 packed-ring.tex |   96 +++++++++++++++++++++++++++++-------------------------
 2 files changed, 59 insertions(+), 51 deletions(-)


---------------------------------------------------------------------
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] [PATCH 1/5] packed-ring: fix used descriptors but meant buffers

[Halil Pasic]

The subsection is talking about virtio buffers, that is possibly as
scatter-gather list mapped by multiple descriptors. Mistakenly we
at places we say descriptors instead of buffers.

While at it fix the title case capitalization. We seem to use
title case capitalization for subsections in most cases.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 packed-ring.tex |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/packed-ring.tex b/packed-ring.tex
index 2ef9559..eb006b0 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -272,10 +272,10 @@ Buffer ID is also reserved and is ignored by the device.
 In descriptors with VIRTQ_DESC_F_INDIRECT set VIRTQ_DESC_F_WRITE
 is reserved and is ignored by the device.
 
-\subsection{In-order use of descriptors}
-\label{sec:Packed Virtqueues / In-order use of descriptors}
+\subsection{In-Order Use of Buffers}
+\label{sec:Packed Virtqueues / In-Order Use of Buffers}
 
-Some devices always use descriptors in the same order in which
+Some devices always use buffers in the same order in which
 they have been made available. These devices can offer the
 VIRTIO_F_IN_ORDER feature. If negotiated, this knowledge allows
 devices to notify the use of a batch of buffers to the driver by
-- 
1.7.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] [PATCH 2/5] consistency: clean up stale wording for v1.1 terms

[Halil Pasic]

Occasionally we still use used and available ring instead of Device and
Driver Area respectively. Same goes for descriptor table with the
difference that depending on the context we need either Descriptor Area
or descriptor ring.

Let's clean these up, at least where the cleanup is trivial.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 content.tex     |   14 +++++++-------
 packed-ring.tex |   10 ++++++----
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/content.tex b/content.tex
index 7a92cb1..e3ac662 100644
--- a/content.tex
+++ b/content.tex
@@ -1247,7 +1247,7 @@ The driver typically does this as follows, for each virtqueue a device has:
 
 \item Optionally, select a smaller virtqueue size and write it to \field{queue_size}.
 
-\item Allocate and zero Descriptor Table, Available and Used rings for the
+\item Allocate and zero Descriptor Area, Device Area and Driver Area for the
    virtqueue in contiguous physical memory.
 
 \item Optionally, if MSI-X capability is present and enabled on the
@@ -1769,9 +1769,9 @@ nor behaviour:
     writing to \field{QueueSel}.
   }
   \hline
-  \mmioreg{QueueAlign}{Used Ring alignment in the virtual queue}{0x03c}{W}{%
+  \mmioreg{QueueAlign}{Device Area alignment in the virtual queue}{0x03c}{W}{%
     Writing to this register notifies the device about alignment
-    boundary of the Used Ring in bytes. This value should be a power
+    boundary of the Device Area in bytes. This value should be a power
     of 2 and applies to the queue selected by writing to \field{QueueSel}.
   }
   \hline
@@ -1779,7 +1779,7 @@ nor behaviour:
     Writing to this register notifies the device about location of the
     virtual queue in the Guest's physical address space. This value
     is the index number of a page starting with the queue
-    Descriptor Table. Value zero (0x0) means physical address zero
+    Descriptor Area. Value zero (0x0) means physical address zero
     (0x00000000) and is illegal. When the driver stops using the
     queue it writes zero (0x0) to this register.
     Reading from this register returns the currently used page
@@ -1830,7 +1830,7 @@ The virtual queue is configured as follows:
    queue is not available.
 
 \item Allocate and zero the queue pages in contiguous virtual
-   memory, aligning the Used Ring to an optimal boundary (usually
+   memory, aligning the Device Area to an optimal boundary (usually
    page size). The driver should choose a queue size smaller than or
    equal to \field{QueueNumMax}.
 
@@ -2066,8 +2066,8 @@ struct vq_info_block {
 \end{lstlisting}
 
 \field{desc}, \field{driver} and \field{device} contain the guest
-addresses for the descriptor area,
-available area and used area for queue \field{index}, respectively. The actual
+addresses for the Descriptor Area,
+Driver Area and Device Area for queue \field{index}, respectively. The actual
 virtqueue size (number of allocated buffers) is transmitted in \field{num}.
 
 \devicenormative{\paragraph}{Configuring a Virtqueue}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Configuring a Virtqueue}
diff --git a/packed-ring.tex b/packed-ring.tex
index eb006b0..e656fed 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -445,7 +445,9 @@ struct pvirtq_event_suppress {
 };
 \end{lstlisting}
 
-\devicenormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table}
+\devicenormative{\subsection}{The Virtqueue Descriptor Ring}{Basic
+Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue
+Descriptor Ring}
 A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT
 read a device-writable buffer.
 A device MUST NOT use a descriptor unless it observes
@@ -454,7 +456,7 @@ the VIRTQ_DESC_F_AVAIL bit in its \field{flags} being changed
 A device MUST NOT change a descriptor after changing it's
 the VIRTQ_DESC_F_USED bit in its \field{flags}.
 
-\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
+\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}
 A driver MUST NOT change a descriptor unless it observes
 the VIRTQ_DESC_F_USED bit in its \field{flags} being changed.
 A driver MUST NOT change a descriptor after changing
@@ -498,7 +500,7 @@ were made available by the driver.
 The device MAY limit the number of buffers it will allow in a
 list.
 
-\drivernormative{\subsection}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
+\drivernormative{\subsection}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Ring / Indirect Descriptors}
 The driver MUST NOT set the DESC_F_INDIRECT flag unless the
 VIRTIO_F_INDIRECT_DESC feature was negotiated.   The driver MUST NOT
 set any flags except DESC_F_WRITE within an indirect descriptor.
@@ -541,7 +543,7 @@ What follows are the requirements of each stage in more detail.
 For each buffer element, b:
 
 \begin{enumerate}
-\item Get the next descriptor table entry, d
+\item Get the next descriptor ring entry, d
 \item Get the next free buffer id value
 \item Set \field{d.addr} to the physical address of the start of b
 \item Set \field{d.len} to the length of b.
-- 
1.7.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] Re: [PATCH 2/5] consistency: clean up stale wording for v1.1 terms

[Cornelia Huck]

On Mon,  9 Apr 2018 20:58:39 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Occasionally we still use used and available ring instead of Device and
> Driver Area respectively. Same goes for descriptor table with the
> difference that depending on the context we need either Descriptor Area
> or descriptor ring.
> 
> Let's clean these up, at least where the cleanup is trivial.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  content.tex     |   14 +++++++-------
>  packed-ring.tex |   10 ++++++----
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 

> -\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
> +\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}

While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a
change in conformance.tex as well.)

>  A driver MUST NOT change a descriptor unless it observes
>  the VIRTQ_DESC_F_USED bit in its \field{flags} being changed.
>  A driver MUST NOT change a descriptor after changing

---------------------------------------------------------------------
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: [PATCH 2/5] consistency: clean up stale wording for v1.1 terms

[Halil Pasic]

On 04/10/2018 11:30 AM, Cornelia Huck wrote:
> On Mon,  9 Apr 2018 20:58:39 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> Occasionally we still use used and available ring instead of Device and
>> Driver Area respectively. Same goes for descriptor table with the
>> difference that depending on the context we need either Descriptor Area
>> or descriptor ring.
>>
>> Let's clean these up, at least where the cleanup is trivial.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  content.tex     |   14 +++++++-------
>>  packed-ring.tex |   10 ++++++----
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
> 
>> -\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
>> +\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}
> 
> While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a
> change in conformance.tex as well.)
> 

Yeah, the thing with unrelated changes. Was considering it myself,
and decided not to since I'm doing basically the same change in
may places here. But I can do it next time.

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: [PATCH 2/5] consistency: clean up stale wording for v1.1 terms

[Cornelia Huck]

On Tue, 10 Apr 2018 12:04:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 04/10/2018 11:30 AM, Cornelia Huck wrote:
> > On Mon,  9 Apr 2018 20:58:39 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> Occasionally we still use used and available ring instead of Device and
> >> Driver Area respectively. Same goes for descriptor table with the
> >> difference that depending on the context we need either Descriptor Area
> >> or descriptor ring.
> >>
> >> Let's clean these up, at least where the cleanup is trivial.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >>  content.tex     |   14 +++++++-------
> >>  packed-ring.tex |   10 ++++++----
> >>  2 files changed, 13 insertions(+), 11 deletions(-)
> >>  
> >   
> >> -\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
> >> +\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}  
> > 
> > While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a
> > change in conformance.tex as well.)
> >   
> 
> Yeah, the thing with unrelated changes. Was considering it myself,
> and decided not to since I'm doing basically the same change in
> may places here. But I can do it next time.

OK, if it's in more than one place, let's do it in a separate patch and
clean up all of them.

---------------------------------------------------------------------
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] [PATCH 3/5] packed-packed: fix supplying buffers description

[Halil Pasic]

There was a discrepancy between the pseudo code and the
textual description of the algorithm describing how shall
a driver supply buffers to the device.

The textual description did not make sense to me, so I've
changed that one to be more in sync with the pseudo code.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
Still a bit shaky. Please review carefully! This patch
may as well be rooted in a terrible misunderstanding.
---
 packed-ring.tex |   64 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/packed-ring.tex b/packed-ring.tex
index e656fed..d93df3b 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -527,27 +527,12 @@ when using the packed virtqueue format in more detail.
 The driver offers buffers to one of the device's virtqueues as follows:
 
 \begin{enumerate}
-\item The driver places the buffer into free descriptor(s) in the Descriptor Ring.
-
-\item The driver performs a suitable memory barrier to ensure that it updates
-  the descriptor(s) before checking for notification suppression.
-
-\item If notifications are not suppressed, the driver notifies the device
-    of the new available buffers.
-\end{enumerate}
-
-What follows are the requirements of each stage in more detail.
-
-\subsubsection{Placing Available Buffers Into The Descriptor Ring}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / Supplying Buffers to The Device / Placing Available Buffers Into The Descriptor Ring}
-
-For each buffer element, b:
-
-\begin{enumerate}
-\item Get the next descriptor ring entry, d
 \item Get the next free buffer id value
+\item For each buffer element, b:
+\begin{enumerate}
+\item Get the next free descriptor ring entry, d
 \item Set \field{d.addr} to the physical address of the start of b
 \item Set \field{d.len} to the length of b.
-\item Set \field{d.id} to the buffer id
 \item Calculate the flags as follows:
 \begin{enumerate}
 \item If b is device-writable, set the VIRTQ_DESC_F_WRITE bit to 1, otherwise 0
@@ -556,20 +541,41 @@ For each buffer element, b:
 \end{enumerate}
 \item Perform a memory barrier to ensure that the descriptor has
       been initialized
-\item Set \field{d.flags} to the calculated flags value
+\item If b is not the first buffer element set \field{d.flags} to the
+      calculated flags value, otherwise save it
 \item If d is the last descriptor in the ring, toggle the
-      Driver Ring Wrap Counter
+      Driver Ring Wrap Counter and reset d to the head of the ring
 \item Otherwise, increment d to point at the next descriptor
 \end{enumerate}
+\item Point d to the descriptor corresponding to the last buffer element
+      and set \field{d.id} to the buffer id
+\item Save some value associated with the buffer id that makes the
+      skipping forward described in section \ref{sec:Packed Virtqueues /
+      Indirect Flag: Scatter-Gather Support} and in section \ref{sec:Packed
+      Virtqueues / In-Order Use of Buffers}
+\item Point d to the descriptor corresponding to the first buffer element
+      and set \field{d.flags} to the value calculated and saved 
+\item The driver performs a suitable memory barrier to ensure that all
+      writes done up to this point precede  all writhes done after this
+      point as observed by the device.
+\item Point d to the descriptor corresponding to the first buffer element
+      and set \field{d.flags} to the value calculated and saved 
+\item The driver performs a suitable memory barrier to ensure that it updates
+  the descriptor(s) before checking for notification suppression.
+\item If notifications are not suppressed, the driver notifies the device
+    of the new available buffers.
+\end{enumerate}
 
-This makes a single descriptor buffer available. However, in
-general the driver MAY make use of a batch of descriptors as part
+This makes a single virtio buffer available. However, in
+general the driver MAY make use of a batch of buffers as part
 of a single request. In that case, it defers updating
-the descriptor flags for the first descriptor
-(and the previous memory barrier) until after the rest of
-the descriptors have been initialized.
+the descriptor flags for the first descriptor of the first
+virtio buffer (and the preceding memory barrier) until after the rest of
+the descriptors have been written. The notification steps are performed
+only after the after the flags of the first descriptor of the first
+virtio buffer are set.
 
-Once the descriptor \field{flags} field is updated by the driver, this exposes
+Once the \field{flags} field of the first descriptor is updated by the driver, this exposes
 the descriptor and its contents.  The device MAY
 access the descriptor and any following descriptors the driver created and the
 memory they refer to immediately.
@@ -577,9 +583,9 @@ memory they refer to immediately.
 \drivernormative{\paragraph}{Updating flags}{Basic Facilities of
 a Virtio Device / Packed Virtqueues / Supplying Buffers to The
 Device / Updating flags}
-The driver MUST perform a suitable memory barrier before the
-\field{flags} update, to ensure the
-device sees the most up-to-date copy.
+The driver MUST perform a suitable memory barrier before updating the
+\field{flags} of the first descriptor (to ensure the device sees
+all the previous changes) .
 
 \subsubsection{Notifying The Device}\label{sec:Basic Facilities
 of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
-- 
1.7.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] [PATCH 4/5] packed-ring: reposition drivernormative on driver notifications

[Halil Pasic]

The drivernormative was simply in the wrong place. It does not belong
to the code example.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 packed-ring.tex |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/packed-ring.tex b/packed-ring.tex
index d93df3b..388850b 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -600,6 +600,10 @@ Suppression Structure Format}.
 The driver has to be careful to expose the new \field{flags}
 value before checking if notifications are suppressed.
 
+\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
+The driver MUST perform a suitable memory barrier before reading
+the Driver Event Suppression structure, to avoid missing a notification.
+
 \subsubsection{Implementation Example}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example}
 
 Below is a driver code example. It does not attempt to reduce
@@ -658,11 +662,6 @@ if (vq->device_event.flags != RING_EVENT_FLAGS_DISABLE) {
 
 \end{lstlisting}
 
-
-\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
-The driver MUST perform a suitable memory barrier before reading
-the Driver Event Suppression structure, to avoid missing a notification.
-
 \subsection{Receiving Used Buffers From The Device}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Receiving Used Buffers From The Device}
 
 Once the device has used buffers referred to by a descriptor (read from or written to them, or
-- 
1.7.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 4/5] packed-ring: reposition drivernormative on driver notifications

[Paolo Bonzini]

On 09/04/2018 20:58, Halil Pasic wrote:
>  The driver has to be careful to expose the new \field{flags}
>  value before checking if notifications are suppressed.

This paragraph should also be reworked to be part of the normative text
below, I think?  The memory barrier must be "between two things", and
only with this paragraph you know that it's between the flags write and
the suppression structure read.

Thanks,

Paolo

> +\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
> +The driver MUST perform a suitable memory barrier before reading
> +the Driver Event Suppression structure, to avoid missing a notification.
> +


---------------------------------------------------------------------
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 4/5] packed-ring: reposition drivernormative on driver notifications

[Halil Pasic]

On 04/09/2018 09:51 PM, Paolo Bonzini wrote:
> On 09/04/2018 20:58, Halil Pasic wrote:
>>  The driver has to be careful to expose the new \field{flags}
>>  value before checking if notifications are suppressed.
> 
> This paragraph should also be reworked to be part of the normative text
> below, I think?  The memory barrier must be "between two things", and
> only with this paragraph you know that it's between the flags write and
> the suppression structure read.
> 
All I did was to move the device normative to a better place. IMHO
it's current place is obviously wrong. So, I read your comment as
there should be a patch on top that further integrates the paragraphs
made adjacent by this patch. Is that right?

About the normative statements. I hinted before that I don't really
understand the role of normative statements in this specification. To
be more precise, my naive understanding of their role is in conflict
with the reality of the specification.

Can someone tell me what is supposed to go in a normative statements
and what is supposed to go outside? Along with an estimate how good
are we at adhering to those rules.

Regards,
Halil

> 
>> +\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
>> +The driver MUST perform a suitable memory barrier before reading
>> +the Driver Event Suppression structure, to avoid missing a notification.
>> +
> 
> 
> ---------------------------------------------------------------------
> 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 
> 


---------------------------------------------------------------------
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 4/5] packed-ring: reposition drivernormative on driver notifications

[Paolo Bonzini]

On 10/04/2018 12:25, Halil Pasic wrote:
> All I did was to move the device normative to a better place. IMHO
> it's current place is obviously wrong.

I agree with that - but IMO you're now separating two related paragraphs.

> So, I read your comment as
> there should be a patch on top that further integrates the paragraphs
> made adjacent by this patch. Is that right?

It can even be the same patch, reworked to put something like this in
the normative text:

The driver MUST ensure the write to the \field{flags} field is performed
before the read of the Driver Event Suppression, in order to avoid
missing a notification from the device.

> About the normative statements. I hinted before that I don't really
> understand the role of normative statements in this specification. To
> be more precise, my naive understanding of their role is in conflict
> with the reality of the specification.
> 
> Can someone tell me what is supposed to go in a normative statements
> and what is supposed to go outside? Along with an estimate how good
> are we at adhering to those rules.

Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a
normative statement.

Paolo

---------------------------------------------------------------------
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 4/5] packed-ring: reposition drivernormative on driver notifications

[Halil Pasic]

On 04/10/2018 01:05 PM, Paolo Bonzini wrote:
> On 10/04/2018 12:25, Halil Pasic wrote:
>> All I did was to move the device normative to a better place. IMHO
>> it's current place is obviously wrong.
> 
> I agree with that - but IMO you're now separating two related paragraphs.
> 

I'm not separating them they were separated, but I get your point.

>> So, I read your comment as
>> there should be a patch on top that further integrates the paragraphs
>> made adjacent by this patch. Is that right?
> 
> It can even be the same patch, reworked to put something like this in
> the normative text:
> 
> The driver MUST ensure the write to the \field{flags} field is performed
> before the read of the Driver Event Suppression, in order to avoid
> missing a notification from the device.
>

Will do a it, but I prefer to make this an extra patch.
 
>> About the normative statements. I hinted before that I don't really
>> understand the role of normative statements in this specification. To
>> be more precise, my naive understanding of their role is in conflict
>> with the reality of the specification.
>>
>> Can someone tell me what is supposed to go in a normative statements
>> and what is supposed to go outside? Along with an estimate how good
>> are we at adhering to those rules.
> 
> Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a
> normative statement.
> 

The problem here is, that this sentence you reformulated as a
'MUST' sentence was formulated as a 'has to be careful' sentence.

My concern (regarding the whole spec) is the completeness and self
the containment of it's normative portion -- I'm not sure if either is
pursued rigorously. For instance take 'Supplying Buffers to The
Device' either for split or for packed. The algorithms described
there aren't constituting a normative section. Do you think these
can be inferred from the normative sections?

Are the normative statements (e.g in v1.0 cs4) sufficient to
guarantee conforming implementations work (interoperably)? 

In math we have primitive notions, definitions, axioms, theorems, proofs
and hypotheses. Textbooks also have less formal text (beyond the
stuff listed) to either help with the understanding, or provide background.

My naive understanding of the purpose of non-normative sections of this
specification is: definitions, background (e.g. legacy interface),
and explanations facilitating *easier* understanding.

To stay at the math analogy, the device normative statements are
most akin to axioms I guess. As axioms along with primitive notions
and definitions, what constitutes a formal system.

So that's where my question is coming from. And I'm not keen to
put too much work into this. I'm just curious how should I reason
about stuff (when reviewing and voting for example). Sorry for getting
philosophical.

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 4/5] packed-ring: reposition drivernormative on driver notifications

[Paolo Bonzini]

On 10/04/2018 16:59, Halil Pasic wrote:
>> I agree with that - but IMO you're now separating two related paragraphs.
>
> I'm not separating them they were separated, but I get your point.

Yeah, I meant separating them with a heading.

>> The driver MUST ensure the write to the \field{flags} field is performed
>> before the read of the Driver Event Suppression, in order to avoid
>> missing a notification from the device.
>
> Will do a it, but I prefer to make this an extra patch.

Sure, yhat's your choice.

>>> About the normative statements. I hinted before that I don't really
>>> understand the role of normative statements in this specification. To
>>> be more precise, my naive understanding of their role is in conflict
>>> with the reality of the specification.
>>>
>>> Can someone tell me what is supposed to go in a normative statements
>>> and what is supposed to go outside? Along with an estimate how good
>>> are we at adhering to those rules.
>> Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a
>> normative statement.
>
> The problem here is, that this sentence you reformulated as a
> 'MUST' sentence was formulated as a 'has to be careful' sentence.

That was a mistake, in my opinion.

> My concern (regarding the whole spec) is the completeness and self
> the containment of it's normative portion -- I'm not sure if either is
> pursued rigorously. For instance take 'Supplying Buffers to The
> Device' either for split or for packed. The algorithms described
> there aren't constituting a normative section. Do you think these
> can be inferred from the normative sections?

Probably more from chapters such as "Basic Facilities of a Virtio
Device"; that is, it's the only way to use them so it need not be
normative.  Those chapters in turn could use some update with respect to
memory barriers, and sometimes lack some normative text.

For example "The Virtqueue Available Ring" for split virtqueues has
almost no normative text, and for "The Virtqueue Used Ring" (split
virtqueues too) "The device MUST set len prior to updating the used idx"
should mention a memory barrier.

> Are the normative statements (e.g in v1.0 cs4) sufficient to
> guarantee conforming implementations work (interoperably)? 
> 
> In math we have primitive notions, definitions, axioms, theorems, proofs
> and hypotheses. Textbooks also have less formal text (beyond the
> stuff listed) to either help with the understanding, or provide background.
> 
> My naive understanding of the purpose of non-normative sections of this
> specification is: definitions, background (e.g. legacy interface),
> and explanations facilitating *easier* understanding.

Yes.

> To stay at the math analogy, the device normative statements are
> most akin to axioms I guess. As axioms along with primitive notions
> and definitions, what constitutes a formal system.

Certainly not "fundamental" axioms (those would be things like a formal
memory model), but I understand what you mean and I think it makes sense.

Paolo

> So that's where my question is coming from. And I'm not keen to
> put too much work into this. I'm just curious how should I reason
> about stuff (when reviewing and voting for example). Sorry for getting
> philosophical.


---------------------------------------------------------------------
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 4/5] packed-ring: reposition drivernormative on driver notifications

[Michael S. Tsirkin]

On Tue, Apr 10, 2018 at 04:59:12PM +0200, Halil Pasic wrote:
> My concern (regarding the whole spec) is the completeness and self
> the containment of it's normative portion -- I'm not sure if either is
> pursued rigorously.

No it isn't, and the reason is we do not want to bring the development
to a complete halt :). Our charter is

	Enhancing the performance of virtual devices by standardizing key
	features of the VIRTIO (Virtual I/O) Device Specification

Our job is thus to facilitate development by standardizing where it's
helpful.

If we start asking people to write out a formal spec for any tiny
change before they can release code, development will stop and
performance will suffer.

And it's a balance: it should be clear what is going on,
and if people can write it rigorously without confusing
matters, it is helpful.

But we don't want something like e.g. the C or C++ language spec,
where you can read all of it and still have no idea how to use it.

> For instance take 'Supplying Buffers to The
> Device' either for split or for packed. The algorithms described
> there aren't constituting a normative section. Do you think these
> can be inferred from the normative sections?

No but it's ok for normative sections to refer to the non-normative
ones.


-- 
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 4/5] packed-ring: reposition drivernormative on driver notifications

[Halil Pasic]

On 04/20/2018 05:59 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2018 at 04:59:12PM +0200, Halil Pasic wrote:
>> My concern (regarding the whole spec) is the completeness and self
>> the containment of it's normative portion -- I'm not sure if either is
>> pursued rigorously.
> 
> No it isn't, and the reason is we do not want to bring the development
> to a complete halt :). Our charter is
> 
> 	Enhancing the performance of virtual devices by standardizing key
> 	features of the VIRTIO (Virtual I/O) Device Specification
> 
> Our job is thus to facilitate development by standardizing where it's
> helpful.
> 
> If we start asking people to write out a formal spec for any tiny
> change before they can release code, development will stop and
> performance will suffer.
> 
> And it's a balance: it should be clear what is going on,
> and if people can write it rigorously without confusing
> matters, it is helpful.
> 
> But we don't want something like e.g. the C or C++ language spec,
> where you can read all of it and still have no idea how to use it.
> 

I understand.

>> For instance take 'Supplying Buffers to The
>> Device' either for split or for packed. The algorithms described
>> there aren't constituting a normative section. Do you think these
>> can be inferred from the normative sections?
> 
> No but it's ok for normative sections to refer to the non-normative
> ones.
> 

Was, kind of, the point I tried to make. I've (for myself) given up on
these ideals. That's why I did not bother to think about isn't that sentence
normative section material. And I don't intend to clean up normative
vs non-normative.

Anyway, it is good to have it spelled out what is the objective of this document,
and what is not. I will continue to try to improve the specification,
in accordance to that -- and avoid hanging myself up on what is not
considered a primary objective.

Many thanks!

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 4/5] packed-ring: reposition drivernormative on driver notifications

[Michael S. Tsirkin]

On Fri, Apr 20, 2018 at 06:22:40PM +0200, Halil Pasic wrote:
> Was, kind of, the point I tried to make. I've (for myself) given up on
> these ideals. That's why I did not bother to think about isn't that sentence
> normative section material. And I don't intend to clean up normative
> vs non-normative.

Makes sense.

We do need to keep RFC2119 keywords out of the rest of the text simply to
avoid confusing people - from experience if we don't then during public
review people jump on it immediately and say hey this must go into the
normative section. It's just something that's easy to spot :)

-- 
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 

[virtio] [PATCH 5/5] packed-ring: tweak driver code example

[Halil Pasic]

This patch does three things. Emphasises that the example is pseudo-code
as we tend to stick to C otherwise and tweaks the wording, fixes
a typo in the pseudo code, and removes a line of code that I neither can
see the necessity for nor the sense of.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 packed-ring.tex |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/packed-ring.tex b/packed-ring.tex
index 388850b..f619069 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -604,9 +604,11 @@ value before checking if notifications are suppressed.
 The driver MUST perform a suitable memory barrier before reading
 the Driver Event Suppression structure, to avoid missing a notification.
 
-\subsubsection{Implementation Example}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example}
+\subsubsection{Pseudo-Code Example}\label{sec:Basic Facilities of a
+Virtio Device / Packed Virtqueues / Supplying Buffers to The Device /
+Pseudo-Code Example}
 
-Below is a driver code example. It does not attempt to reduce
+Below is driver pseudo-code example supplying buffers to the device. It does not attempt to reduce
 the number of device interrupts, neither does it support
 the VIRTIO_F_RING_EVENT_IDX feature.
 
@@ -621,7 +623,6 @@ sgs = 0;
 for (each buffer element b) {
         sgs++;
 
-        vq->ids[vq->next_avail] = -1;
         vq->desc[vq->next_avail].address = get_addr(b);
         vq->desc[vq->next_avail].len = get_len(b);
 
@@ -645,7 +646,7 @@ for (each buffer element b) {
 
         if (vq->next_avail >= vq->size) {
                 vq->next_avail = 0;
-                vq->avail_wrap_count \^= 1;
+                vq->avail_wrap_count ^= 1;
         }
 }
 vq->sgs[id] = sgs;
-- 
1.7.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 0/5] fixes and tweaks for virtio v1.1

[Halil Pasic]

ping

Some of the patches received feedback suggesting changes are to
be pursued. Some patches did not receive any.

I would prefer having at least a 'the right direction'/'seems wrong' type
of feedback for each patch before sending a non RFC with the requests
made here addressed.

On 04/09/2018 08:58 PM, Halil Pasic wrote:
> Some of these are more and some are less important. Please
> be careful, there is always a chance that I misunderstood something
> and that I'm trying to change for the worse.
> 
> I've identified some other weak point's too, but tried to
> proceed in order the more grave and the better I think I
> understand it (and thus propose a fix) the higher the priority.
> 
> Some stuff I've left for my notifiers patch set. This one
> and that one will have to get merged if both turn out to
> be good.
> 
> Halil Pasic (5):
>    packed-ring: fix used descriptors but meant buffers
>    consistency: clean up stale  wording for v1.1 terms
>    packed-packed: fix supplying buffers description
>    packed-ring: reposition drivernormative on driver notifications
>    packed-ring: tweak driver code example
> 
>   content.tex     |   14 ++++----
>   packed-ring.tex |   96 +++++++++++++++++++++++++++++-------------------------
>   2 files changed, 59 insertions(+), 51 deletions(-)
> 
> 
> ---------------------------------------------------------------------
> 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
> 


---------------------------------------------------------------------
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 0/5] fixes and tweaks for virtio v1.1

[Halil Pasic]

I've decided not send a v1 until the fate of the notifications
series gets deiced (so I can hopefully base these on those). So,
some more time for comments.

Regards,
Halil


On 04/20/2018 05:00 PM, Halil Pasic wrote:
> ping
> 
> Some of the patches received feedback suggesting changes are to
> be pursued. Some patches did not receive any.
> 
> I would prefer having at least a 'the right direction'/'seems wrong' type
> of feedback for each patch before sending a non RFC with the requests
> made here addressed.
> 
> On 04/09/2018 08:58 PM, Halil Pasic wrote:
>> Some of these are more and some are less important. Please
>> be careful, there is always a chance that I misunderstood something
>> and that I'm trying to change for the worse.
>>
>> I've identified some other weak point's too, but tried to
>> proceed in order the more grave and the better I think I
>> understand it (and thus propose a fix) the higher the priority.
>>
>> Some stuff I've left for my notifiers patch set. This one
>> and that one will have to get merged if both turn out to
>> be good.
>>
>> Halil Pasic (5):
>>    packed-ring: fix used descriptors but meant buffers
>>    consistency: clean up stale  wording for v1.1 terms
>>    packed-packed: fix supplying buffers description
>>    packed-ring: reposition drivernormative on driver notifications
>>    packed-ring: tweak driver code example
>>
>>   content.tex     |   14 ++++----
>>   packed-ring.tex |   96 +++++++++++++++++++++++++++++-------------------------
>>   2 files changed, 59 insertions(+), 51 deletions(-) 


---------------------------------------------------------------------
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