On Mon, 2014-07-21 at 15:06 +0100, Pawel Moll wrote:
> * 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? Should we explicitly say the device
> ignores it? Is there any way of signalling an error?
>
>
> * 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?
>
>
> * 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"
Agreed.
> * 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 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...)
I'll add reference to 4.2.2.1 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
seeing").
> * 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
explicit.
> * 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.
> * 4.2.2.2 "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 :-)
Paweł
