* [PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc
2023-09-21 9:56 [PATCH 1/4] dma: define __dma_aligned attribute Ahmad Fatoum
@ 2023-09-21 9:56 ` Ahmad Fatoum
2023-09-21 10:35 ` Marco Felsch
2023-09-21 9:56 ` [PATCH 3/4] hab: habv4: align config/state at 64 byte boundary Ahmad Fatoum
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2023-09-21 9:56 UTC (permalink / raw)
To: barebox; +Cc: rcz, mfe, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc
2023-09-21 9:56 ` [PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc Ahmad Fatoum
@ 2023-09-21 10:35 ` Marco Felsch
0 siblings, 0 replies; 6+ messages in thread
From: Marco Felsch @ 2023-09-21 10:35 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox, rcz
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/4] hab: habv4: align config/state at 64 byte boundary
2023-09-21 9:56 [PATCH 1/4] dma: define __dma_aligned attribute Ahmad Fatoum
2023-09-21 9:56 ` [PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc Ahmad Fatoum
@ 2023-09-21 9:56 ` Ahmad Fatoum
2023-09-21 9:56 ` [PATCH 4/4] hab: habv4: apply sizeof() to correct object Ahmad Fatoum
2023-09-21 10:35 ` [PATCH 1/4] dma: define __dma_aligned attribute Ahmad Fatoum
3 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2023-09-21 9:56 UTC (permalink / raw)
To: barebox; +Cc: rcz, mfe, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 4/4] hab: habv4: apply sizeof() to correct object
2023-09-21 9:56 [PATCH 1/4] dma: define __dma_aligned attribute Ahmad Fatoum
2023-09-21 9:56 ` [PATCH 2/4] ARM: dma: define DMA_ALIGNMENT instead of defining dma_alloc Ahmad Fatoum
2023-09-21 9:56 ` [PATCH 3/4] hab: habv4: align config/state at 64 byte boundary Ahmad Fatoum
@ 2023-09-21 9:56 ` Ahmad Fatoum
2023-09-21 10:35 ` [PATCH 1/4] dma: define __dma_aligned attribute Ahmad Fatoum
3 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2023-09-21 9:56 UTC (permalink / raw)
To: barebox; +Cc: rcz, mfe, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] dma: define __dma_aligned attribute
2023-09-21 9:56 [PATCH 1/4] dma: define __dma_aligned attribute Ahmad Fatoum
` (2 preceding siblings ...)
2023-09-21 9:56 ` [PATCH 4/4] hab: habv4: apply sizeof() to correct object Ahmad Fatoum
@ 2023-09-21 10:35 ` Ahmad Fatoum
3 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2023-09-21 10:35 UTC (permalink / raw)
To: barebox; +Cc: rcz, mfe
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 |
^ permalink raw reply [flat|nested] 6+ messages in thread