summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorrusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>2014-01-21 00:15:21 +0000
committerrusty <rusty@0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652>2014-01-21 00:15:21 +0000
commit75f991bc5eae472cdee81bb1b0cc50cc50f09405 (patch)
tree2e12e0124d05617c3f3f601a63fd9e09a84eaf33
parent27b14663e69c182367d02a9751ed579d1d9dbbdc (diff)
Split feedback into multiple files.
Makes it easier to edit/apply individual proposals. Signed-off-by: Rusty Russell <rusty@au1.ibm.com> git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@189 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652
-rw-r--r--feedback/1.txt (renamed from feedback.txt)5
-rw-r--r--feedback/2.txt221
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