Merge together all feedback from Arun.

Signed-off-by: Rusty Russell <rusty@au1.ibm.com>

git-svn-id: https://tools.oasis-open.org/version-control/svn/virtio@227 0c8fb4dd-22a2-4bb5-bc14-6c75a5f43652

1 files changed, 258 insertions, 0 deletions

diff --git a/feedback/8.txt b/feedback/8.txt
new file mode 100644
index 0000000..870953e
--- /dev/null
+++ b/feedback/8.txt
@@ -0,0 +1,258 @@
+Document: virtio-v1.0-csprd01
+Number: 8
+Date: Wed, 29 Jan 2014 17:05:06 -0800
+Link to Mail: https://lists.oasis-open.org/archives/virtio-comment/201401/msg00058.html
+Commenter name: Arun Subbarao <asubbarao@lnxw.com>
+Decision: 
+
+1) 3.2.1.4 Notifying The Device:
+"We" is confusing and ambiguous. Please use either "device" or "driver" instead of "we" everywhere.
+
+2) 3.2.2 Receiving Used Buffers From The Device:
+Is it intentional that the if statement body doesn't disable the interrupts again and the loop continues running with interrupts enabled?
+
+3) 4.1.2 PCI Device Layout
+It would be nice to know if there is any guidance on the standard PCI register implementation, such as the PCI Command and Status registers. Even if there is none, the document should mention it, rather than leave the developer guessing if something would break, e.g. if they don't implement the Interrupt Status bit in the Status register.
+
+4) 4.1.2.1 Common configuration structure layout:
+I'm confused. Is this the "virtio header" mentioned later in the text? If yes, please use a single term throughout the document to avoid confusion. If not, then what is a "virtio header"? Searching for that term doesn't find a definition.
+
+5) Any alignment requirements for these three fields? Whether or not there are any, please state so explicitly.
+
+6) 4.1.2.2 ISR status structure layout
+It would be helpful if this section provided the meaning of each bit in the register.
+
+7) 4.1.3.1.1 Virtio Device Configuration Layout Detection
+The double "and" on the first line makes the first two sentences ambiguous. Suggest to rephrase as: "Virtio Device Configuration Layout includes the following structures: virtio configuration header, Notification, ISR Status, device configuration."
+
+8) By the way, this is the only occurrence of the word combination "virtio configuration header" in the document. Makes trying to find what it is impossible.
+
+9) (offset) Any alignment requirements? E.g. is it OK to align structures at an odd offset? Whether there are or aren't any alignment requirements, please state so explicitly.
+
+10) 4.1.3.1.2 Queue Vector Configuration
+Some of the information from section 8.4 needs to be moved to here, for example that the device may have an MSI-X table size other than 2048. Otherwise, this reads as though the MSI-X table must always have 2048 entries.
+
+11) Please explicitly describe the device behavior when writing a vector value beyond the MSI-X table size. 
+
+12) 4.1.3.4 Notification of Device Configuration Changes
+Please use "PCI configuration space" and "device configuration state" consistently, without abbreviation. For example, from the first sentence it looks like "device configuration state" can be changed, but the first bullet claims it's "configuration space". So, which one? Does "configuration space" mean "PCI configuration space" or is it a synonym for "device configuration state"? Because those are two different things; the driver needs to know what exactly to rescan.
+
+13) Another thing not entirely clear: does the driver need to do anything if it read the "virtio header" (whatever it is) from a BAR rather than PCI config space?
+
+Proposal(s):
+
+diff --git a/content.tex b/content.tex
+index 2adc393..d6c2591 100644
+--- a/content.tex
++++ b/content.tex
+@@ -760,6 +760,8 @@ for (;;) {
+ 
+                 if (vq->last_seen_used != le16_to_cpu(vring->used.idx))
+                         break;
++
++                vring_disable_interrupts(vq);
+         }
+ 
+         struct vring_used_elem *e = vring.used->ring[vq->last_seen_used%vsz];
+@@ -797,6 +799,17 @@ into virtio general and bus-specific sections.
+ 
+ Virtio devices are commonly implemented as PCI devices.
+ 
++A Virtio device can be implemented as any kind of PCI device:
++a Conventional PCI device, a PCI-X device or a PCI Express
++device.  A Virtio device using Virtio Over PCI Bus MUST expose to
++guest an interface that meets the specification requirements of
++the appropriate PCI specification: \hyperref[intro:PCI]{[PCI]},
++\hyperref[intro:PCI-X]{[PCI-X]} and \hyperref[intro:PCIe]{[PCIe]}
++respectively.  To assure designs meet the latest level
++requirements, designers of Virtio Over PCI devices must refer to
++the PCI-SIG home page at \url{http://www.pcisig.com} for any
++approved changes.
++
+ \subsection{PCI Device Discovery}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}
+ 
+ Any PCI device with Vendor ID 0x1AF4, and Device ID 0x1000 through
+@@ -830,7 +843,13 @@ MUST access each field using the “natural” access method (i.e. 32-bit access
+ 
+ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Capabilities}
+ 
+-The virtio device configuration layout includes a common configuration header, notification area, ISR status area and a device-specific configuration area.
++The virtio device configuration layout includes several structures:
++\begin{item}
++\item Common configuration
++\item Notifications
++\item ISR Status
++\item Device-specific configuration (optional)
++\end{item}
+ 
+ Each structure can be mapped by a Base Address register (BAR) belonging to
+ the function, or accessed via the special VIRTIO_PCI_CAP_PCI_CFG field in the PCI configuration space.
+@@ -865,7 +884,7 @@ The fields are interpreted as follows:
+         0x09; Identifies a vendor-specific capability.
+ 
+ \item[\field{cap_next}]
+-        Link to next capability in the capability list in the configuration space.
++        Link to next capability in the capability list in the PCI configuration space.
+ 
+ \item[\field{cap_len}]
+         Length of this capability structure, including the whole of
+@@ -907,7 +926,7 @@ The fields are interpreted as follows:
+ 
+ \item[\field{bar}]
+         values 0x0 to 0x5 specify a Base Address register (BAR) belonging to
+-        the function located beginning at 10h in Configuration Space
++        the function located beginning at 10h in PCI Configuration Space
+         and used to map the structure into Memory or I/O Space.
+         The BAR is permitted to be either 32-bit or 64-bit, it can map Memory Space
+         or I/O Space.
+@@ -918,7 +937,8 @@ The fields are interpreted as follows:
+ 
+ \item[\field{offset}]
+         indicates where the structure begins relative to the base address associated
+-        with the BAR.
++        with the BAR.  The alignment requirement of \field{offset} are indicated
++        in each structure-specific section below.
+ 
+ \item[\field{length}]
+         indicates the length of the structure.
+@@ -942,6 +962,7 @@ The fields are interpreted as follows:
+ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
+ 
+ The common configuration structure is found at the \field{bar} and \field{offset} within the VIRTIO_PCI_CAP_COMMON_CFG capability; its layout is below.
++\field{offset} must be 4-byte aligned.
+ 
+ The device MUST present at least one common configuration capability.
+ 
+@@ -1034,13 +1055,13 @@ struct virtio_pci_common_cfg {
+         Note: this is *not* an offset in bytes. See \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} below.
+ 
+ \item[\field{queue_desc}]
+-        The driver writes the physical address of Descriptor Table here.
++        The driver writes the physical address of Descriptor Table here.  See section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
+ 
+ \item[\field{queue_avail}]
+-        The driver writes the physical address of Available Ring here.
++        The driver writes the physical address of Available Ring here.  See section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
+ 
+ \item[\field{queue_used}]
+-        The driver writes the physical address of Used Ring here.
++        The driver writes the physical address of Used Ring here.  See section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
+ \end{description}
+ 
+ \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
+@@ -1048,7 +1069,7 @@ struct virtio_pci_common_cfg {
+ The device MUST present at least one notification capability.
+ 
+ The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
+-capability.  This capability is immediately followed by an additional
++capability.  The \field{offset} must be 2-byte aligned.  This capability is immediately followed by an additional
+ field, like so:
+ 
+ \begin{lstlisting}
+@@ -1081,13 +1102,23 @@ Queue Notify address.
+ \subsubsection{ISR status capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / ISR status capability}
+ 
+ The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.  This
+-refers to at least a single byte, which contains the 8-bit ISR status field.
++refers to at least a single byte, which contains the 8-bit ISR status field:
++\begin{lstlisting}
++#define VIRTIO_PCI_ISR_VQ       0x1
++#define VIRTIO_PCI_ISR_CONFIG   0x2
++\end{lstlisting}
++
++See sections \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Virtqueue Interrupts From The Device} and \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes} for how this is used.
++
++The \field{offset} for the ISR status has no specific alignment requirements.
+ 
+ \subsubsection{Device specific structure}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Device specific structure}
+ 
+ The device MAY present at least one VIRTIO_PCI_CAP_DEVICE_CFG capability (some
+ devices may not have any device specific structure).
+ 
++The \field{offset} for the device specific structure must be 4-byte aligned.
++
+ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
+ 
+ The device MUST present at least one VIRTIO_PCI_CAP_PCI_CFG.  This
+@@ -1132,18 +1163,18 @@ registers in a legacy configuration structure in BAR0 in the first I/O
+ region of the PCI device, as documented below.
+ 
+ There may be different widths of accesses to the I/O region; the
+-“natural” access method for each field in the virtio header must be
++“natural” access method for each field in the virtio common configuration structure must be
+ used (i.e. 32-bit accesses for 32-bit fields, etc), but 
+ when accessed through the legacy interface the
+ device-specific region can be accessed using any width accesses, and
+ should obtain the same results.
+ 
+-Note that this is possible because while the virtio header is PCI
++Note that this is possible because while the virtio common configuration structure is PCI
+ (i.e. little) endian, when using the legacy interface the device-specific
+ region is encoded in the native endian of the guest (where such distinction is
+ applicable).
+ 
+-When used through the legacy interface, the virtio header looks as follows:
++When used through the legacy interface, the virtio common configuration structure looks as follows:
+ 
+ \begin{tabularx}{\textwidth}{ |X||X|X|X|X|X|X|X|X| }
+ \hline
+@@ -1171,7 +1202,7 @@ Purpose (MSI-X) & \field{config_msix_vector}  & \field{queue_msix_vector} \\
+ \end{tabular}
+ 
+ Note: When MSI-X capability is enabled, device specific configuration starts at
+-byte offset 24 in virtio header structure. When MSI-X capability is not
++byte offset 24 in virtio common configuration structure structure. When MSI-X capability is not
+ enabled, device specific configuration starts at byte offset 20 in virtio
+ header.  ie. once you enable MSI-X on the device, the other fields move.
+ If you turn it off again, they move back!
+@@ -1203,7 +1234,7 @@ see \ref{sec:Basic Facilities of a Virtio Device / Configuration Space / Legacy
+ 
+ This documents PCI-specific steps executed during Device Initialization.
+ As the first step, driver must detect device configuration layout
+-to locate configuration fields in memory, I/O or configuration space of the
++to locate configuration fields in memory, I/O or PCI configuration space of the
+ device.
+ 
+ \paragraph{Virtio Device Configuration Layout Detection}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtio Device Configuration Layout Detection}
+@@ -1215,7 +1246,7 @@ Structure PCI capabilities.
+ \subparagraph{Legacy Interface: A Note on Device Layout Detection}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtio Device Configuration Layout Detection / Legacy Interface: A Note on Device Layout Detection}
+ 
+ Legacy drivers skipped the Device Layout Detection step, assuming legacy
+-configuration space in BAR0 in I/O space unconditionally.
++device registers in BAR0 in I/O space unconditionally.
+ 
+ Legacy devices did not have the Virtio PCI Capability in their
+ capability list.
+@@ -1361,8 +1392,7 @@ The driver interrupt handler SHOULD:
+ 
+ \subsubsection{Notification of Device Configuration Changes}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+ 
+-Some virtio PCI devices can change the device configuration
+-state, as reflected in the device-specific region of the device. In this case:
++Some devices can change the device configuration space. In this case:
+ 
+ \begin{itemize}
+   \item If MSI-X capability is disabled: an interrupt is delivered and
+diff --git a/introduction.tex b/introduction.tex
+index 8e08d02..745fabf 100644
+--- a/introduction.tex
++++ b/introduction.tex
+@@ -43,6 +43,18 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S
+ 	\phantomsection\label{intro:rfc2119}\textbf{[RFC2119]} & S. Bradner, Key words for use in RFCs to Indicate Requirement Levels, \newline\url{http://www.ietf.org/rfc/rfc2119.txt}, March 1997\\
+ 	\phantomsection\label{intro:S390 PoP}\textbf{[S390 PoP]} & z/Architecture Principles of Operation, \newline IBM Publication SA22-7832\\
+ 	\phantomsection\label{intro:S390 Common I/O}\textbf{[S390 Common I/O]} & ESA/390 Common I/O-Device and Self-Description, \newline IBM Publication SA22-7204\\
++	\phantomsection\label{intro:PCI}\textbf{[PCI]} &
++	Conventional PCI Specifications,
++	\newline\url{http://www.pcisig.com/specifications/conventional/},
++	PCI-SIG\\
++	\phantomsection\label{intro:PCI-X}\textbf{[PCI-X]} &
++	PCI-X Specifications,
++	\newline\url{http://www.pcisig.com/specifications/pcix_20/},
++	PCI-SIG\\
++	\phantomsection\label{intro:PCI-X}\textbf{[PCIe]} &
++	PCI Express Specifications
++	\newline\url{http://www.pcisig.com/specifications/pciexpress/},
++	PCI-SIG\\
+ \end{longtable}
+ 
+ \section{Structure Specifications}