feedback.txt: update

Signed-off-by: Rusty Russell <rusty@au1.ibm.com>

git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@188 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652

1 files changed, 147 insertions, 5 deletions

diff --git a/feedback.txt b/feedback.txt
index c1a7a01..dd47da9 100644
--- a/feedback.txt
+++ b/feedback.txt
@@ -1,11 +1,153 @@
 COMMENTS FOR virtio-v1.0-csprd01
 ================================
 
-Number:
-Date:
-Link to Mail:
-Commenter name:
+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>
 
-[DETAILS]
+- 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
 
 Decision: