[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] VIRTIO Spec feedback
Arun Subbarao <asubbarao@lnxw.com> writes:
> Please consider our feedback on the Virtio spec.
Hi Arun,
Thanks a great deal for the feedback. It's invaluable to have
fresh eyes on the spec, and comments on the PCI part particlarly have
lead to some changes.
The parts of your feedback not already addressed in my previous mail
and by Michael Tsirkin are listed below:
===
> 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.
Indeed, I've proposed we add the following:
diff --git a/content.tex b/content.tex
index 43114b3..104fa55 100644
--- a/content.tex
+++ b/content.tex
@@ -1094,7 +1094,13 @@ 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.
\subsubsection{Device specific structure}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Device specific structure}
===
> 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."
Agreed, in fact I went further and made it an itemized list:
diff --git a/content.tex b/content.tex
index 104fa55..b18003a 100644
--- a/content.tex
+++ b/content.tex
@@ -843,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 structure, the notification area, ISR status register and a device-specific configuration registers.
+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.
===
> 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.
It depends on the structure. I've illuminated the possibilities:
diff --git a/content.tex b/content.tex
index b18003a..4df2513 100644
--- a/content.tex
+++ b/content.tex
@@ -937,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.
@@ -961,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.
@@ -1067,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}
@@ -1108,11 +1110,15 @@ refers to at least a single byte, which contains the 8-bit ISR status field:
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
===
> 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.
I'm leaving these to MST to follow up. My PCI is limited. Michael?
===
> 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.
Indeed.
diff --git a/content.tex b/content.tex
index 4df2513..3fcc5ab 100644
--- a/content.tex
+++ b/content.tex
@@ -884,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
@@ -926,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.
@@ -1234,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}
@@ -1246,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.
@@ -1392,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
===
I've put these patches in the feedback/ directory of the SVN repository,
and we'll vote on them next meeting (Tuesday). I'll mail you and tell
you the results.
Thanks,
Rusty.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]