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@denx.de Cc: Wolfgang Denk wd@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.
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.
Besides, it would be awkward user interface to allow an address to be used in one direction but not the other.
Correct, it should be possible to adjust the message.
What if the caller wants to try a different strategy if this returns 0, rather than print an error?
Good point.
Why should the success of a command depend on whether caches are enabled?
On less capable architectures, flushing caches over unaligned area causes real mayhem (and DMA depends on this) and it's really tough to debug. If the caches are off, it's all good.
Good point is, that this code should be enabled on per-CPU-model basis probably? Or entirely configurable in the include/configs/....
If we're going to forbid unaligned addresses in certain contexts, shouldn't it always be forbidden to ensure consistent user experience?
This is a good argument ... I wonder, let's hear others opinions.
Or if we're going to be picky about when we reject it, why don't we care whether the device in question does DMA, and whether that DMA is coherent?
Correct, good point. But then this is something that should be added into the upcomming uboot driver model!
-Scott
Best regards, Marek Vasut