On Fri, 07 Feb 2014 11:08:37 +1030
Rusty Russell <> wrote:
> Thomas Huth <> 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.
> > 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 ..." ?
>
> Yes, it's wrong. How's this:
>
> The port 0 receive and transmit queues always exist: other queues
> only exist if VIRTIO_CONSOLE_F_MULTIPORT is set.
Sounds good, too!
> > 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?
>
> In fact, this chapter needs a good rewrite. I've worked on that,
> and CC'd Amit if he wants to expand on what I've written.
>
> > 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.
>
> Page 55: this is the balloon device? Can't see the reference to
> read-only and write-only here, am I looking in the wrong place?
>
> Ah, the struct virtio_pci_common_cfg ? Changed each 'read-only'
> to explicitly 'read-only by driver'.
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.
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?
> > 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.
>
> Good feedback. It's a bit tricky, since nothing is read-only for the
> driver (the driver owns the buffers, after all).
Yes, there is nothing read-only for the driver when talking about
buffers. But for example the virtio_pci_common_cfg.device_feature
configuration field is read-only for the driver, too. But I see that
you've addressed this in your patch already.
> Perhaps we should establish the terms "device-read" and
> "device-written", define them to be absolute (ie. R/O and W/O) and use
> them throughout? That's what I've done below.
>
> 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?
Thomas
