[PATCH] nvme: Invalidate dcache before submitting admin cmd

[Bin Meng]

Hi André,

On Mon, Jun 8, 2020 at 9:52 PM André Przywara <andre.przywara at arm.com> wrote:
>
> On 07/06/2020 12:22, Jagan Teki wrote:
>
> Hi,
>
> (CC: ing Mark)
>
> Without looking to deep, I think invalidating the cache might be the
> right thing to do, but the rationale or at least the wording of it seems
> somehow flawed:
>
> > Some architecture like ARM Cortex A53, A72 would need
>
> Please don't mix the term "architecture" with actual implementations.
> And the problem is more general, I would say. This *might* be a case
> here where this is due to speculation, and then I would expect it to
> only show on an A72, for instance. I guess this is about NVMe on RK3399?
> Does U-Boot run on an A53 or an A72 core?
>
> > to invalidate dcache to sync the cache with the memory
> > contents before flushing the cache to memory.
>
> That is somewhat confusing. What does "sync" and "flush" mean here?
> AFAIK only "clean" and "invalidate" are clearly defined terms.
> The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is
> purely about the buffer for the *return* data, in which case I would
> expect this being a pure invalidation problem.
>
> This issue looks like the case described on slide 30 and 31 here:
> https://elinux.org/images/6/6d/Rutland2.pdf

Thanks for the slides. This is really helpful.

>
> > The NVME here submitting the admin command using dma_addr
> > to the memory without prior cache invalidation. This causing
> > dma_addr is pointing to an invalid location in the memory
>
> This does not sound right: dma_addr is always pointing to the right
> location in memory.
> The actual reason this fixes something might be that (some) cache lines
> of the DMA buffer were dirty and were evicted *before* the existing
> invalidation, but *after* the DMA transfer. That sounds unlikely in an
> U-Boot context, though, but would match the case described in Mark's slides.
>
> So there might be more to this. Are we missing a barrier here? Should we
> use coherent buffers (memory mapped as un-cached) for the return DMA in
> the first place (but I don't think U-Boot provides those)?
>
> > and found the sane nvme_ctrl result.
> >
> > Below example shows the nvme disk scan result improper result
> >
> > => nvme scan
> > nvme_get_info_from_identify: nn = 544502629, vwc = 100,
> > sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
> >
> > So, invalidating the cache before submitting the admin command
> > makes the dma_addr points to a valid location in the memory.
>
> If this shows up already, I think we should address all the comments in
> the driver where it says we should do cache maintenance. And it makes me
> wonder how this actually works today ...
>

I believe the driver was only validated on x86 and NXP's PowerPC
series where cache coherence is ensured between the DMA master and the
system memory. I suspect it was never validated on ARM hence this
patch.

I agree we should address all the comments in the driver about cache
maintenance.

Regards,
Bin