On 6/7/19 7:08 AM, Paolo Bonzini wrote:
> 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
>
Gotcha, I see, that's why it doesn't cause problems. Thanks :)
--js
