Re: [virtio-comment] Feedback for the first three chapters

[Rusty Russell <rusty@au1.ibm.com>]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>  Hello everybody,
>
> here's my feedback for the first three chapters of the Virtio draft 01.

Thanks very much for your feedback Thomas!

I've recorded this in our feedback file.  Some comments below, and the
resulting patch I'll propose to the committee at the bottom.

> Disclaimer: I am not a native english speaker, so some of my comments
> might be wrong, but then you could also take this as an indication that
> the related part of the spec might be difficult to understand for
> non-english speakers.
>
>
> General remarks: 
>
> - The first three chapters sometimes uses the pronoun "we" in sentences.
>   I think this should be avoided, since it is not always clear who is
>   meant with this pronoun: The reader? The driver? The device?
>
> - Some of the generic sections still use the term "PCI" though they
>   should not.
>
> I tried to mention the related spots below, but I'd like to suggest to
> scan again the whole document for "we" and "PCI" to be sure to get
> everything right.
>
>
> Page 8 / Introduction:
>
> - "Extensible: Virtio PCI devices contain feature bits ..."
>   => Remove the "PCI" in above sentence.

Thanks, a committee member caught this too, so should be fixed in draft
02.

> Page 10 / Configuration Space:
>
> - "... nor or reads from multiple fields"
>   => that's difficult to parse, is this sentence right?

Yes, the "or" is extraneous.

> Page 14 / The Virtqueue Available Ring
>
> - "The available ring refers to what descriptor chains the driver is
>   offering the device"
>   => Somewhat hard to read, maybe better something like this:
>   "The available ring refers to the descriptor chains that the driver
>   is offering to the device" ?

Agreed.  It is awkward.  How about this:

  The driver uses the available ring to offer buffers to the
  device: each ring entry refers to the head of a descriptor chain

> - "The "idx" field indicates where we would put the next descriptor
>   entry in the ring"
>   => "The "idx" field indicates where the driver would put the next
>   descriptor entry in the ring"

Yes.

> Page 16 / Device Initialization:
>
> - "2. Set the ACKNOWLEDGE status bit: we have noticed the device."
>   => "2. The guest OS sets the ACKNOWLEDGE status bit to indicate
>   that it has noticed the device."
>
> - "3. Set the DRIVER status bit: we know how to drive the device."
>   => "3. The driver sets the DRIVER status bit to indicate that
>   it knows how to drive the device"
>
> Page 18 / Notifying the device:
>
> - "... we go ahead and write to the PCI configuration space."
>   => "... the driver can go ahead and write to the configuration space."
>
> - "The avail_event field wraps naturally at 65536 as well, iving the
>   following algorithm ..."
>   => What does "iving" mean? I did not find that in my dictionary.

"giving"

> Page 19:
>
> - "It can then process used ring entries finally enabling interrupts ..."
>   => This sentence is hard to parse ... is there missing something
>   before "finally"?

Yes, I reworked this:

  For each ring, the driver MAY then disable interrupts by writing
  VRING_AVAIL_F_NO_INTERRUPT flag in avail structure, if required.
  Once it has processed the ring entries, it SHOULD re-enable
  interrupts by clearing the VRING_AVAIL_F_NO_INTERRUPT flag or updating the
  EVENT_IDX field in the available structure.

Here's the diff I'll propose from your feedback in this mail.

Thanks!
Rusty.

diff --git a/content.tex b/content.tex
index 803615d..8850c1a 100644
--- a/content.tex
+++ b/content.tex
@@ -145,7 +145,7 @@ Interface' in the section title.
 
 Configuration space is generally used for rarely-changing or
 initialization-time parameters.  Drivers MUST NOT assume reads from
-fields greater than 32 bits wide are atomic, nor or reads from
+fields greater than 32 bits wide are atomic, nor reads from
 multiple fields.
 
 Each transport provides a generation count for the configuration
@@ -418,11 +418,11 @@ the device MUST ignore the write-only flag (flags\&VRING_DESC_F_WRITE) in the de
 	};
 \end{lstlisting}
 
-The available ring refers to what descriptor chains the driver is offering the
+The driver uses the available ring to offer buffers to the
 device: each ring entry refers to the head of a descriptor chain.  It is only
 written by the driver and read by the device.
 
-The “idx” field indicates where we would put the next descriptor
+The “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.
 
 If the VIRTIO_RING_F_INDIRECT_DESC feature bit is not negotiated, the
@@ -515,9 +515,9 @@ The driver MUST follow this sequence to initialize a device:
 \begin{enumerate}
 \item Reset the device.
 
-\item Set the ACKNOWLEDGE status bit: we have noticed the device.
+\item Set the ACKNOWLEDGE status bit: the guest OS has notice the device.
 
-\item Set the DRIVER status bit: we know how to drive the device.
+\item Set the DRIVER status bit: the guest OS knows how to drive the device.
 
 \item Read device feature bits, and write the subset of feature bits
    understood by the OS and driver to the device.
@@ -686,7 +686,7 @@ we use a memory barrier here before reading the flags or the
 avail_event field.
 
 If the VIRTIO_F_RING_EVENT_IDX feature is not negotiated, and if the
-VRING_USED_F_NOTIFY flag is not set, we go ahead and notify the
+VRING_USED_F_NOTIFY flag is not set, the driver SHOULD notify the
 device.
 
 If the VIRTIO_F_RING_EVENT_IDX feature is negotiated, we read the
@@ -694,7 +694,7 @@ avail_event field in the available ring structure. If the
 available index crossed_the avail_event field value since the
 last notification, we go ahead and write to the PCI configuration
 space.  The avail_event field wraps naturally at 65536 as well,
-iving the following algorithm for calculating whether a device needs
+giving the following algorithm for calculating whether a device needs
 notification:
 
 \begin{lstlisting}
@@ -705,7 +705,7 @@ notification:
 
 Once the device has used a buffer (read from or written to it, or
 parts of both, depending on the nature of the virtqueue and the
-device), it sends an interrupt, following an algorithm very
+device), it SHOULD send an interrupt, following an algorithm very
 similar to the algorithm used for the driver to send the device a
 buffer:
 
@@ -732,11 +732,11 @@ buffer:
   \end{enumerate}
 \end{enumerate}
 
-For each ring, the driver should then disable interrupts by writing
+For each ring, the driver MAY then disable interrupts by writing
 VRING_AVAIL_F_NO_INTERRUPT flag in avail structure, if required.
-It can then process used ring entries finally enabling interrupts
-by clearing the VRING_AVAIL_F_NO_INTERRUPT flag or updating the
-EVENT_IDX field in the available structure.  The driver should then
+Once it has processed the ring entries, it SHOULD re-enable
+interrupts by clearing the VRING_AVAIL_F_NO_INTERRUPT flag or updating the
+EVENT_IDX field in the available structure.  The driver SHOULD then
 execute a memory barrier, and then recheck the ring empty
 condition. This is necessary to handle the case where after the
 last check and before enabling interrupts, an interrupt has been