diff options
| -rw-r--r-- | feedback/1.txt (renamed from feedback.txt) | 5 | ||||
| -rw-r--r-- | feedback/2.txt | 221 |
2 files changed, 223 insertions, 3 deletions
diff --git a/feedback.txt b/feedback/1.txt index dd47da9..114955b 100644 --- a/feedback.txt +++ b/feedback/1.txt @@ -1,6 +1,4 @@ -COMMENTS FOR virtio-v1.0-csprd01 -================================ - +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 @@ -151,3 +149,4 @@ index 803615d..8850c1a 100644 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 |