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


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: Feedback for CSPRD03

On Mon, 2014-07-21 at 15:06 +0100, Pawel Moll wrote:
> * "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? Should we explicitly say the device
> ignores it? Is there any way of signalling an error?
> * "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?
> * 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 disagree woth my esteemed colleague on these three... To my mind, what
is not allowed is forbidden and out of scope.

> * 4.2.1 "MMIO provides no generic device discovery"
> Should be "MMIO provides no generic device discovery mechanism"

Fair enough, will fix.

> * 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".

Good idea.

> * 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 

Ok, let me remove the "size" bit then:

"Queue size is the number of elements in the queue, therefore in each of
the Descriptor Table, and the Available Ring and the Used Ring."

> * 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'.  

"Writing one (0x1) to this register notifies the device that it 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 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...)

I'll add reference to and fix the sentence there, so it is
actually correct (currently it's saying "if there is any risk of a
device seeing an inconsistent configuration state", should be "driver

> * 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? 

Yes, I'll expand the description to make it clearer.

> What happens if I try to ack
> something that hasn't happened, eg write 3 when InterruptStatus==0?

Illegal operation, out of scope, but I'll add a MUST NOT to make it

> * 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? 

Yes! The requirement for aligned access was there originally, must have
got lost in translation. Will fix.

> * "When QueueReady is not zero, the drive..."
> Period missing at end of sentence.

Sure thing.

Will send a patch fixing all above soon. Looks "non material" to me :-)


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]