From dda7e67c94e4dbf25902fe6d22d8b00711ea24af Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Fri, 28 Jan 2022 14:42:38 +0100 Subject: [PATCH 1/2] coherent: tighten up the API This patch adds documentation to the coherent API header and cleans up several potentially harmful operations. Specifically, it isn't a good idea to modify memory via uncached addresses and then write back caches, because the latter can overwrite uncached writes. It works because there is no data in cache at that point, which also makes that call redundant. Similarly, we are sure not to have any data in caches when obtaining access to the memory, because otherwise our spinlock-acquisition operation could be overwritten by a speculative write back. Signed-off-by: Guennadi Liakhovetski --- src/include/sof/coherent.h | 42 +++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/src/include/sof/coherent.h b/src/include/sof/coherent.h index a3fa493a2148..efc059b9083a 100644 --- a/src/include/sof/coherent.h +++ b/src/include/sof/coherent.h @@ -20,11 +20,37 @@ #define __coherent __attribute__((packed, aligned(DCACHE_LINE_SIZE))) /* + * The coherent API allows optimized access to memory by multiple cores, using + * cache, taking care about coherence. The intended use is to let cores acquire + * ownership of such shared objects, use them, and then release them, possibly + * to be re-acquired by other cores. Such shared objects must only be accessed + * via this API. It's designed to be primarily used with dynamically allocated + * objects because of their well-defined life span. It can also be used with + * objects from .data or .bss sections but greater care must be takenwith them + * to strictly follow the API flow. + * + * The API assumes, that in the beginning no core has cache lines associated + * with the memory area, used with it. That is true for dynamically allocated + * memory, because when such memory is freed, its cache is invalidated - as long + * as that memory was never accessed by other cores, except by using this API. + * The first call must be coherent_init(), which initializes the header. If the + * object will be used by multiple cores, next coherent_shared() must be called. + * After that to use that memory, coherent_acquire_irq() or coherent_acquire() + * must be called, which acquires ownership of the object and returns a cached + * address of the memory. After that the user can perform cached access to the + * memory. To release the memory, one of coherent_release_irq() or + * coherent_release() must be called. The only time when the memory is accessed + * using cache is between those two calls, so only when releasing the memory we + * have to write back and invalidate caches to make sure, that next time we + * acquire this memory, our uncached header access will not be overwritten! When + * memory is not needed any more, typically before freeing the memory, + * coherent_free() should be called. + * * This structure needs to be embedded at the start of any container to ensure * container object cache alignment and to minimise non cache access when * acquiring ownership. * - * This structure should not be accessed outside of these APIs. + * This structure must not be accessed outside of these APIs. * The shared flag is only set at coherent init and thereafter it's RO. */ struct coherent { @@ -58,12 +84,8 @@ struct coherent { #define CHECK_COHERENT_IRQ(_c) #endif -/* - * Incoherent devices require manual cache invalidation and writeback as - * well as locking to manage shared access. - */ - #if CONFIG_INCOHERENT +/* When coherent_acquire() is called, we are sure not to have cache for this memory */ __must_check static inline struct coherent *coherent_acquire(struct coherent *c, const size_t size) { @@ -158,8 +180,8 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si object->member.shared = false; \ object->member.core = cpu_get_id(); \ list_init(&object->member.list); \ - /* wtb and inv local data to coherent object */ \ - dcache_writeback_invalidate_region(uncache_to_cache(object), sizeof(*object)); \ + /* inv local data to coherent object */ \ + dcache_invalidate_region(uncache_to_cache(object), sizeof(*object)); \ } while (0) #define coherent_free(object, member) \ @@ -177,7 +199,7 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si ADDR_IS_COHERENT(object); \ spin_lock(&(object)->member.lock); \ (object)->member.shared = true; \ - dcache_writeback_invalidate_region(object, sizeof(*object)); \ + dcache_invalidate_region(object, sizeof(*object)); \ spin_unlock(&(object)->member.lock); \ } while (0) @@ -188,7 +210,7 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si ADDR_IS_COHERENT(object); \ spin_lock_irq(&(object)->member.lock, &(object)->member.flags); \ (object)->member.shared = true; \ - dcache_writeback_invalidate_region(object, sizeof(*object)); \ + dcache_invalidate_region(object, sizeof(*object)); \ spin_unlock_irq(&(object)->member.lock, &(object)->member.flags); \ } while (0) #else From ed12ade37583af105078b775e2df715f97a77426 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Wed, 2 Feb 2022 11:59:54 +0100 Subject: [PATCH 2/2] coherent: (cosmetic) align backslashes Align several line-continuation backslashes in macro definitions. Signed-off-by: Guennadi Liakhovetski --- src/include/sof/coherent.h | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/include/sof/coherent.h b/src/include/sof/coherent.h index efc059b9083a..981060669779 100644 --- a/src/include/sof/coherent.h +++ b/src/include/sof/coherent.h @@ -171,41 +171,41 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si return cache_to_uncache(c); } -#define coherent_init(object, member) \ - do { \ +#define coherent_init(object, member) \ + do { \ /* assert if someone passes a cache/local address in here. */ \ ADDR_IS_COHERENT(object); \ /* TODO static assert if we are not cache aligned */ \ spinlock_init(&object->member.lock); \ - object->member.shared = false; \ + object->member.shared = false; \ object->member.core = cpu_get_id(); \ list_init(&object->member.list); \ /* inv local data to coherent object */ \ dcache_invalidate_region(uncache_to_cache(object), sizeof(*object)); \ } while (0) -#define coherent_free(object, member) \ - do { \ - /* assert if someone passes a cache address in here. */ \ +#define coherent_free(object, member) \ + do { \ + /* assert if someone passes a cache address in here. */ \ ADDR_IS_COHERENT(object); \ /* wtb and inv local data to coherent object */ \ dcache_writeback_invalidate_region(uncache_to_cache(object), sizeof(*object)); \ } while (0) /* set the object to shared mode with coherency managed by SW */ -#define coherent_shared(object, member) \ - do { \ +#define coherent_shared(object, member) \ + do { \ /* assert if someone passes a cache/local address in here. */ \ ADDR_IS_COHERENT(object); \ spin_lock(&(object)->member.lock); \ (object)->member.shared = true; \ dcache_invalidate_region(object, sizeof(*object)); \ - spin_unlock(&(object)->member.lock); \ + spin_unlock(&(object)->member.lock); \ } while (0) /* set the object to shared mode with coherency managed by SW */ -#define coherent_shared_irq(object, member) \ - do { \ +#define coherent_shared_irq(object, member) \ + do { \ /* assert if someone passes a cache/local address in here. */ \ ADDR_IS_COHERENT(object); \ spin_lock_irq(&(object)->member.lock, &(object)->member.flags); \ @@ -267,8 +267,8 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si return c; } -#define coherent_init(object, member) \ - do { \ +#define coherent_init(object, member) \ + do { \ /* TODO static assert if we are not cache aligned */ \ spinlock_init(&object->member.lock); \ object->member.shared = 0; \