More feedback from Thomas.

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

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

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;