[PATCH] nvme: Fix cache alignment

[Marek Vasut]

On 2/2/21 10:12 AM, Bin Meng wrote:
[...]
>>>>>> cache aligned in memory, however the cache operations are called on
>>>>>> the structure sizes, which themselves might not be cache aligned. Add
>>>>>> the necessary rounding to fix this, which permits the nvme to work on
>>>>>> arm64.
>>>>>
>>>>> +ARM guys
>>>>>
>>>>> Which ARM64 SoC did you test this with?
>>>>
>>>> RCar3, although that's irrelevant, the problem will happen on any arm or
>>>> arm64, and possibly any other system which needs cache management.
>>>
>>> There was a recent change to nvme.c that fixed a cache issue on ARMv8
>>> so I thought this might be platform related.
>>
>> I used master, so unlikely.
> 
> It's strange this issue was not exposed last time when this driver was
> tested on ARMv8.

Which ARMv8 platform ?

>>>>> The round down in this patch should be unnecessary.
>>>>
>>>> Can you explain why ?
>>>
>>> I just took a further look and most of the start address should be
>>> cache line aligned (4KiB aligned) except the
>>> nvme_read_completion_status(). It's only 16 bytes aligned which might
>>> not be cache line aligned.
>>
>> Right, there are various arm chips with 32B/64B alignment requirements.
>>
>>>>> But it's better to
>>>>> figure out which call to dcache_xxx() with an unaligned end address.
>>>>
>>>> If you look at the code, most of them can (and do) trigger this,
>>>> therefore they need such alignment, as explained in the commit message.
>>>
>>> Now I wonder what's the correct implementation of the
>>> invalidate_dcache_range() and flush_dcache_range() in U-Boot?
>>> Shouldn't the round down/up happen in these APIs instead of doing such
>>> in drivers?
>>
>> Definitely not, because then the rounding might flush/invalidate cache
>> over areas where this could cause a problem (e.g. neighboring DMA
>> descriptors). The driver must do the cache management correctly.
> 
> Well we can implement in these APIs and document its expected usage.

That would be harmful, because you cannot really flush/invalidate half 
of cache line. Consider you have two 16B DMA descriptors next to each 
other, which is often the case, then the driver has to implement proper 
cache handling.

That this particular driver already has structures reasonably well 
padded that they can be flushed by adding rounding is an exception.

> Either way a driver has to do the cache management correctly. Not
> doing it in the driver eliminates some duplications of rounding
> up/down.

Indeed, and rounding up/down is the correct way to do it in this driver, 
see the commit message.

> For this case, I believe we just need to take care of
> nvme_read_completion_status().

Can you validate this ?