diff options
Diffstat (limited to 'feedback')
| -rw-r--r-- | feedback/1.txt | 152 | ||||
| -rw-r--r-- | feedback/2.txt | 221 |
2 files changed, 373 insertions, 0 deletions
diff --git a/feedback/1.txt b/feedback/1.txt new file mode 100644 index 0000000..114955b --- /dev/null +++ b/feedback/1.txt @@ -0,0 +1,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: + diff --git a/feedback/2.txt b/feedback/2.txt new file mode 100644 index 0000000..59b4e76 --- /dev/null +++ b/feedback/2.txt @@ -0,0 +1,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 |