summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--feedback/6.txt467
1 files changed, 467 insertions, 0 deletions
diff --git a/feedback/6.txt b/feedback/6.txt
new file mode 100644
index 0000000..b17da69
--- /dev/null
+++ b/feedback/6.txt
@@ -0,0 +1,467 @@
+Document: virtio-v1.0-csprd01
+Number: 6
+Date: Wed, 29 Jan 2014 21:36:30 +0100
+Link to Mail: https://lists.oasis-open.org/archives/virtio-comment/201401/msg00055.html
+Commenter name: Thomas Huth <thuth@linux.vnet.ibm.com>
+Decision:
+
+Generic remark: Quite a lot of structures use a mixture of "u8"
+and "char" types for bytes, e.g. struct virtio_blk_req. Is that
+intended? If not, I'd suggest to always use "u8" here instead.
+
+
+Page 38 / Device Types:
+
+Is the information about other virtio devices like 9P documented
+somewhere else? If yes, you might want to put a pointer to these
+documents here.
+
+
+Page 43 and page 44:
+
+Some of the #defines are indented with an additional space.
+That looks a little bit ugly, would be great if you could get rid
+of the indentation here.
+
+
+Page 43 / Setting MAC Address Filtering
+
+ETH_ALEN is a Linux-internal #define only, which is not defined
+in this spec, so I'd suggest to simply replace ETH_ALEN with 6 here.
+
+
+Page 48 / Virtqueues
+
+The initial numbered list names the second pair of console virtqueues
+"port1" but the next sentence talks about "Port 2" ... that's confusing.
+==> Maybe replace "Ports 2 onwards only exist ..." with something like
+"All ports except the first one only exist ..." ?
+
+
+Page 50:
+
+The paragraph "5." says:
+"If the device specified a port `name', a sysfs attribute is created with
+the name filled in, so that udev rules can be written that can create a
+symlink from the port's name to the char device for port discovery by
+applications in the driver."
+==> That's completely specific to Linux as far as I can tell, so I think
+this should not go into a generic specification document, or at least it
+should be marked as an example for Linux.
+Also, this paragraph talks about the "name" which is not introduced before
+paragraph "6.", so I think if you really want to keep this paragraph "5.",
+it should be moved behind "6." instead.
+
+Concerning paragraph "6.":
+The console port change events are hardly documented ... I think this
+paragraph needs some love. How does the "value" look like for the
+various message types? For example, how is the size of the console
+passed when the VIRTIO_CONSOLE_RESIZE event occured? Or the name? What
+is the difference between VIRTIO_CONSOLE_PORT_ADD and
+VIRTIO_CONSOLE_PORT_OPEN?
+
+As a further remark, I also I wonder whether should be a way to signal
+the terminal type (like "vt100") to the guest?
+
+
+Page 55 / Device Operation: Request Queues
+
+While reading this chapter, I first got a little bit confused about the
+terms "read-only" and "write-only" since I read chapter 4.1.3.1.1
+("Virtio Device Configuration Layout Detection") shortly before, where
+the terms are used in the opposite way - since "read-only" and
+"write-only" are dependend on the view, whether you talk about the device
+or the driver.
+So even it's clear when you read the various chapters twice and think
+about everything logically, it might be more consisten and easier to
+read if you always say something more explicit like "read-only for the
+device" or "read-only for the driver" throughout the specification.
+
+Proposal:
+
+diff --git a/content.tex b/content.tex
+index 48c2d8e..f9a83b6 100644
+--- a/content.tex
++++ b/content.tex
+@@ -289,7 +289,7 @@ struct vring {
+ struct vring_avail avail;
+
+ // Padding to the next PAGE_SIZE boundary.
+- char pad[ Padding ];
++ u8 pad[ Padding ];
+
+ // A ring of used descriptor heads with free-running index.
+ struct vring_used used;
+@@ -337,8 +337,11 @@ VIRTIO_F_ANY_LAYOUT feature is accepted.
+ The descriptor table refers to the buffers the driver is using for
+ the device. The addresses are physical addresses, and the buffers
+ can be chained via the next field. Each descriptor describes a
+-buffer which is read-only or write-only, but a chain of
+-descriptors can contain both read-only and write-only buffers.
++buffer which is read-only for the device (``device-readable'') or write-only for the device (``device-writable''), but a chain of
++descriptors can contain both device-readable and device-writable buffers.
++A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT
++read a device-writable buffer (it might do so for debugging or diagnostic
++purposes).
+
+ The actual contents of the memory offered to the device depends on the
+ device type. Most common is to begin the data with a header
+@@ -357,7 +360,7 @@ struct vring_desc {
+
+ /* This marks a buffer as continuing via the next field. */
+ #define VRING_DESC_F_NEXT 1
+-/* This marks a buffer as write-only (otherwise read-only). */
++/* This marks a buffer as device write-only (otherwise device read-only). */
+ #define VRING_DESC_F_WRITE 2
+ /* This means the buffer contains a list of buffer descriptors. */
+ #define VRING_DESC_F_INDIRECT 4
+@@ -403,7 +406,7 @@ chained by next field. An indirect descriptor without next field
+ An
+ indirect descriptor can not refer to another indirect descriptor
+ table (flags\&VRING_DESC_F_INDIRECT MUST be off). A single indirect descriptor
+-table can include both read-only and write-only descriptors;
++table can include both device-readable and device-writable descriptors;
+ the device MUST ignore the write-only flag (flags\&VRING_DESC_F_WRITE) in the descriptor that refers to it.
+
+ \subsection{The Virtqueue Available Ring}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}
+@@ -570,8 +573,8 @@ There are two parts to device operation: supplying new buffers to
+ the device, and processing used buffers from the device. As an
+ example, the simplest virtio network device has two virtqueues: the
+ transmit virtqueue and the receive virtqueue. The driver adds
+-outgoing (read-only) packets to the transmit virtqueue, and then
+-frees them after they are used. Similarly, incoming (write-only)
++outgoing (device-readable) packets to the transmit virtqueue, and then
++frees them after they are used. Similarly, incoming (device-writable)
+ buffers are added to the receive virtqueue, and processed after
+ they are used.
+
+@@ -616,9 +619,9 @@ Here is a description of each stage in more detail.
+
+ \subsubsection{Placing Buffers Into The Descriptor Table}\label{sec:General Initialization And Device Operation / Device Operation / Supplying Buffers to The Device / Placing Buffers Into The Descriptor Table}
+
+-A buffer consists of zero or more read-only physically-contiguous
++A buffer consists of zero or more device-readable physically-contiguous
+ elements followed by zero or more physically-contiguous
+-write-only elements (it must have at least one element). This
++device-writable elements (it must have at least one element). This
+ algorithm maps it into the descriptor table to form a descriptor
+ chain:
+
+@@ -628,7 +631,7 @@ for each buffer element, b:
+ \item Get the next free descriptor table entry, d
+ \item Set d.addr to the physical address of the start of b
+ \item Set d.len to the length of b.
+-\item If b is write-only, set d.flags to VRING_DESC_F_WRITE,
++\item If b is device-writable, set d.flags to VRING_DESC_F_WRITE,
+ otherwise 0.
+ \item If there is a buffer element after this:
+ \begin{enumerate}
+@@ -703,7 +706,7 @@ notification:
+
+ \subsection{Receiving Used Buffers From The Device}\label{sec:General Initialization And Device Operation / Device Operation / Receiving Used Buffers From The Device}
+
+-Once the device has used a buffer (read from or written to it, or
++Once the device has used buffers referred to by a descriptor (read from or written to them, or
+ parts of both, depending on the nature of the virtqueue and the
+ device), it SHOULD send an interrupt, following an algorithm very
+ similar to the algorithm used for the driver to send the device a
+@@ -825,20 +828,20 @@ Common configuration structure layout is documented below:
+ struct virtio_pci_common_cfg {
+ /* About the whole device. */
+ le32 device_feature_select; /* read-write */
+- le32 device_feature; /* read-only */
++ le32 device_feature; /* read-only for driver */
+ le32 driver_feature_select; /* read-write */
+ le32 driver_feature; /* read-write */
+ le16 msix_config; /* read-write */
+- le16 num_queues; /* read-only */
++ le16 num_queues; /* read-only for driver */
+ u8 device_status; /* read-write */
+- u8 config_generation; /* read-only */
++ u8 config_generation; /* read-only for driver */
+
+ /* About a specific virtqueue. */
+ le16 queue_select; /* read-write */
+ le16 queue_size; /* read-write, power of 2, or 0. */
+ le16 queue_msix_vector; /* read-write */
+ le16 queue_enable; /* read-write */
+- le16 queue_notify_off; /* read-only */
++ le16 queue_notify_off; /* read-only for driver */
+ le64 queue_desc; /* read-write */
+ le64 queue_avail; /* read-write */
+ le64 queue_used; /* read-write */
+@@ -2325,6 +2328,13 @@ Device ID & Virtio Device \\
+ \hline
+ \end{tabular}
+
++Some of the devices above are unspecified by this document,
++because they are seen as immature or especially niche. Be warned
++that may only be specified by the sole existing implementation;
++they may become part of a future specification, be abandoned
++entirely, or live on outside this standard. We shall speak of
++them no further.
++
+ \section{Network Device}\label{sec:Device Types / Network Device}
+
+ The virtio network device is a virtual ethernet card, and is the
+@@ -2736,7 +2746,7 @@ off. The command-specific-data is one byte containing 0 (off) or
+ \begin{lstlisting}
+ struct virtio_net_ctrl_mac {
+ le32 entries;
+- u8 macs[entries][ETH_ALEN];
++ u8 macs[entries][6];
+ };
+
+ #define VIRTIO_NET_CTRL_MAC 1
+@@ -3060,7 +3070,7 @@ struct virtio_blk_req {
+ le32 type;
+ le32 reserved;
+ le64 sector;
+- char data[][512];
++ u8 data[][512];
+ u8 status;
+ };
+ \end{lstlisting}
+@@ -3125,8 +3135,8 @@ struct virtio_scsi_pc_req {
+ u32 type;
+ u32 ioprio;
+ u64 sector;
+- char cmd[];
+- char data[][512];
++ u8 cmd[];
++ u8 data[][512];
+ #define SCSI_SENSE_BUFFERSIZE 96
+ u8 sense[SCSI_SENSE_BUFFERSIZE];
+ u32 errors;
+@@ -3152,8 +3162,8 @@ single, separate read-only buffer; command length can be derived
+ from the length of this buffer.
+
+ Note that these first three (four for scsi packet commands)
+-fields are always read-only: the data field is either read-only
+-or write-only, depending on the request. The size of the read or
++fields are always device-readable: the data field is either device-readable
++or device-writable, depending on the request. The size of the read or
+ write can be derived from the total size of the request buffers.
+
+ The sense field is only present for scsi packet command requests,
+@@ -3172,12 +3182,12 @@ requests and indicates the residual size, calculated as data
+ length - number of bytes actually transferred.
+
+ Historically, devices assumed that the fields type, ioprio and
+-sector reside in a single, separate read-only buffer; the fields
++sector reside in a single, separate device-readable buffer; the fields
+ errors, data_len, sense_len and residual reside in a single,
+-separate write-only buffer; the sense field in a separate
++separate device-writable buffer; the sense field in a separate
+ write-only buffer of size 96 bytes, by itself; the fields errors,
+-data_len, sense_len and residual in a single write-only buffer;
+-and the status field is a separate write-only buffer of size 1
++data_len, sense_len and residual in a single device-writable buffer;
++and the status field is a separate device-writable buffer of size 1
+ byte, by itself.
+
+
+@@ -3212,7 +3222,8 @@ data and outgoing characters are placed in the transmit queue.
+ \item[\ldots]
+ \end{description}
+
+- Ports 2 onwards only exist if VIRTIO_CONSOLE_F_MULTIPORT is set.
++The port 0 receive and transmit queues always exist: other queues
++only exist if VIRTIO_CONSOLE_F_MULTIPORT is set.
+
+ \subsection{Feature bits}\label{sec:Device Types / Console Device / Feature bits}
+
+@@ -3274,7 +3285,7 @@ native endian of the guest rather than (necessarily) little-endian.
+ control messages for adding new ports to the device. After
+ creating and initializing each port, a
+ VIRTIO_CONSOLE_PORT_READY control message is sent to the device
+- for that port so the device can let us know of any additional
++ for that port so the device can let the driver know of any additional
+ configuration options set for that port.
+
+ \item The receiveq for each port is populated with one or more
+@@ -3300,22 +3311,21 @@ when a port is closed or hot-unplugged.
+
+ \item If the driver negotiated the VIRTIO_CONSOLE_F_SIZE feature, a
+ configuration change interrupt may occur. The updated size can
+- be read from the configuration fields.
++ be read from the configuration fields. This size applies to port 0 only.
+
+ \item If the driver negotiated the VIRTIO_CONSOLE_F_MULTIPORT
+ feature, active ports are announced by the device using the
+ VIRTIO_CONSOLE_PORT_ADD control message. The same message is
+ used for port hot-plug as well.
++\end{enumerate}
+
+-\item If the device specified a port `name', a sysfs attribute is
+- created with the name filled in, so that udev rules can be
+- written that can create a symlink from the port's name to the
+- char device for port discovery by applications in the driver.
++\subsubsection{Multiport Device Operation}\label{sec:Device Types / Console Device / Device Operation / Multiport Device Operation}
+
+-\item Changes to ports' state are effected by control messages.
+- Appropriate action is taken on the port indicated in the
+- control message. The layout of the structure of the control
+- buffer and the events associated are:
++If the driver negotiated the VIRTIO_CONSOLE_F_MULTIPORT, the two
++control queues are used to manipulate the different console ports: the
++control receiveq for messages from the device to the driver, and the
++control sendq for driver-to-device messages. The layout of the
++control messages is:
+
+ \begin{lstlisting}
+ struct virtio_console_control {
+@@ -3323,18 +3333,41 @@ struct virtio_console_control {
+ le16 event; /* The kind of control event */
+ le16 value; /* Extra information for the event */
+ };
++\end{lstlisting}
+
+-/* Some events for the internal messages (control packets) */
+-#define VIRTIO_CONSOLE_DEVICE_READY 0
+-#define VIRTIO_CONSOLE_PORT_ADD 1
+-#define VIRTIO_CONSOLE_PORT_REMOVE 2
+-#define VIRTIO_CONSOLE_PORT_READY 3
+-#define VIRTIO_CONSOLE_CONSOLE_PORT 4
+-#define VIRTIO_CONSOLE_RESIZE 5
+-#define VIRTIO_CONSOLE_PORT_OPEN 6
+-#define VIRTIO_CONSOLE_PORT_NAME 7
++\begin{description}
++\item [VIRTIO_CONSOLE_DEVICE_READY (0)] Sent by the driver at initialization
++ to indicate that it is ready to receive control messages. A value of
++ 1 indicates success, and 0 indicates failure. The port number is unused.
++\item [VIRTIO_CONSOLE_DEVICE_ADD (1)] Sent by the device, to create a new
++ port. The device MUST NOT specify a port which exists. event is unused.
++\item [VIRTIO_CONSOLE_DEVICE_READY (2)] Sent by the driver in response
++ to the device's VIRTIO_CONSOLE_PORT_ADD message, to indicate that
++ the port is ready to be used. A value of 1 indicates success, and 0
++ indicates failure.
++\item [VIRTIO_CONSOLE_CONSOLE_PORT (3)] Sent by the device to nominate
++ a port as a console port. There may be more than one console port.
++ The driver SHOULD treat the port in a manner suitable for text
++ console access; the driver MUST respond with a VIRTIO_CONSOLE_PORT_OPEN
++ message.
++\item [VIRTIO_CONSOLE_RESIZE (5)] Sent by the device to indicate
++ a console size change. This is followed by the number of columns and rows:
++\begin{lstlisting}
++struct virtio_console_resize {
++ le16 cols;
++ le16 rows;
++};
+ \end{lstlisting}
+-\end{enumerate}
++\item [VIRTIO_CONSOLE_PORT_OPEN (6)] This message is sent by both the
++ device and the driver. The value field MUST BE set to 0 (port
++ closed) or 1 (port open). This allows for ports to be used directly
++ by guest and host processes to communicate in an application-defined
++ manner.
++\item [VIRTIO_CONSOLE_PORT_NAME (7)] Sent by the device to give a tag
++ to the port. This control command is immediately
++ followed by the UTF-8 name of the port for identification
++ within the guest (without a NUL terminator).
++\end{description}
+
+ \subsubsection{Legacy Interface: Device Operation}\label{sec:Device Types / Console Device / Device Operation / Legacy Interface: Device Operation}
+ For legacy devices, the fields in struct virtio_console_control are the
+@@ -3583,7 +3616,7 @@ targets that receive and process the requests.
+
+ \begin{description}
+ \item[VIRTIO_SCSI_F_INOUT (0)] A single request can include both
+- read-only and write-only data buffers.
++ device-readable and device-writable data buffers.
+
+ \item[VIRTIO_SCSI_F_HOTPLUG (1)] The host should enable
+ hot-plug/hot-unplug of new LUNs and targets on the SCSI bus.
+@@ -3683,22 +3716,22 @@ Requests have the following format:
+
+ \begin{lstlisting}
+ struct virtio_scsi_req_cmd {
+- // Read-only
++ // Device-readable part
+ u8 lun[8];
+ le64 id;
+ u8 task_attr;
+ u8 prio;
+ u8 crn;
+- char cdb[cdb_size];
+- char dataout[];
+- // Write-only part
++ u8 cdb[cdb_size];
++ u8 dataout[];
++ // Device-writable part
+ le32 sense_len;
+ le32 residual;
+ le16 status_qualifier;
+ u8 status;
+ u8 response;
+ u8 sense[sense_size];
+- char datain[];
++ u8 datain[];
+ };
+
+
+@@ -3739,10 +3772,10 @@ value defined by the protocol is 255, since CRN is stored in an
+ 8-bit integer.
+
+ All of these fields are defined in SAM. They are always
+-read-only, as are the cdb and dataout field. The cdb_size is
++device readable, as are the cdb and dataout field. The cdb_size is
+ taken from the configuration space.
+
+-sense and subsequent fields are always write-only. The sense_len
++sense and subsequent fields are always device writable. The sense_len
+ field indicates the number of bytes actually written to the sense
+ buffer. The residual field indicates the residual size,
+ calculated as “data_length - number_of_transferred_bytes”, for
+@@ -3844,12 +3877,12 @@ The following commands are defined:
+
+ struct virtio_scsi_ctrl_tmf
+ {
+- // Read-only part
++ // Device-readable part
+ le32 type;
+ le32 subtype;
+ u8 lun[8];
+ le64 id;
+- // Write-only part
++ // Device-writable part
+ u8 response;
+ }
+
+@@ -3881,11 +3914,11 @@ struct virtio_scsi_ctrl_tmf
+ #define VIRTIO_SCSI_T_AN_QUERY 1
+
+ struct virtio_scsi_ctrl_an {
+- // Read-only part
++ // Device-readable part
+ le32 type;
+ u8 lun[8];
+ le32 event_requested;
+- // Write-only part
++ // Device-writable part
+ le32 event_actual;
+ u8 response;
+ }
+@@ -3916,11 +3949,11 @@ struct virtio_scsi_ctrl_an {
+ #define VIRTIO_SCSI_T_AN_SUBSCRIBE 2
+
+ struct virtio_scsi_ctrl_an {
+- // Read-only part
++ // Device-readable part
+ le32 type;
+ u8 lun[8];
+ le32 event_requested;
+- // Write-only part
++ // Device-writable part
+ le32 event_actual;
+ u8 response;
+ }
+@@ -3975,7 +4008,7 @@ following format:
+ #define VIRTIO_SCSI_T_EVENTS_MISSED 0x80000000
+
+ struct virtio_scsi_event {
+- // Write-only part
++ // Device-writable part
+ le32 event;
+ u8 lun[8];
+ le32 reason;