Re: [virtio-comment] Feedback for chapter 4

[Rusty Russell <rusty@au1.ibm.com>]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> Here's my feedback for Virtio draft 01, chapter 4:

Thanks again for the feedback!

>
> Page 20 /  PCI Device Layout:
>
> - "To configure the device, use I/O and/or memory regions and/or PCI configuration
>   space of the PCI device."
>   => That sounds a little bit sparse/confusing. Maybe rather something like:
>   "To configure the device, it is possible to use the PCI configuration space
>   and/or to access the configuration data via an I/O and/or MMIO base-address
>   register."

I ended up with this:
        The device is configured via I/O and/or memory regions (though see
        VIRTIO_PCI_CAP_PCI_CFG for access via the PCI configutation space).

        These regions contain the virtio header registers, the notification register, the...

> Page 21:
>
> - The "device_feature_select" and "driver_feature_select" paragraphs are lacking
>   some punctuation marks inbetween.

Indeed, formatting bug.  There's a similar one in "queue_enable".

> Page 23 / Virtio Device Configuration Layout Detection:
>
> - "This structure can optionally followed by extra data"
>   => "This structure can optionally be followed by extra data"

Thanks.

> Page 27 / MMIO Device Discovery:
>
> - The device tree snippet is obviously an example. That's ok, but I think the
>   spec should explicitely say so (and maybe add some generic words about the
>   required properties before the example, too).

OK, I clarified this:

Unlike PCI, MMIO provides no generic device discovery.  For each
device, the guest OS will need to know the location of the registers
and interrupt(s) used.  The suggested binding for Open Firmware is
shown in this example:

        // EXAMPLE: virtio_block device taking 256 bytes at 0x1e000, interrupt 42.
	virtio_block@1e000 {
		compatible = "virtio,mmio";
		reg = <0x1e000 0x100>;
		interrupts = <42>;
	}

> Chapter 4.3.2.*:
>
> - In this chapter, the C-structs are marked with "__attribute__ ((packed));"
>   which is just a GNU-C extension, as far as I know. In the other chapters,
>   the structs are not marked with this string. So for consistency, I'd remove
>   them here, too (and maybe state somewhere at the beginning of the spec
>   that structs are considered to be without compiler padding inbetween)

Well, they do need to be explicitly marked where packing is required,
and that's at least understandable for someone reading the spec.

> Page 33:
>
> - Some typos:
>   neccessarily => necessarily
>   issueing => issuing

Looks like Cornelia already found the latter one.

> Page 34 / Virtqueue Layout:
>
> - "...  with padded added ..."
>   => "... with padding added ..."
>
> Page 34 / Handling Device Features:
>
> - The text says "Feature bits are in little-endian byte order", but the
>   "struct virtio_feature_desc" is described with "be32 features" ...
>   that's confusing -- are the feature bits now little or big endian?

Good catch, Cornelia caught that for draft 2, too.

> Page 36:
>
> - "Bit numbers start at the left"
>   => I'd make this sentence more explicit, e.g.:
>   "Bit numbers start at the left, i.e. the most significant bit in the
>   first byte is assigned the bit number 0."

Cornelia, is this correct?

> Page 36 / Notification via Adapter I/O Interrupts:
>
> - "The guest-provided summary indicator is also set."
>   => What value is set in the summary indicator byte? 0x01? 0x80? 0xff?
>   It maybe does not matter, since any non-zero value could be used, but
>   it might help to avoid confusion if you specify the exact value here.

Punting this to Cornelia too...

> Page 37 / Early printk for Virtio Consoles
>
> - Is this early print really part of virtio-ccw? If yes, I think you
>   should also describe the register usage here.

Good catch.  This points out that VIRTIO_CONSOLE_F_EMERG_WRITE is not
included in the draft spec.  I have filed a bug for that:

https://tools.oasis-open.org/issues/browse/VIRTIO-53

Other fixes I've gathered so far are below.

Thanks,
Rusty.

diff --git a/content.tex b/content.tex
index 803615d..4ebc4b1 100644
--- a/content.tex
+++ b/content.tex
@@ -801,9 +801,10 @@ any Revision ID value.
 
 \subsection{PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout}
 
-To configure the device,
-use I/O and/or memory regions and/or PCI configuration space of the PCI device.
-These contain the virtio header registers, the notification register, the
+The device is configured via I/O and/or memory regions (though see
+VIRTIO_PCI_CAP_PCI_CFG for access via the PCI configutation space).
+
+These regions contain the virtio header registers, the notification register, the
 ISR status register and device specific registers, as specified by Virtio
 Structure PCI Capabilities.
 
@@ -847,8 +848,7 @@ Common configuration structure layout is documented below:
 \begin{description}
 \item[device_feature_select]
         The driver uses this to select which Feature Bits the device_feature field shows.
-        Value 0x0 selects Feature Bits 0 to 31
-        Value 0x1 selects Feature Bits 32 to 63
+        Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature Bits 32 to 63.
         The device MUST present 0 on device_feature for any other value.
 
 \item[device_feature]
@@ -857,8 +857,7 @@ Common configuration structure layout is documented below:
 
 \item[driver_feature_select]
         The driver uses this to select which Feature Bits the driver_feature field shows.
-        Value 0x0 selects Feature Bits 0 to 31
-        Value 0x1 selects Feature Bits 32 to 63
+        Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature Bits 32 to 63.
         When set to any other value, reads from driver_feature
         return 0, writing 0 into driver_feature has no effect.  The driver
         MUST not write any other value into driver_feature (a corollary of
@@ -899,7 +898,7 @@ Common configuration structure layout is documented below:
 
 \item[queue_enable]
         The driver uses this to selectively prevent the device from executing requests from this virtqueue.
-        1 - enabled; 0 - disabled
+        1 - enabled; 0 - disabled.
 
         The driver MUST configure the other virtqueue fields before enabling
         the virtqueue.
@@ -1043,7 +1042,7 @@ read-only:
 	};
 \end{lstlisting}
 
-This structure can optionally followed by extra data, depending on
+This structure can optionally be followed by extra data, depending on
 other fields, as documented below.
 
 Note that future versions of this specification will likely
@@ -1369,10 +1368,13 @@ following sections.
 
 \subsection{MMIO Device Discovery}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Discovery}
 
-Unlike PCI, MMIO provides no generic device discovery. For
-systems using Flattened Device Trees the suggested format is:
+Unlike PCI, MMIO provides no generic device discovery.  For each
+device, the guest OS will need to know the location of the registers
+and interrupt(s) used.  The suggested binding for Open Firmware is
+shown in this example:
 
 \begin{lstlisting}
+        // EXAMPLE: virtio_block device taking 256 bytes at 0x1e000, interrupt 42.
 	virtio_block@1e000 {
 		compatible = "virtio,mmio";
 		reg = <0x1e000 0x100>;
@@ -1941,7 +1943,7 @@ revision & length & data      & remarks \\
 \hline
 \end{tabular}
 
-Note that a change in the virtio standard does not neccessarily
+Note that a change in the virtio standard does not necessarily
 correspond to a change in the virtio-ccw revision.
 
 A device MUST post a unit check with command reject for any revision
@@ -2037,7 +2039,7 @@ and align the alignment.
 
 \subsubsection{Virtqueue Layout}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Virtqueue Layout}
 
-The virtqueue is physically contiguous, with padded added to make the
+The virtqueue is physically contiguous, with padding added to make the
 used ring meet the align value:
 
 \begin{tabular}{|l|l|l|}