[U-Boot] [PATCH 2/9] CACHE: Add cache_aligned() macro

[Aneesh V]

Hi Marek,

On 06/25/2012 04:30 PM, Marek Vasut wrote:
> Dear Scott Wood,
>
>> On 06/24/2012 07:17 PM, Marek Vasut wrote:
>>> This macro returns 1 if the argument (address) is aligned, returns
>>> zero otherwise. This will be used to test user-supplied address to
>>> various commands to prevent user from loading data to/from unaligned
>>> address when using caches.
>>>
>>> This is made as a macro, because macros are expanded where they are
>>> used. Therefore it can be easily instrumented to report position of
>>> the fault.
>>>
>>> Signed-off-by: Marek Vasut<marex at denx.de>
>>> Cc: Wolfgang Denk<wd at denx.de>
>>> ---
>>>
>>>   include/common.h |   18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/include/common.h b/include/common.h
>>> index 322569e..17c64b0 100644
>>> --- a/include/common.h
>>> +++ b/include/common.h
>>> @@ -730,6 +730,24 @@ void	invalidate_dcache_range(unsigned long start,
>>> unsigned long stop);
>>>
>>>   void	invalidate_dcache_all(void);
>>>   void	invalidate_icache_all(void);
>>>
>>> +/* Test if address is cache-aligned. Returns 0 if it is, 1 otherwise. */
>>> +#define cacheline_aligned(addr)						
> \
>>> +	({								\
>>> +	int __ret;							\
>>> +	if (!dcache_status()) {						\
>>> +		__ret = 1;						\
>>> +	} else if ((addr)&  (ARCH_DMA_MINALIGN - 1)) {			\
>>> +		puts("Align load address to "				\
>>> +			__stringify(ARCH_DMA_MINALIGN)			\
>>> +			" bytes when using caches!\n");			\
>>> +		__ret = 0;						\
>>> +	} else {							\
>>> +		__ret = 1;						\
>>> +	}								\
>>> +	__ret;								\
>>> +	})
>>
>> What if it's a store rather than a load?
>
> Goot point #1
>
>> If this is only supposed to be
>> used for loads (because on a store you can flush the cache rather than
>> invalidate), it's not labelled that way, the changelog says "to/from
>> unaligned address", and the caller might be common code that doesn't
>> know which direction the transfer is in.
>
> And we're back at the flush/invalidate thing. This is what I'd really love to
> understand once and for all:
>
> 1) We must prevent user from loading to address 0x.......1 (unaligned), I'll get
> back to this later on.
>
> 2) If the user had address that starts at aligned location but ends at
> unaligned:
>
> Terminology:
> va -- unrelated variable
> [****] -- aligned range
>
> 2a) LOAD from external source to local address:
> The cache must be invalidated over that area. I see the following issue:
>
> [---------buffer---------][va]
> [********][********][********]
> So consider the scenario where
> i) va is written in u-boot
> ii) data arrive into the buffer via DMA at the same time
> iii) cache is invalidated over whole buffer
>
> This will cause variable "va" to be lost forever. The "va" variable might as
> well be user data, therefore in order to protect those, we should first flush
> the user-supplied area.

Flushing the last cache line may corrupt the data you just DMAed in.
The current implementation after a lot of discussions [1] with Albert
is this:

Invalidate only the aligned part of the buffer. So, in the above case
the first two [********] will get invalidated not the last one.
The last un-aligned cache line will result in a warning.

[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/105113
>
> 2b) STORE from local address to external source:
> Basically the inverted problem as in 2a), but if the driver is written properly,
> shouldn't be any issue.

No. Store doesn't have any problem because flush doesn't harm anything.

I have laid down the invalidate/flush requirements for DMA buffers in:
doc/README.arm-caches

best regards,
Aneesh