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

[John Snow]

On 6/5/19 3:47 AM, Maxim Levitsky wrote:
> On Mon, 2019-06-03 at 18:25 -0400, John Snow wrote:
>>
>> On 4/17/19 3:53 PM, Maxim Levitsky wrote:
>>> Phase bits are only set by the hardware to indicate new completions
>>> and not by the device driver.
>>>
>>> Signed-off-by: Maxim Levitsky <address@hidden>
>>> ---
>>>  block/nvme.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/block/nvme.c b/block/nvme.c
>>> index 0684bbd077..2d208000df 100644
>>> --- a/block/nvme.c
>>> +++ b/block/nvme.c
>>> @@ -340,8 +340,6 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
>>> NVMeQueuePair *q)
>>>          qemu_mutex_lock(&q->lock);
>>>          c->cid = cpu_to_le16(0);
>>>          q->inflight--;
>>> -        /* Flip Phase Tag bit. */
>>> -        c->status = cpu_to_le16(le16_to_cpu(c->status) ^ 0x1);
>>>          progress = true;
>>>      }
>>>      if (progress) {
>>>
>>
>> Since you've not got much traction on this and you've pinged a v2, can
>> you point me to a spec or a reproducer that illustrates the problem?
>>
>> (Or wait for more NVME knowledgeable people to give you a review...!)
> 
> "A Completion Queue entry is posted to the Completion Queue when the 
> controller write of that Completion
> Queue entry to the next free Completion Queue slot inverts the Phase Tag (P) 
> bit from its previous value
> in memory. The controller may generate an interrupt to the host to indicate 
> that one or more Completion
> Queue entries have been posted."
> 

In the future, please reference the sections in your commit messages
when relevant:

NVM Express 1.3, Section 4.1 "Submission Queue & Completion Queue
Definition"

I also found 4.6 "Completion Queue Entry" to be informative; especially
Figure 28 which defines the phase bit.

So: This looks right; does this fix a bug that can be observed? Do we
have any regression tests for block/NVMe?

--js

> 
> 
> Best regards,
>       Maxim Levitsky
>