On Mon, Jul 21, 2014 at 03:06:57PM +0100, Pawel Moll wrote:
> Greetings, >
> I've finally managed to "convince" one of my colleagues to provide some
> feedback. Thanks, Brian! (I'm just acting as an IPR proxy here...) >
> Here it goes: > > > * 2.3.1 "nor reads from multiple fields" >
> Should be "nor *are* reads...", possibly "from multiple adjacent
> fields"? > > > * 2.3.4 "This effects the block capacity and network mac fields" >
> Should be "This *affects* the block capacity and network MAC fields" > > > * 2.4 "When the device has finished a buffer" >
> Would this be clearer as "When the device has finished > *using/accessing/processing/something* a buffer"? > > > * 2.4.5.3.1 "The driver MUST NOT set the VRING_DESC_F_INDIRECT flag
> unless the VIRTIO_RING_F_INDIRECT_DESC feature was negotiated." >
> What happens if we set the flag? Historically devices followed the descriptor bit ignoring the feature.
> Should we explicitly say the device
> ignores it? I don't see a lot of value.
> Is there any way of signalling an error?
>
No as we don't have good ways to signal driver bugs at the moment. I doubt we can do this for 1.0, post 1.0 it's one area which could be improved. When we have that, we can use it. > * 2.4.5.3.1 "The driver MUST NOT set the VIRTQ_DESC_F_INDIRECT flag
> within an indirect descriptor (ie. only one table per descriptor)." >
> OK, good, but again what does the failure mode look like? Should the
> device ignore it? Historically devices behaved in different broken ways.
> > * 3.1.1 "If any of these steps go irrecoverably wrong, the driver SHOULD
> set the FAILED status bit to indicate that it has given up on the device > (it can reset the device later to restart if desired). The driver MUST
> not continue initialization in that case." >
> In terms of impl, does this mean if FAILED is set, the device must
> respond to nothing but reset? I don't think it's worth enforcing by the device. > > * 3.1.1 "The driver MUST not accept" >
> Should be "The driver MUST NOT accept" > > > * 3.1.2 "Legacy devices do not support the FEATURES_OK status bit, and
> thus did not have a graceful way for the device to indicate unsupported
> feature combinations. It also did not provide a clear mechanism to
> end feature negotiation, which meant that devices finalized features on
> first-use, and no features could be introduced which radically changed
> the initial operation of the device." >
> This has mixed tenses ("do not"/"did not"/"which meant"). Probably
> should all be present tense, since all the statements are still true of
> legacy devices now as well as in the past. > > > * 4.2 "Therefore most of operations like..." >
> Should be "Therefore most operations including" > > > * 4.2.1 "MMIO provides no generic device discovery" >
> Should be "MMIO provides no generic device discovery mechanism" > > > * 4.2.2 "MMIO virtio devices provides" >
> Should be "MMIO virtio devices provide" or "An MMIO virtio device
> provides" > > > * 4.2.2 DeviceFeatures, DriverFeatures: "First bit" >
> Should probably be "the least signficiant bit". > > > * 4.2.2 QueueNum "therefore size of the Descriptor Table and both
> Available and Used rings" >
> This is a bit confusing and a bit ambiguous. I think it would be clearer
> as "i.e. the size of the descriptor table, the number of entries in the
> availabe ring, and the number of entries in the used ring." (otherwise
> it's not clear if you mean size(DT) == size(available)+size(used), or > > > * 4.2.2 QueueReady: "Ready to be used" >
> Slightly confusing here: could mean 'CPU has updated the available
> ring/DT', when it really means 'the DT/available ring have first been
> initialised to sensible values'. Might be better to copy the PCI
> description which says 'the device may execute requests from this
> virtual queue'.
> > > * 4.2.2 ConfigGeneration: >
> I was a bit lost as to what counts as a configuration change. I think
> the ref to section 2.3 doesn't help. Sec 4.2.2.1 is a bit better: "The
> device MUST change ConfigGeneration if there is any risk of a device
> seeing an inconsistent configuration state, but it MAY only change the
> value after a configuration read operation.", but I think that the PCI
> description (and the note following) is a bit clearer: "The device MUST
> present a changed config_generation after the driver has read a
> device-specific configuration value which has changed since any part of
> the device-specific configuration was last read. (Note, counter can wrap
> so do this...) > > > * 4.2.2 InterruptAck: "Writing to this register notifies the device that
> the interrupt has been handled" >
> Does this mean if InterruptStatus=3 and I write 1, I'm acking the ring
> update but not the configuration change? What happens if I try to ack
> something that hasn't happened, eg write 3 when InterruptStatus==0? > > > * 4.2.2 Along with saying the registers are LE, should we say that all
> register accesses must be aligned, 32-bit accesses, and anything else is
> undefined behaviour? > > > * 4.2.2.2 "When QueueReady is not zero, the drive..." >
> Period missing at end of sentence. > > > * 5.1.6.5.7.2 "a offload" >
> Should be "an offload" > >
> 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:
> Unsubscribe:
> List help:
> 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/
