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
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
|
Document: virtio-v1.0-csprd01
Number: 2
Date: Fri, 10 Jan 2014 13:49:49 +0100
Link to Mail: https://lists.oasis-open.org/archives/virtio-comment/201401/msg00001.html
Commenter name: Thomas Huth <thuth@linux.vnet.ibm.com>
Here's my feedback for Virtio draft 01, chapter 4:
Page 20 / PCI Device Layout:
- "To configure the device, use I/O and/or memory regions and/or PCI
configuration
space of the PCI device."
=> That sounds a little bit sparse/confusing. Maybe rather something like:
"To configure the device, it is possible to use the PCI configuration space
and/or to access the configuration data via an I/O and/or MMIO base-address
register."
Page 21:
- The "device_feature_select" and "driver_feature_select" paragraphs are
lacking
some punctuation marks inbetween.
Page 23 / Virtio Device Configuration Layout Detection:
- "This structure can optionally followed by extra data"
=> "This structure can optionally be followed by extra data"
Page 27 / MMIO Device Discovery:
- The device tree snippet is obviously an example. That's ok, but I think the
spec should explicitely say so (and maybe add some generic words about the
required properties before the example, too).
Chapter 4.3.2.*:
- In this chapter, the C-structs are marked with "__attribute__ ((packed));"
which is just a GNU-C extension, as far as I know. In the other chapters,
the structs are not marked with this string. So for consistency, I'd remove
them here, too (and maybe state somewhere at the beginning of the spec
that structs are considered to be without compiler padding inbetween)
Page 33:
- Some typos:
neccessarily => necessarily
issueing => issuing
Page 34 / Virtqueue Layout:
- "... with padded added ..."
=> "... with padding added ..."
Page 34 / Handling Device Features:
- The text says "Feature bits are in little-endian byte order", but the
"struct virtio_feature_desc" is described with "be32 features" ...
that's confusing -- are the feature bits now little or big endian?
Page 36:
- "Bit numbers start at the left"
=> I'd make this sentence more explicit, e.g.:
"Bit numbers start at the left, i.e. the most significant bit in the
first byte is assigned the bit number 0."
Page 36 / Notification via Adapter I/O Interrupts:
- "The guest-provided summary indicator is also set."
=> What value is set in the summary indicator byte? 0x01? 0x80? 0xff?
It maybe does not matter, since any non-zero value could be used, but
it might help to avoid confusion if you specify the exact value here.
Page 37 / Early printk for Virtio Consoles
- Is this early print really part of virtio-ccw? If yes, I think you
should also describe the register usage here.
Proposal:
Two patches:
diff --git a/content.tex b/content.tex
index 803615d..4ebc4b1 100644
--- a/content.tex
+++ b/content.tex
@@ -801,9 +801,10 @@ any Revision ID value.
\subsection{PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout}
-To configure the device,
-use I/O and/or memory regions and/or PCI configuration space of the PCI device.
-These contain the virtio header registers, the notification register, the
+The device is configured via I/O and/or memory regions (though see
+VIRTIO_PCI_CAP_PCI_CFG for access via the PCI configuration space).
+
+These regions contain the virtio header registers, the notification register, the
ISR status register and device specific registers, as specified by Virtio
Structure PCI Capabilities.
@@ -847,8 +848,7 @@ Common configuration structure layout is documented below:
\begin{description}
\item[device_feature_select]
The driver uses this to select which Feature Bits the device_feature field shows.
- Value 0x0 selects Feature Bits 0 to 31
- Value 0x1 selects Feature Bits 32 to 63
+ Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature Bits 32 to 63.
The device MUST present 0 on device_feature for any other value.
\item[device_feature]
@@ -857,8 +857,7 @@ Common configuration structure layout is documented below:
\item[driver_feature_select]
The driver uses this to select which Feature Bits the driver_feature field shows.
- Value 0x0 selects Feature Bits 0 to 31
- Value 0x1 selects Feature Bits 32 to 63
+ Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature Bits 32 to 63.
When set to any other value, reads from driver_feature
return 0, writing 0 into driver_feature has no effect. The driver
MUST not write any other value into driver_feature (a corollary of
@@ -899,7 +898,7 @@ Common configuration structure layout is documented below:
\item[queue_enable]
The driver uses this to selectively prevent the device from executing requests from this virtqueue.
- 1 - enabled; 0 - disabled
+ 1 - enabled; 0 - disabled.
The driver MUST configure the other virtqueue fields before enabling
the virtqueue.
@@ -1043,7 +1042,7 @@ read-only:
};
\end{lstlisting}
-This structure can optionally followed by extra data, depending on
+This structure can optionally be followed by extra data, depending on
other fields, as documented below.
Note that future versions of this specification will likely
@@ -1369,10 +1368,13 @@ following sections.
\subsection{MMIO Device Discovery}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Discovery}
-Unlike PCI, MMIO provides no generic device discovery. For
-systems using Flattened Device Trees the suggested format is:
+Unlike PCI, MMIO provides no generic device discovery. For each
+device, the guest OS will need to know the location of the registers
+and interrupt(s) used. The suggested binding for systems using
+flattened device trees is shown in this example:
\begin{lstlisting}
+ // EXAMPLE: virtio_block device taking 256 bytes at 0x1e000, interrupt 42.
virtio_block@1e000 {
compatible = "virtio,mmio";
reg = <0x1e000 0x100>;
@@ -1941,7 +1943,7 @@ revision & length & data & remarks \\
\hline
\end{tabular}
-Note that a change in the virtio standard does not neccessarily
+Note that a change in the virtio standard does not necessarily
correspond to a change in the virtio-ccw revision.
A device MUST post a unit check with command reject for any revision
@@ -2037,7 +2039,7 @@ and align the alignment.
\subsubsection{Virtqueue Layout}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Virtqueue Layout}
-The virtqueue is physically contiguous, with padded added to make the
+The virtqueue is physically contiguous, with padding added to make the
used ring meet the align value:
\begin{tabular}{|l|l|l|}
Subject: [PATCH 1/1] virtio-ccw: clarifications
- further qualify bit numbering
- specify summary indicator contents
- drop early printk spec; it is only a hack that was never used upstream
Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
content.tex | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/content.tex b/content.tex
index 803615d..0a94c07 100644
--- a/content.tex
+++ b/content.tex
@@ -2174,7 +2174,8 @@ summary_indicator contains the guest address of the 8 bit summary
indicator.
indicator contains the guest address of an area wherin the indicators
for the devices are contained, starting at bit_nr, one bit per
-virtqueue of the device. Bit numbers start at the left.
+virtqueue of the device. Bit numbers start at the left, i.e. the most
+significant bit in the first byte is assigned the bit number 0.
isc contains the I/O interruption subclass to be used for the adapter
I/O interrupt. It may be different from the isc used by the proxy
virtio-ccw device's subchannel.
@@ -2224,7 +2225,7 @@ host->guest notification about virtqueue activity.
For notifying the driver of virtqueue buffers, the device sets the
bit in the guest-provided indicator area at the corresponding offset.
-The guest-provided summary indicator is also set. An adapter I/O
+The guest-provided summary indicator is set to 0x01. An adapter I/O
interrupt for the corresponding interruption subclass is generated.
The device SHOULD only generate an adapter I/O interrupt if the
summary indicator had not been set prior to notification. The driver
@@ -2273,11 +2274,6 @@ should be passed in GPR4 for the next notification:
info->cookie);
\end{lstlisting}
-\subsubsection{Early printk for Virtio Consoles}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Operation / Early printk for Virtio Consoles}
-
-For the early printk mechanism, diagnose 0x500 with subcode 0 is
-used.
-
\subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Operation / Resetting Devices}
In order to reset a device, a driver sends the
|
