| Message ID | 1340583477-14018-3-git-send-email-marex@denx.de |
|---|---|
| State | Rejected |
| Delegated to: | Marek Vasut |
| Headers | show |
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? 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. Besides, it would be awkward user interface to allow an address to be used in one direction but not the other. What if the caller wants to try a different strategy if this returns 0, rather than print an error? Why should the success of a command depend on whether caches are enabled? If we're going to forbid unaligned addresses in certain contexts, shouldn't it always be forbidden to ensure consistent user experience? 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? -Scott
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
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@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. 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
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; \ + }) + + /* arch/$(ARCH)/lib/ticks.S */ unsigned long long get_ticks(void); void wait_ticks (unsigned long);
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(+)