[RFC PATCH 0/1] Cache incoherent if re-enabled on Cortex-R(5)
From: Emanuele Ghidoli emanuele.ghidoli@toradex.com
I want to propose this patch cause I experience weird behavior on AM62x Cortex-R U-boot spl. Seem that the stack is "restored" to the time dcache_disable was called. The effect is that the code between dcache_disable and dcache_enable is executed twice, but the second execution runs with wrong function parameters.
Best regards,
Emanuele
Emanuele Ghidoli (1): arm: add invalidate_dcache_all() after disable cache
arch/arm/lib/cache-cp15.c | 8 ++++++++ 1 file changed, 8 insertions(+)
From: Emanuele Ghidoli emanuele.ghidoli@toradex.com
On Cortex-R5 flushing and disabling cache is not enough to avoid cache and memory incoherence.
In particular, when the cache is enabled after a disable, and if there are entry in the cache the value from the cache is used instead of the value from the memory. This, in particular, lead to stack corruption if the stack is in a cached area.
The commit 44df5e8d30a2 ("arm: move flush_dcache_all() to just before disable cache") already states that following a cache disable an invalidate is expected to be done but without coping with cache enable. So just add it explicitly.
Signed-off-by: Emanuele Ghidoli emanuele.ghidoli@toradex.com --- arch/arm/lib/cache-cp15.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c index 0893915b3004..2921277d69e0 100644 --- a/arch/arm/lib/cache-cp15.c +++ b/arch/arm/lib/cache-cp15.c @@ -254,7 +254,15 @@ static void cache_disable(uint32_t cache_bit) if (cache_bit == CR_C) #endif flush_dcache_all(); + set_cr(reg & ~cache_bit); + +#ifdef CONFIG_SYS_ARM_MMU + if (cache_bit == (CR_C | CR_M)) +#elif defined(CONFIG_SYS_ARM_MPU) + if (cache_bit == CR_C) +#endif + invalidate_dcache_all(); } #endif
On 5/31/23 00:08, Emanuele Ghidoli wrote:
From: Emanuele Ghidoli emanuele.ghidoli@toradex.com
On Cortex-R5 flushing and disabling cache is not enough to avoid cache and memory incoherence.
In particular, when the cache is enabled after a disable, and if there are entry in the cache the value from the cache is used instead of the value from the memory. This, in particular, lead to stack corruption if the stack is in a cached area.
The commit 44df5e8d30a2 ("arm: move flush_dcache_all() to just before disable cache") already states that following a cache disable an invalidate is expected to be done but without coping with cache enable. So just add it explicitly.
Signed-off-by: Emanuele Ghidoli emanuele.ghidoli@toradex.com
arch/arm/lib/cache-cp15.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c index 0893915b3004..2921277d69e0 100644 --- a/arch/arm/lib/cache-cp15.c +++ b/arch/arm/lib/cache-cp15.c @@ -254,7 +254,15 @@ static void cache_disable(uint32_t cache_bit) if (cache_bit == CR_C) #endif flush_dcache_all();
- set_cr(reg & ~cache_bit);
+#ifdef CONFIG_SYS_ARM_MMU
- if (cache_bit == (CR_C | CR_M))
+#elif defined(CONFIG_SYS_ARM_MPU)
- if (cache_bit == CR_C)
+#endif
We prefer 'if' over '#ifdef'. So you should use something like:
if (IS_ENABLED(CONFIG_SYS_ARM_MMU) && (cache_bit == (CR_C | CR_M) || IS_ENABLED(CONFIG_SYS_ARM_MPU) && (cache_bit == CR_C)) { flush_dcache_all(); invalidate_dcache_all(); } set_cr(reg & ~cache_bit);
(Assuming set_cr() can be called after invalidate_dcache_all.)
Best regards
Heinrich
invalidate_dcache_all();
participants (2)
-
Emanuele Ghidoli -
Heinrich Schuchardt