[PATCH] nvme: Fix cache alignment

[Marek Vasut]

On 2/8/21 2:32 PM, Andre Przywara wrote:
[...]
>>>>>>>>>>> +static void nvme_flush_dcache_range(void *start, unsigned long size)
>>>>>>>>>>> +{
>>>>>>>>>>> +       unsigned long s, e;
>>>>>>>>>>> +       nvme_align_dcache_range(start, size, &s, &e);
>>>>>>>>>>> +       flush_dcache_range(s, e);
>>>>>>>>>
>>>>>>>>> There is no good reason for alignment restrictions when it comes to
>>>>>>>>> clean (& invalidate), so there is no need for this wrapper.
>>>>>>>>
>>>>>>>> Is that on ARM64-specific or is that applicable in general ? The driver
>>>>>>>> is expected to work on any CPU.
>>>>>>>
>>>>>>> Cache clean (actually: cache clean&invalidate) is what happens on evictions
>>>>>>> all of the time, at the cache controller's discretion. So there is no
>>>>>>> real harm in that operation per se. When an eviction happens on a
>>>>>>> *clean* cache line, this is basically just an invalidate, which is also not
>>>>>>> harmful.
>>>>>>>
>>>>>>> There are harmful cases when buffers sharing a cache line are both "just invalidated"
>>>>>>> and "cleaned" at different points in time.
>>>>>>
>>>>>> Is that on ARM64-specific or is that applicable in general ? (the above
>>>>>> does not answer that question)
>>>>>
>>>>> I would say that's a property of *every* write-back cache
>>>>> implementation:
>>>>> https://en.wikipedia.org/wiki/Cache_(computing)#/media/File:Write-back_with_write-allocation.svg
>>>>
>>>> I've been reading and digesting the thread as it goes, and the only
>>>> thing I do want to chime in on here right now is that yes, U-Boot
>>>> does and will continue to support every CPU that someone wants to run it
>>>> on, and one of the takeaways I see from this thread is we need some
>>>> better documented abstractions around cache, as it's very tricky to get
>>>> right all the time.
>>>
>>> Documenting the u-boot cache function behavior precisely is fine by me, but
>>> that is somewhat separate topic from this bugfix.
>>>
>>> So I will ask a simple question -- is there anything blocking this bugfix
>>> from being applied ?
>>
>> While I can fix the typo, I'm waiting for an Ack/Reviewed-by from Bin
>> (as he's spent the most time on this driver of late) and I'd really like
>> one from Andre, or at least him agreeing this patch is a step in the
>> right direction.
> 
> As I said: I don't see how this patch changes anything on arm64, which
> the commit messages claims to be the reason for this post.
> If someone please can confirm, but invalidate_dcache_range() always
> works on arm64, in fact does the very rounding already that this patch
> introduces here. So what is the actual fix here?

You can NOT depend on the behavior of cache functions on specific 
architecture, U-Boot is universal and this must work on every single 
architecture, not just aarch64.

The entire point of this patch is to call flush/invalidate only with 
cacheline-aligned addresses, which is what they expect, otherwise these 
functions do no operation at all.

> Plus, I consider this blanket rounding more harmful than helpful, since
> it actually just silences the check_cache_range() check.

Can you back this claim with something ? Because I spent my fair share 
of time analyzing the driver and the rounding is exactly sufficient to 
make the cache ops work correctly.

> If we include armv7 here, which in fact would ignore(!) an unaligned
> invalidate

Yes, on armv7 and older, the cache ops behave as they were designed to 
behave.

>, this is my analysis (just for invalidate):
> nvme_read_completion_status():		NEEDS ALIGNMENT
> 	size smaller than cache line, cqes[1] base address unaligned.
> 	fix needed, rounding *could* be a temporary fix with comments
> 	as of why this is legitimate in this case.
> 	Better alternative sketched in a previous email.
> nvme_identify():			OK
> 	struct nvme_id_ctrl is 4K, if I counted correctly, so is fine.
> 	buffer comes the callers, the in-file users pass an aligned
> 	address, the only external user I see is in nvme_show.c, which
> 	also explicitly aligns the buffer.
> nvme_blk_rw():				OK
> 	total_len seems to be a multiple of block size, so is hopefully
> 	already cache line aligned.
> 	buffer comes from the upper layers, I guess it's page
> 	aligned already (at least 512b sector aligned)?
> 
> I turned my sketch for the cqes[] fix into a proper patch and will send
> this in a minute
Please add check_cache_range() to armv8 cache ops and test whether there 
are no warnings from it, thanks.