[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Feedback for chapter 5
Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> On Fri, 07 Feb 2014 11:08:37 +1030
> Rusty Russell <rusty@au1.ibm.com> wrote:
>
>> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>> > Here's my review for Virtio draft 01, chapter 05:
> ...
>> > 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.
>>
>> I've added this to the bottom. Hopefully it captures the correct
>> "Abandon hope ye who enter here" without actually saying it:
>>
>> 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.
>
> s/Be warned that may/Be warned that they may/ ?
>
> Otherwise sounds fine to me.
Changed, thanks.
> Ok, seems like there are different versions of draft 01 floating
> around? In my version, page 55 is about the SCSI host device, so I was
> referring to the struct virtio_scsi_req_cmd.
No, I've probably got updates in mine from previous feedback which
altered numbers.
> And with chapter 4.1.3.1.1, I was referring to the description before
> struct virtio_pci_cap:
>
> "This virtio structure capability uses little-endian format; all bits
> are read-only"
>
> ==> This "read-only" refers to the driver, not the device, right?
Agreed. Made explicit:
This virtio structure capability uses little-endian format; all bits are
read-only for the driver:
>> OK, here's the suggested result:
>
> Looks mostly fine to me, apart from one question about
> VIRTIO_CONSOLE_DEVICE_READY (see below). And maybe you could also fix the
> comment before "struct virtio_pci_cap" that I've mentioned above?
>
>> diff --git a/content.tex b/content.tex
>> index 48c2d8e..f9a83b6 100644
>> --- a/content.tex
>> +++ b/content.tex
> [...]
>> @@ -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.
>
> There is something wrong in the above definitions ... you specified
> VIRTIO_CONSOLE_DEVICE_READY twice, as 0 and 2. And what about the REMOVE event
> that was originally specified here, too?
Yes, re-worked again to match numbering, fix DEVICE_READY (should be
PORT_READY) and add PORT_REMOVE:
Here's the resulting de-TeXified text for easier reading:
The values for event are:
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.
VIRTIO_CONSOLE_DEVICE_ADD (1): Sent by the device, to create a new
port. The device MUST NOT specify a port which exists. value is unused.
VIRTIO_CONSOLE_DEVICE_REMOVE (2): Sent by the device, to remove an
existing port. The device MUST NOT specify a port which has not been
created with VIRTIO_CONSOLE_DEVICE_ADD. value is unused.
VIRTIO_CONSOLE_PORT_READY (3): 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.
VIRTIO_CONSOLE_CONSOLE_PORT (4): 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. The driver MUST set value to 1.
VIRTIO_CONSOLE_RESIZE (5): Sent by the device to indicate
a console size change. value is unused. The buffer is followed by the number of columns and rows:
struct virtio_console_resize {
le16 cols;
le16 rows;
};
VIRTIO_CONSOLE_PORT_OPEN (6): This message is sent by both the
device and the driver. value 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.
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).
Thanks,
Rusty.
===
diff --git a/content.tex b/content.tex
index 2eb0b26..4a5e868 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}
@@ -574,8 +577,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.
@@ -620,9 +623,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:
@@ -632,7 +635,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}
@@ -707,7 +710,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
@@ -842,20 +845,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 */
@@ -1044,7 +1047,7 @@ or accessed though PCI configuration space.
Actual location of each structure is specified using vendor-specific PCI capability located
on capability list in PCI configuration space of the device.
This virtio structure capability uses little-endian format; all bits are
-read-only:
+read-only for the driver:
\begin{lstlisting}
struct virtio_pci_cap {
@@ -2342,6 +2345,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 they 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
@@ -2753,7 +2763,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
@@ -3077,7 +3087,7 @@ struct virtio_blk_req {
le32 type;
le32 reserved;
le64 sector;
- char data[][512];
+ u8 data[][512];
u8 status;
};
\end{lstlisting}
@@ -3142,8 +3152,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;
@@ -3169,8 +3179,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,
@@ -3189,12 +3199,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.
@@ -3229,7 +3239,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}
@@ -3291,7 +3302,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
@@ -3317,22 +3328,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 {
@@ -3340,18 +3350,45 @@ 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
+The values for \field{event} are:
+\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. \field{value} is unused.
+\item [VIRTIO_CONSOLE_DEVICE_REMOVE (2)] Sent by the device, to remove an
+ existing port. The device MUST NOT specify a port which has not been
+ created with VIRTIO_CONSOLE_DEVICE_ADD. \field{value} is unused.
+\item [VIRTIO_CONSOLE_PORT_READY (3)] 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 \field{value} of 1 indicates success, and 0
+ indicates failure.
+\item [VIRTIO_CONSOLE_CONSOLE_PORT (4)] 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. The driver MUST set \field{value} to 1.
+\item [VIRTIO_CONSOLE_RESIZE (5)] Sent by the device to indicate
+ a console size change. \field{value} is unused. The buffer 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. \field{value} 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
@@ -3600,7 +3637,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.
@@ -3700,22 +3737,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[];
};
@@ -3756,10 +3793,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
@@ -3861,12 +3898,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;
}
@@ -3898,11 +3935,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;
}
@@ -3933,11 +3970,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;
}
@@ -3992,7 +4029,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;
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]