diff options
Diffstat (limited to 'feedback.txt')
| -rw-r--r-- | feedback.txt | 152 |
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: |