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

[Maxim Levitsky]

On Mon, 2019-07-08 at 14:23 +0200, Max Reitz wrote:
> On 07.07.19 10:43, Maxim Levitsky wrote:
> > 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.
> 
> Ah, you’re right, I misread.  I didn’t pay as much attention to the
> “...prior to setting CC.EN to ‘1’” as I should have.  Yep, and that is
> done in nvme_init_queue().
> 
> OK, I cease my wrongful protest:
> 
> Reviewed-by: Max Reitz <address@hidden>
> 
> > 

Thank you very much!
BTW, the qemu driver does allocate zeroed memory (in nvme_init_queue, 
"q->queue = qemu_try_blockalign0(bs, bytes);"

Thus I think this is all that is needed in that regard.

Note that this patch doesn't fix any real bug I know of, 
but just makes the thing right in regard to the spec.
Also racing with hardware in theory can have various memory ordering bugs,
although in this case the writes are done in 
entries which controller probably won't touch, but still.

TL;DR - no need in code which does nothing and might cause issues.

Do you want me to resend the series or shall I wait till we decide
what to do with the image creation support? I done fixing all the
review comments long ago, just didn't want to resend the series.
Or shall I drop that patch and resend?

>From the urgency standpoint the only patch that really should
be merged ASAP is the one that adds support for block sizes,
because without it, the whole thing crashes and burns on 4K
nvme drives.

Best regards,
        Maxim Levitsky