feedback/1.txt

Document: virtio-v1.0-csprd01
Number: 1
Date: Fri, 10 Jan 2014 11:01:44 +0100
Link to Mail: https://lists.oasis-open.org/archives/virtio-comment/201401/msg00000.html
Commenter name: Thomas Huth <thuth@linux.vnet.ibm.com>
Decision: Agreed at meeting 2014-01-28.

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

Page 10 / Configuration Space:

- "... nor or reads from multiple fields"
  => that's difficult to parse, is this sentence right?

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

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

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.

Page 19:

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

Proposal:

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