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 <mlevi...@redhat.com> > > --- > > 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) { > > > >