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
151
152
|
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>
- 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:
|
