[PATCH 0/3] ARM64 early cache fixes

[PATCH 0/3] ARM64 early cache fixes

[Lucas Stach]

This is a resend of the 2 ARM64 early cache fixes by Ahmad. They were
not applied at the time, as there was a report of them breaking Barebox
on i.MX8M. I finally had time to look into that, the first patch by
me cleans the stage, so the cache fixes no longer break anything.

At least the icache invalidation patch fixes an issue I hit on one
system, were minimal (harmless) changes to the Barebox code would
break execution.

Regards,
Lucas

Ahmad Fatoum (2):
  ARM: cache_64: invalidate dcache in arm_early_mmu_cache_invalidate
  ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

Lucas Stach (1):
  ARM64: entry: save/restore potentially clobbered registers

 arch/arm/cpu/cache_64.c    |  2 ++
 arch/arm/cpu/entry_ll_64.S | 12 +++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] ARM64: entry: save/restore potentially clobbered registers

[Lucas Stach]

While the comment is correct that currently arm_early_mmu_cache_invalidate()
is only a call to to v8_invalidate_icache_all() , which doesn't clobber x0-x2,
this starts to fall apart as soon as we do something more in this function.

Make sure to properly save/restore the parameters passed to the entry function.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/cpu/entry_ll_64.S | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm/cpu/entry_ll_64.S b/arch/arm/cpu/entry_ll_64.S
index 37e0cb66b549..fb8645e0a03f 100644
--- a/arch/arm/cpu/entry_ll_64.S
+++ b/arch/arm/cpu/entry_ll_64.S
@@ -10,14 +10,16 @@
 .section .text.__barebox_arm_entry
 ENTRY(__barebox_arm_entry)
 	mov	sp, x3
-	/*
-	 * arm_early_mmu_cache_invalidate is jsut a call to
-	 * v8_invalidate_icache_all() which doesn't clobber x0, x1 or x2
- 	 */
+	mov	x19, x0
+	mov	x20, x1
+	mov	x21, x2
 	bl	arm_early_mmu_cache_invalidate
+	mov	x0, x19
+	mov	x1, x20
+	mov	x2, x21
 #if IS_ENABLED(CONFIG_PBL_IMAGE)
 	b	barebox_pbl_start
 #else
 	b	barebox_non_pbl_start
 #endif
-ENDPROC(__barebox_arm_entry)
\ No newline at end of file
+ENDPROC(__barebox_arm_entry)
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] ARM64: entry: save/restore potentially clobbered registers

[Ahmad Fatoum]

Hello Lucas,

On 12/19/19 10:13 AM, Lucas Stach wrote:
> While the comment is correct that currently arm_early_mmu_cache_invalidate()
> is only a call to to v8_invalidate_icache_all() , which doesn't clobber x0-x2,
> this starts to fall apart as soon as we do something more in this function.
> 
> Make sure to properly save/restore the parameters passed to the entry function.

I did the same in <20191002075754.9257-1-a.fatoum@pengutronix.de>, except for
x4-x6 instead of x19-x21, but reported that his i.MX8 still didn't boot.

Cheers
Ahmad


-- 
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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] ARM64: entry: save/restore potentially clobbered registers

[Lucas Stach]

Hi Ahmad,

On Fr, 2019-12-20 at 16:34 +0100, Ahmad Fatoum wrote:
> Hello Lucas,
> 
> On 12/19/19 10:13 AM, Lucas Stach wrote:
> > While the comment is correct that currently arm_early_mmu_cache_invalidate()
> > is only a call to to v8_invalidate_icache_all() , which doesn't clobber x0-x2,
> > this starts to fall apart as soon as we do something more in this function.
> > 
> > Make sure to properly save/restore the parameters passed to the entry function.
> 
> I did the same in <20191002075754.9257-1-a.fatoum@pengutronix.de>, except for
> x4-x6 instead of x19-x21, but reported that his i.MX8 still didn't boot.

I have not thoroughly analyzed the called cache maintenance functions,
but I specifically used x19-x21 as they are defined as callee-saved in
the aarch64 calling convention. 
x4-x6 would be function parameter/return values, so they might get
clobbered, right?

Regards,
Lucas


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] ARM64: entry: save/restore potentially clobbered registers

[Ahmad Fatoum]

Hi,

On 12/20/19 4:42 PM, Lucas Stach wrote:
>> I did the same in <20191002075754.9257-1-a.fatoum@pengutronix.de>, except for
>> x4-x6 instead of x19-x21, but

Rouven* [was missing here]

> reported that his i.MX8 still didn't boot.
> 
> I have not thoroughly analyzed the called cache maintenance functions,
> but I specifically used x19-x21 as they are defined as callee-saved in
> the aarch64 calling convention.
> x4-x6 would be function parameter/return values, so they might get
> clobbered, right?

Ah, v8_invalidate_dcache_all indeed clobbers x4. So this explains
it. Thanks for fixing. :)

Cheers
Ahmad

-- 
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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] ARM: cache_64: invalidate dcache in arm_early_mmu_cache_invalidate

[Lucas Stach]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

On some ARM cores, cache contents are indeterminate after a Power-On
Reset. Turning on the MMU on such cores risks interpreting random cache
lines as valid, causing hard-to-debug errors.

For this reason, we always invalidate the dcache on <= ARMv7. Let's do
likewise for ARM64. Newer ARM cores tend to come up with their dcaches
invalidated already, but for some, like the Cortex-A72, L2 caches are
invalidated dependent on a signal sampled at reset, so better play
it safe.

The icache invalidate here seems to serve no useful purpose. It's kept
for now for symmetry with ARM32.

Note that this is wrong should barebox be entered with the MMU enabled,
but this is so far not the case with any ARM64 platform we support.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/cpu/cache_64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/cpu/cache_64.c b/arch/arm/cpu/cache_64.c
index 45f01e8dc9cf..81c37e1c34fd 100644
--- a/arch/arm/cpu/cache_64.c
+++ b/arch/arm/cpu/cache_64.c
@@ -31,5 +31,6 @@ void arm_early_mmu_cache_flush(void)
 
 void arm_early_mmu_cache_invalidate(void)
 {
+	v8_invalidate_dcache_all();
 	v8_invalidate_icache_all();
 }
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

[Lucas Stach]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

So far arm_early_mmu_cache_flush has only been used in preparation for
executing newly-written code. For this reason, on ARMv7 and below,
it had always invalidate the icache after the dcache flush.
We don't do this on ARM64, but sync_caches_for_execution depends on this,
which had this comment that didn't hold true for ARM64:

> Despite the name arm_early_mmu_cache_flush not only flushes the
> data cache, but also invalidates the instruction cache.

It might be worthwhile to decouple dcache flushing from icache
invalidation, but for now, align what we do on ARM64 with what we do for
32-bit ARMs.

This fixes a potential read of stale instructions when loading
second-stage barebox from the PBL with MMU disabled.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/cpu/cache_64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/cpu/cache_64.c b/arch/arm/cpu/cache_64.c
index 81c37e1c34fd..6e18d981a434 100644
--- a/arch/arm/cpu/cache_64.c
+++ b/arch/arm/cpu/cache_64.c
@@ -27,6 +27,7 @@ int arm_set_cache_functions(void)
 void arm_early_mmu_cache_flush(void)
 {
 	v8_flush_dcache_all();
+	v8_invalidate_icache_all();
 }
 
 void arm_early_mmu_cache_invalidate(void)
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] ARM64 early cache fixes

[Sascha Hauer]

On Thu, Dec 19, 2019 at 10:13:07AM +0100, Lucas Stach wrote:
> This is a resend of the 2 ARM64 early cache fixes by Ahmad. They were
> not applied at the time, as there was a report of them breaking Barebox
> on i.MX8M. I finally had time to look into that, the first patch by
> me cleans the stage, so the cache fixes no longer break anything.
> 
> At least the icache invalidation patch fixes an issue I hit on one
> system, were minimal (harmless) changes to the Barebox code would
> break execution.
> 
> Regards,
> Lucas
> 
> Ahmad Fatoum (2):
>   ARM: cache_64: invalidate dcache in arm_early_mmu_cache_invalidate
>   ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush
> 
> Lucas Stach (1):
>   ARM64: entry: save/restore potentially clobbered registers
> 
>  arch/arm/cpu/cache_64.c    |  2 ++
>  arch/arm/cpu/entry_ll_64.S | 12 +++++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)

Applied, thanks

Sascha

-- 
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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox