Re: [Qemu-block] [PATCH v3 1/6] block/nvme: don't touch the completion entries

[Max Reitz]

On 03.07.19 17:59, Maxim Levitsky wrote:
> Completion entries are meant to be only read by the host and written by the 
> device.
> The driver is supposed to scan the completions from the last point where it 
> left,
> and until it sees a completion with non flipped phase bit.

(Disclaimer: This is the first time I read the nvme driver, or really
something in the nvme spec.)

Well, no, completion entries are also meant to be initialized by the
host.  To me it looks like this is the place where that happens:
Everything that has been processed by the device is immediately being
re-initialized.

Maybe we shouldn’t do that here but in nvme_submit_command().  But
currently we don’t, and I don’t see any other place where we currently
initialize the CQ entries.

Max

> Signed-off-by: Maxim Levitsky <address@hidden>
> ---
>  block/nvme.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 73ed5fa75f..6d4e7f3d83 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -315,7 +315,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
> NVMeQueuePair *q)
>      while (q->inflight) {
>          int16_t cid;
>          c = (NvmeCqe *)&q->cq.queue[q->cq.head * NVME_CQ_ENTRY_BYTES];
> -        if (!c->cid || (le16_to_cpu(c->status) & 0x1) == q->cq_phase) {
> +        if ((le16_to_cpu(c->status) & 0x1) == q->cq_phase) {
>              break;
>          }
>          q->cq.head = (q->cq.head + 1) % NVME_QUEUE_SIZE;
> @@ -339,10 +339,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, 
> NVMeQueuePair *q)
>          qemu_mutex_unlock(&q->lock);
>          req.cb(req.opaque, nvme_translate_error(c));
>          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) {
>

signature.asc
Description: OpenPGP digital signature