diff mbox series

[RFC,1/1] arm: add invalidate_dcache_all() after disable cache

Message ID 20230530220835.2358955-2-ghidoliemanuele@gmail.com
State Changes Requested
Delegated to: Heinrich Schuchardt
Headers show
Series Cache incoherent if re-enabled on Cortex-R(5) | expand

Commit Message

Emanuele Ghidoli May 30, 2023, 10:08 p.m. UTC 
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(+)

Comments

Heinrich Schuchardt May 31, 2023, 7:42 a.m. UTC | #1 
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();
>   }
>   #endif
>
diff mbox series

Patch

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