diff options
Diffstat (limited to 'feedback/2.txt')
| -rw-r--r-- | feedback/2.txt | 222 |
1 files changed, 0 insertions, 222 deletions
diff --git a/feedback/2.txt b/feedback/2.txt deleted file mode 100644 index e02264a..0000000 --- a/feedback/2.txt +++ /dev/null @@ -1,222 +0,0 @@ -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> -Decision: Agreed at meeting 2014-01-28. - -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 |