OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

# virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-comment] Feedback for chapter 5

• From: Thomas Huth <thuth@linux.vnet.ibm.com>
• To: Rusty Russell <rusty@au1.ibm.com>
• Date: Fri, 7 Feb 2014 10:59:26 +0100

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

==> 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).

buffers. But for example the  virtio_pci_common_cfg.device_feature
configuration field is read-only for the driver, too. But I see that

> 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_PORT_REMOVE      2
> -#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



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]