[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Feedback for CSPRD03
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: 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/
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]