[PATCH 1/2] ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

[PATCH 1/2] ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

[Ahmad Fatoum]

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>
---
 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..847323525424 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.23.0


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

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

[Ahmad Fatoum]

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>
---
 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 847323525424..6e18d981a434 100644
--- a/arch/arm/cpu/cache_64.c
+++ b/arch/arm/cpu/cache_64.c
@@ -32,5 +32,6 @@ void arm_early_mmu_cache_flush(void)
 
 void arm_early_mmu_cache_invalidate(void)
 {
+	v8_invalidate_dcache_all();
 	v8_invalidate_icache_all();
 }
-- 
2.23.0


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

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

[Ahmad Fatoum]

On 10/1/19 11:09 AM, Ahmad Fatoum wrote:
> For this reason, we always invalidate the dcache on <= ARMv7. Let's do

s/dcache/dcache on startup/

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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/2] ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

[Rouven Czerwinski]

Hello Ahmad,

with these two patches applied, the NXP i.MX8MQ no longer starts into
barebox.

- rcz

On Tue, 2019-10-01 at 11:09 +0200, Ahmad Fatoum wrote:
> 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>
> ---
>  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..847323525424 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)


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

Re: [PATCH 1/2] ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

[Ahmad Fatoum]

On 10/2/19 9:38 AM, Rouven Czerwinski wrote:
> Hello Ahmad,
> 
> with these two patches applied, the NXP i.MX8MQ no longer starts into
> barebox.

Can you try each patch separately to see which one is the culprit?

Thanks for testing,
Ahmad

> 
> - rcz
> 
> On Tue, 2019-10-01 at 11:09 +0200, Ahmad Fatoum wrote:
>> 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>
>> ---
>>  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..847323525424 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)
> 
> 


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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/2] ARM: cache_64: invalidate icache in arm_early_mmu_cache_flush

[Rouven Czerwinski]

On Wed, 2019-10-02 at 09:43 +0200, Ahmad Fatoum wrote:
> On 10/2/19 9:38 AM, Rouven Czerwinski wrote:
> > Hello Ahmad,
> > 
> > with these two patches applied, the NXP i.MX8MQ no longer starts
> > into
> > barebox.
> 
> Can you try each patch separately to see which one is the culprit?

Patch 2/2 is the culprit.

-rcz

> Thanks for testing,
> Ahmad
> 
> > - rcz
> > 
> > On Tue, 2019-10-01 at 11:09 +0200, Ahmad Fatoum wrote:
> > > 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>
> > > ---
> > >  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..847323525424 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)
> 
> 


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

[PATCH] ARM: aarch64: save clobbered registers in __barebox_arm_entry

[Ahmad Fatoum]

arm_early_mmu_cache_invalidate now clobbers x0, x1, x2, which might be
passed by a previous stage bootloader. Have the caller save them.
---
Rouven, does this work for you?
---
 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..41d6cfb6a851 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	x4, x0
+	mov	x5, x1
+	mov	x6, x2
 	bl	arm_early_mmu_cache_invalidate
+	mov	x0, x4
+	mov	x1, x5
+	mov	x2, x6
 #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.23.0


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

Re: [PATCH] ARM: aarch64: save clobbered registers in __barebox_arm_entry

[Rouven Czerwinski]

On Wed, 2019-10-02 at 09:57 +0200, Ahmad Fatoum wrote:
> arm_early_mmu_cache_invalidate now clobbers x0, x1, x2, which might
> be
> passed by a previous stage bootloader. Have the caller save them.
> ---
> Rouven, does this work for you?

No, this does not fix the issue. I'll take some time to look into this
at a later point.

- rcz


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

Re: [PATCH] ARM: aarch64: save clobbered registers in __barebox_arm_entry

[Ahmad Fatoum]

On 10/2/19 11:30 AM, Rouven Czerwinski wrote:
> On Wed, 2019-10-02 at 09:57 +0200, Ahmad Fatoum wrote:
>> arm_early_mmu_cache_invalidate now clobbers x0, x1, x2, which might
>> be
>> passed by a previous stage bootloader. Have the caller save them.
>> ---
>> Rouven, does this work for you?
> 
> No, this does not fix the issue. I'll take some time to look into this
> at a later point.

Hmm, could you tell me a bit about how the i.MX8 boots?
At what stage does it break? barebox PBL? barebox proper?
Is there something running prior to the barebox PBL?
Is the MMU off when barebox is run?

> 
> - rcz
> 
> 


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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