[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: [OASIS Issue Tracker] (VIRTIO-94) VIRTIO Spec feedback #2 From: Thomas Huth
Michael Tsirkin created VIRTIO-94: ------------------------------------- Summary: VIRTIO Spec feedback #2 From: Thomas Huth Key: VIRTIO-94 URL: https://tools.oasis-open.org/issues/browse/VIRTIO-94 Project: OASIS Virtual I/O Device (VIRTIO) TC Issue Type: Bug Reporter: Michael Tsirkin Date: Fri, 10 Jan 2014 13:49:49 +0100 Link to Mail: https://lists.oasis-open.org/archives/virtio-comment/201401/msg00001.html Commenter name: Thomas Huth <thuth@linux.vnet.ibm.com> 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) 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. -- This message was sent by Atlassian JIRA (v6.1.1#6155)
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]