[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: > * 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ł
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]