Feedback for CSPRD03

[Pawel Moll <pawel.moll@arm.com>]

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


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