[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
[ https://tools.oasis-open.org/issues/browse/VIRTIO-94?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Michael Tsirkin updated VIRTIO-94: ---------------------------------- Affects Version/s: virtio 1.0 csprd01 Fix Version/s: virtio 1.0 csprd02 > 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 > Affects Versions: virtio 1.0 csprd01 > Reporter: Michael Tsirkin > Fix For: virtio 1.0 csprd02 > > > 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]