Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/5] block/nvme: don't flip CQ phase bits

[John Snow]

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