OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio message

[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]