On 06/06/19 23:23, John Snow wrote:
> So: This looks right; does this fix a bug that can be observed? Do we
> have any regression tests for block/NVMe?
I don't think it fixes a bug; by the time the CQ entry is picked up by
QEMU, the device is not supposed to touch it anymore.
However, the idea behind the phase bits is that you can decide whether
the driver has placed a completion in the queue. When we get here, we have
le16_to_cpu(c->status) & 0x1) == !q->cq_phase
On the next pass through the ring buffer q->cq_phase will be flipped,
and thus when we see this element we'll get
le16_to_cpu(c->status) & 0x1) == q->cq_phase
and not process it. Since block/nvme.c flips the bit, this mechanism
does not work and the loop termination relies on the other part of the
condition, "if (!c->cid) break;".
So the patch is correct, but it would also be nice to also either remove
phase handling altogether, or check that the phase handling works
properly and drop the !c->cid test.
Paolo
