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

[Maxim Levitsky]

On Fri, 2019-07-05 at 13:03 +0200, Max Reitz wrote:
> 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.

Hi!
I couldn't find any place in the spec that says that completion entries should 
be initialized.
It is probably wise to initialize that area to 0 on driver initialization, but 
nothing beyond that.
In particular that is what the kernel nvme driver does. 
Other that allocating a zeroed memory (and even that I am not sure it does), 
it doesn't write to the completion entries.

Thanks for the very very good review btw. I will go over all patches now and 
fix things.

Best regards,
        Maxim Levitsky

> 
> 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) {
> > 
> 
>