summaryrefslogtreecommitdiff
path: root/feedback/1.txt
blob: 65645c805f90226ff80a684bd45bdb4b3cb925f6 (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
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