[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/6] block/nvme: don't touch the completion e
From: |
Maxim Levitsky |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/6] block/nvme: don't touch the completion entries |
Date: |
Sun, 07 Jul 2019 11:43:33 +0300 |
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) {
> >
>
>
[Qemu-devel] [PATCH v3 3/6] block/nvme: support larger that 512 bytes sector devices, Maxim Levitsky, 2019/07/03
[Qemu-devel] [PATCH v3 2/6] block/nvme: fix doorbell stride, Maxim Levitsky, 2019/07/03