Re: [virtio-comment] Feedback for chapter 5

[Thomas Huth <thuth@linux.vnet.ibm.com>]

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.

> > 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