diff options
Diffstat (limited to 'feedback.txt')
| -rw-r--r-- | feedback.txt | 153 |
1 files changed, 0 insertions, 153 deletions
diff --git a/feedback.txt b/feedback.txt deleted file mode 100644 index dd47da9..0000000 --- a/feedback.txt +++ /dev/null @@ -1,153 +0,0 @@ -COMMENTS FOR 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> - -- 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: |