[PATCH 1/4] dma: define __dma_aligned attribute

[PATCH 1/4] dma: define __dma_aligned attribute

[Ahmad Fatoum]

Unlike the kernel, we always map barebox stack 1:1, so DMA to barebox
stack is ok if care is taken for alignment. Otherwise, cache maintenance
may end up clobbering data unintentionally.

Provide a __dma_aligned attribute for use in such situations and use the
already existing DMA_ALIGNMENT as alignment.

To be able to do that, we need to make sure that the default DMA_ALIGNMENT
is only defined when architectures don't define their own dma_alloc. If
they do, they are responsible to define their own DMA_ALIGNMENT as well.

The new attribute is intentionally not called __cacheline_aligned,
because it differs functionally: We care about the cache line size of
the outer cache, while in Linux __cacheline_aligned is for L1 cache.
A __dma_aligned wouldn't make sense for Linux as it would be too easy to
abuse (e.g. placing it on VMAP_STACK), but for barebox, we do this at
many places and an attribute would increase readability and even safety.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/dma.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/dma.h b/include/dma.h
index 2a09b747d1e2..469c482e7a3a 100644
--- a/include/dma.h
+++ b/include/dma.h
@@ -17,17 +17,19 @@
 
 #define DMA_ADDRESS_BROKEN	NULL
 
+#ifndef dma_alloc
 #ifndef DMA_ALIGNMENT
 #define DMA_ALIGNMENT	32
 #endif
 
-#ifndef dma_alloc
 static inline void *dma_alloc(size_t size)
 {
 	return xmemalign(DMA_ALIGNMENT, ALIGN(size, DMA_ALIGNMENT));
 }
 #endif
 
+#define __dma_aligned __attribute__((__aligned__((DMA_ALIGNMENT))))
+
 #ifndef dma_free
 static inline void dma_free(void *mem)
 {
-- 
2.39.2

[PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc

[Ahmad Fatoum]

There's a suitable fallback dma_alloc implementation already in dma.h,
which we can use as soon as we define DMA_ALIGNMENT. By doing that, we
also can use the __dma_aligned attribute that will come in handy in the
follow-up commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/include/asm/dma.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/dma.h b/arch/arm/include/asm/dma.h
index 53953a486393..0774a11c5a30 100644
--- a/arch/arm/include/asm/dma.h
+++ b/arch/arm/include/asm/dma.h
@@ -3,11 +3,7 @@
 
 #include <common.h>
 
-#define dma_alloc dma_alloc
-static inline void *dma_alloc(size_t size)
-{
-	return xmemalign(64, ALIGN(size, 64));
-}
+#define DMA_ALIGNMENT	64
 
 #ifndef CONFIG_MMU
 #define dma_alloc_coherent dma_alloc_coherent
-- 
2.39.2

[PATCH 3/4] hab: habv4: align config/state at 64 byte boundary

[Ahmad Fatoum]

We do manual cache maintenance for &config and &state, because they are
passed as arguments to a secure monitor call. Before the SMC, they are
flushed and afterwards, they are invalidated, but still, we should keep
data we do manual cache maintenance for separate from usual local
variables. Do this by using the new __dma_aligned attribute.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/hab/habv4.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index 38f464fbf978..bbceb0e985b6 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -12,6 +12,7 @@
 #include <init.h>
 #include <types.h>
 #include <mmu.h>
+#include <dma.h>
 #include <zero_page.h>
 #include <linux/sizes.h>
 #include <linux/arm-smccc.h>
@@ -547,8 +548,8 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 	uint32_t len;
 	int i;
 	enum hab_status status;
-	enum hab_config config = 0x0;
-	enum habv4_state state = 0x0;
+	__dma_aligned enum hab_config config = 0x0;
+	__dma_aligned enum habv4_state state = 0x0;
 
 	if (rvt->header.tag != HAB_TAG_RVT) {
 		pr_err("ERROR - RVT not found!\n");
-- 
2.39.2

[PATCH 4/4] hab: habv4: apply sizeof() to correct object

[Ahmad Fatoum]

This introduces no functional change, because sizeof(enum habv4_state)
== sizeof(enum hab_config), but it's the correct thing to do.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/hab/habv4.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index bbceb0e985b6..e65dc8756e17 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -178,20 +178,20 @@ static enum hab_status hab_sip_report_status(enum hab_config *config,
 
 	if (state)
 		v8_flush_dcache_range((unsigned long)state,
-				      (unsigned long)state + sizeof(*config));
+				      (unsigned long)state + sizeof(*state));
 	if (config)
 		v8_flush_dcache_range((unsigned long)config,
-				      (unsigned long)config + sizeof(*state));
+				      (unsigned long)config + sizeof(*config));
 
 	arm_smccc_smc(FSL_SIP_HAB, FSL_SIP_HAB_REPORT_STATUS,
 		      (unsigned long) config,
 		      (unsigned long) state, 0, 0, 0, 0, &res);
 	if (state)
 		v8_inv_dcache_range((unsigned long)state,
-				    (unsigned long)state + sizeof(*config));
+				    (unsigned long)state + sizeof(*state));
 	if (config)
 		v8_inv_dcache_range((unsigned long)config,
-				    (unsigned long)config + sizeof(*state));
+				    (unsigned long)config + sizeof(*config));
 	return (enum hab_status)res.a0;
 }
 
-- 
2.39.2

Re: [PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc

[Marco Felsch]

Hi Ahmad,

On 23-09-21, Ahmad Fatoum wrote:
> There's a suitable fallback dma_alloc implementation already in dma.h,
> which we can use as soon as we define DMA_ALIGNMENT. By doing that, we
> also can use the __dma_aligned attribute that will come in handy in the
> follow-up commit.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

feel free to add my r-b for the whole series.

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> ---
>  arch/arm/include/asm/dma.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma.h b/arch/arm/include/asm/dma.h
> index 53953a486393..0774a11c5a30 100644
> --- a/arch/arm/include/asm/dma.h
> +++ b/arch/arm/include/asm/dma.h
> @@ -3,11 +3,7 @@
>  
>  #include <common.h>
>  
> -#define dma_alloc dma_alloc
> -static inline void *dma_alloc(size_t size)
> -{
> -	return xmemalign(64, ALIGN(size, 64));
> -}
> +#define DMA_ALIGNMENT	64
>  
>  #ifndef CONFIG_MMU
>  #define dma_alloc_coherent dma_alloc_coherent
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/4] dma: define __dma_aligned attribute

[Ahmad Fatoum]

On 21.09.23 11:56, Ahmad Fatoum wrote:
> Unlike the kernel, we always map barebox stack 1:1, so DMA to barebox
> stack is ok if care is taken for alignment. Otherwise, cache maintenance
> may end up clobbering data unintentionally.
> 
> Provide a __dma_aligned attribute for use in such situations and use the
> already existing DMA_ALIGNMENT as alignment.
> 
> To be able to do that, we need to make sure that the default DMA_ALIGNMENT
> is only defined when architectures don't define their own dma_alloc. If
> they do, they are responsible to define their own DMA_ALIGNMENT as well.
> 
> The new attribute is intentionally not called __cacheline_aligned,
> because it differs functionally: We care about the cache line size of
> the outer cache, while in Linux __cacheline_aligned is for L1 cache.
> A __dma_aligned wouldn't make sense for Linux as it would be too easy to
> abuse (e.g. placing it on VMAP_STACK), but for barebox, we do this at
> many places and an attribute would increase readability and even safety.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Please dismiss this series. __dma_aligned as attribute only makes sense
if it would not only affect alignment, but also would round up variable
size to multiple of cache line to ensure, we don't touch adjacent unrelated
variables. I'll have to revisit this.

> ---
>  include/dma.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/dma.h b/include/dma.h
> index 2a09b747d1e2..469c482e7a3a 100644
> --- a/include/dma.h
> +++ b/include/dma.h
> @@ -17,17 +17,19 @@
>  
>  #define DMA_ADDRESS_BROKEN	NULL
>  
> +#ifndef dma_alloc
>  #ifndef DMA_ALIGNMENT
>  #define DMA_ALIGNMENT	32
>  #endif
>  
> -#ifndef dma_alloc
>  static inline void *dma_alloc(size_t size)
>  {
>  	return xmemalign(DMA_ALIGNMENT, ALIGN(size, DMA_ALIGNMENT));
>  }
>  #endif
>  
> +#define __dma_aligned __attribute__((__aligned__((DMA_ALIGNMENT))))
> +
>  #ifndef dma_free
>  static inline void dma_free(void *mem)
>  {

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |