Re: [virtio-comment] Feedback for chapter 4

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jan 10, 2014 at 01:49:49PM +0100, Thomas Huth wrote:
> 
> Here's my feedback for Virtio draft 01, chapter 4:
> 
> 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."
> 
> Page 21:
> 
> - The "device_feature_select" and "driver_feature_select" paragraphs are lacking
>   some punctuation marks inbetween.
> 
> Page 23 / Virtio Device Configuration Layout Detection:
> 
> - "This structure can optionally followed by extra data"
>   => "This structure can optionally be followed by extra data"
> 
> 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).
> 
> 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)

I agree since C spec seems to allow compilers to add
arbitrary padding anywhere in theory, we should mention all structures
assume there's no padding.

And in most places, we don't really need the packing as
fields are aligned properly so any sane compiler will just do the right
thing.

Unfortunately CCW has several instances where any reasonable
compiler would instroduce padding, I think just a note won't be enough
since compiler-introduced issues are so hard to debug.
So I think that instead of asking people to use compiler extensions to
suppress padding, it's best just to add padding explicitly.

Cornelia, can we do this?
For example

struct virtio_feature_desc {
	be32 features;
	u8 index;
	u8 padding[3]; /* Optional padding */
};

Unfortunately this won't work with existing devices,
but we can note in the legacy section that
transitional drivers must suppress padding, and
transitional devices must support commands without padding.

Similarly:

struct virtio_thinint_area {
	be64 summary_indicator;
	be64 indicator;
	be64 bit_nr;
	u8 isc;
	u8 padding[7]; /* Optional padding */
};


Balloon has this issue too, and even though we intend to
make this a legacy section, I think we should fix it portably by
splitting up the val field:

struct virtio_balloon_stat {
#define VIRTIO_BALLOON_S_SWAP_IN
#define VIRTIO_BALLOON_S_SWAP_OUT
#define VIRTIO_BALLOON_S_MAJFLT
#define VIRTIO_BALLOON_S_MINFLT
#define VIRTIO_BALLOON_S_MEMFREE
#define VIRTIO_BALLOON_S_MEMTOT
	le16 tag;
	le16 val_0_15;  /* Value bits  0 to 15 */
	le16 val_16_31; /* Value bits 16 to 31 */
	le16 val_32_47; /* Value bits 32 to 47 */
	le16 val_48_63; /* Value bits 48 to 63 */
};



> Page 33:
> 
> - Some typos:
>   neccessarily => necessarily
>   issueing => issuing
> 
> 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?
> 
> 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."
> 
> 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.
> 
> 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.
> 
> 
>  Regards,
>   Thomas
> 
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/