ARM: setup_mm_for_reboot(): use flush_cache_louis()

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/mm/idmap.c +0 -0

index ab88ed4f8e..2c61085a10 100644

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/mm/idmap.c +0 -0

index ab88ed4f8e..2c61085a10 100644

Quoted 20 lines · Nicolas Pitre

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/mm/idmap.c +1 -1

index ab88ed4f8e..2c61085a10 100644
@@ -104,7 +104,7 @@ early_initcall(init_static_idmap);
  void setup_mm_for_reboot(void)
  {
  	/* Clean and invalidate L1. */
-	flush_cache_all();
+	flush_cache_louis();
 

arch/arm/mm/idmap.c early_initcall(init_static_idmap); · line 110

Quoted 3 lines

arch/arm/mm/idmap.c +0 -0

  	/* Switch to the identity mapping. */
  	cpu_switch_mm(idmap_pgd, &init_mm);

Nice. Just one difference is that the I-cache invalidation won't happen with this change. Not that it is needed here but capturing that in change-log would be good.

Quoted 21 lines · Santosh Shilimkar

Quoted 20 lines · Nicolas Pitre

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/mm/idmap.c +1 -1

index ab88ed4f8e..2c61085a10 100644
@@ -104,7 +104,7 @@ early_initcall(init_static_idmap);
   void setup_mm_for_reboot(void)
   {
   	/* Clean and invalidate L1. */
-	flush_cache_all();
+	flush_cache_louis();
 

Quoted 6 lines

arch/arm/mm/idmap.c early_initcall(init_static_idmap); · line 110

Quoted 3 lines

arch/arm/mm/idmap.c +0 -0

   	/* Switch to the identity mapping. */
   	cpu_switch_mm(idmap_pgd, &init_mm);

Nice. Just one difference is that the I-cache invalidation won't happen with this change. Not that it is needed here but capturing that in change-log would be good.

Yes, it does happen. The LoUIS API mirrors the flush_cache_all() API in this respect, and it has to. The only change is the data cache level at which it operates.

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Quoted 6 lines · Nicolas Pitre

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Why does the cost at reboot count? It's a relatively slow operation as it is anyway, because you have to wait for the system to shut down, call the boot loader, etc.

However, the opposite argument is that the state of the L2 _shouldn't_ matter - except for one small little detail. Dirty data, which could get evicted and overwrite something that matters. Generally there won't be any dirty data in the L2 cache on normal boot, so this is a situation which boot loaders probably don't expect.

So all in all, I'm not sure of the wiseness of your change. It's likely to cause regressions.

Quoted 11 lines · Russell King - ARM Linux

Quoted 6 lines · Nicolas Pitre

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Why does the cost at reboot count? It's a relatively slow operation as it is anyway, because you have to wait for the system to shut down, call the boot loader, etc.

Because I have a use case with the big.LITTLE switcher where the full boot is bypassed but the kernel must be reintered as if the CPU was powered up. This is of course something that _could_ happen multiple times in a second, and therefore minimizing its unneeded costs is a good thing(tm).

Quoted 5 lines

However, the opposite argument is that the state of the L2 _shouldn't_ matter - except for one small little detail. Dirty data, which could get evicted and overwrite something that matters. Generally there won't be any dirty data in the L2 cache on normal boot, so this is a situation which boot loaders probably don't expect.

In the use case that concerns me, L2 is retained and I'd well prefer if it didn't get flushed at all.

Quoted 2 lines

So all in all, I'm not sure of the wiseness of your change. It's likely to cause regressions.

Could you please tell me if you have such a regression in mind? Of course I could carry the few operations performed by setup_mm_for_reboot() locally, but that looks like useless code duplication.

Quoted 6 lines · Nicolas Pitre

... instead of flush_cache_all(). The later unconditionally flushes the L2 cache on ARMv7 architectures such as Cortex A15 and A7 which is costly and unnecessary in some scenarios where setup_mm_for_reboot() is used. If L2 has to be flushed as well, it should already be done separately on other architectures anyway.

Why does the cost at reboot count? It's a relatively slow operation as it is anyway, because you have to wait for the system to shut down, call the boot loader, etc.

However, the opposite argument is that the state of the L2 _shouldn't_ matter - except for one small little detail. Dirty data, which could get evicted and overwrite something that matters. Generally there won't be any dirty data in the L2 cache on normal boot, so this is a situation which boot loaders probably don't expect.

Wouldn't the L2 flush in this case be included with the code that turns off caching? For reboot, that's currently done in __sort_restart -- the cache flush in setup_mm_for_reboot is just to ensure that the idmap tables are visible to the hardware walker iirc.

Quoted 4 lines · Will Deacon

Wouldn't the L2 flush in this case be included with the code that turns off caching? For reboot, that's currently done in __sort_restart -- the cache flush in setup_mm_for_reboot is just to ensure that the idmap tables are visible to the hardware walker iirc.

Good point - but it does raise another issue. Why do we do this flush and TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if we change existing page tables, but we don't anymore, we're just switching them, and cpu_switch_mm() will do whatever's necessary to make the new page tables visible. So I think we can get rid of that flusing in there.

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Will Deacon

Wouldn't the L2 flush in this case be included with the code that turns off caching? For reboot, that's currently done in __sort_restart -- the cache flush in setup_mm_for_reboot is just to ensure that the idmap tables are visible to the hardware walker iirc.

Good point - but it does raise another issue. Why do we do this flush and TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if we change existing page tables, but we don't anymore, we're just switching them, and cpu_switch_mm() will do whatever's necessary to make the new page tables visible. So I think we can get rid of that flusing in there.

Hmm, I'm not sure about that. Looking at cpu_v7_switch_mm (the 2-level version) for example, we set the ASID and then set the new TTBR. There's no flushing of page tables like we get in set_pte and no TLB flushing either.

Now, given that the idmap_pgd is populated as an early_initcall, I really doubt we need that flush_cache_all but the TLB invalidate is surely required?

Quoted 17 lines · Will Deacon

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Will Deacon

Wouldn't the L2 flush in this case be included with the code that turns off caching? For reboot, that's currently done in __sort_restart -- the cache flush in setup_mm_for_reboot is just to ensure that the idmap tables are visible to the hardware walker iirc.

Good point - but it does raise another issue. Why do we do this flush and TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if we change existing page tables, but we don't anymore, we're just switching them, and cpu_switch_mm() will do whatever's necessary to make the new page tables visible. So I think we can get rid of that flusing in there.

Hmm, I'm not sure about that. Looking at cpu_v7_switch_mm (the 2-level version) for example, we set the ASID and then set the new TTBR. There's no flushing of page tables like we get in set_pte and no TLB flushing either.

Quoted 3 lines

Now, given that the idmap_pgd is populated as an early_initcall, I really doubt we need that flush_cache_all but the TLB invalidate is surely required?

Why wouldn't cpu_switch_mm() take care of that already if that is necessary? Hmmm, I suppose the asid of the init task isn't associated with the idmap in any way, hence the need for flushing the tlbs.

I'd not rely on the early_initcall to assume the new page table is moved out of the cache though.

What about this alternate patch:

arch/arm/mm/idmap.c +0 -0

index ab88ed4f8e..1ab429761c 100644

arch/arm/mm/idmap.c +0 -3

 early_initcall(init_static_idmap);

@@ -103,12 +106,12 @@ early_initcall(init_static_idmap);
  */
 void setup_mm_for_reboot(void)
 {
-	/* Clean and invalidate L1. */
-	flush_cache_all();
-

Quoted 18 lines · Nicolas Pitre

Quoted 17 lines · Will Deacon

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Will Deacon

Wouldn't the L2 flush in this case be included with the code that turns off caching? For reboot, that's currently done in __sort_restart -- the cache flush in setup_mm_for_reboot is just to ensure that the idmap tables are visible to the hardware walker iirc.

Good point - but it does raise another issue. Why do we do this flush and TLB invalidate afterwards in setup_mm_for_reboot() ? It makes sense if we change existing page tables, but we don't anymore, we're just switching them, and cpu_switch_mm() will do whatever's necessary to make the new page tables visible. So I think we can get rid of that flusing in there.

Hmm, I'm not sure about that. Looking at cpu_v7_switch_mm (the 2-level version) for example, we set the ASID and then set the new TTBR. There's no flushing of page tables like we get in set_pte and no TLB flushing either.

Quoted 7 lines

Quoted 3 lines

Now, given that the idmap_pgd is populated as an early_initcall, I really doubt we need that flush_cache_all but the TLB invalidate is surely required?

Why wouldn't cpu_switch_mm() take care of that already if that is necessary? Hmmm, I suppose the asid of the init task isn't associated with the idmap in any way, hence the need for flushing the tlbs.

Yes, cpu_switch_mm can't know about whether tables are visible or an ASID is dirty so all it can do is defer that to the caller or do the cleaning and invalidation every time.

Quoted 2 lines

I'd not rely on the early_initcall to assume the new page table is moved out of the cache though.

Yeah, it's not nice, I was just pointing out that it's all hanging off an initcall now (before it was created ad-hoc by its users).

Quoted 7 lines

What about this alternate patch:

arch/arm/mm/idmap.c +0 -0

index ab88ed4f8e..1ab429761c 100644

arch/arm/mm/idmap.c static int __init init_static_idmap(void) · line 99

Quoted 8 lines

arch/arm/mm/idmap.c +3 -0

@@ -92,6 +92,9 @@ static int __init init_static_idmap(void)
 		(long long)idmap_start, (long long)idmap_end);
 	identity_mapping_add(idmap_pgd, idmap_start, idmap_end);
 
+	/* Flush L1 for the hardware to see this page table content */
+	flush_cache_louis();
+
 	return 0;
 }

Yep, we can do this now. Good thinking! At some point I'll get around to making this conditional, as I don't think it's needed on A5, A7, A9 or A15.

Quoted 8 lines

arch/arm/mm/idmap.c +0 -3

 early_initcall(init_static_idmap);

@@ -103,12 +106,12 @@ early_initcall(init_static_idmap);
  */
 void setup_mm_for_reboot(void)
 {
-	/* Clean and invalidate L1. */
-	flush_cache_all();
-

arch/arm/mm/idmap.c early_initcall(init_static_idmap); · line 117

Quoted 10 lines

arch/arm/mm/idmap.c +4 -1

 	/* Switch to the identity mapping. */
 	cpu_switch_mm(idmap_pgd, &init_mm);
 
-	/* Flush the TLB. */
+	/*
+	 * On ARMv7, the ASID of the init task is not associated with
+	 * this mapping.  TLBs must be flushed.
+	 */
 	local_flush_tlb_all();
 }

Can we change this comment slightly please? Basically, we don't have a clean ASID for the identity mapping, which may clash with virtual addresses of the previous page tables and therefore potentially in the TLB. That's why we need the invalidation: so that we don't hit stale entries from before.

Other than that, looks good to me:

Acked-by Will Deacon <[email protected]>

Here's my latest version. I made the tlb flush conditional. Please review again before I add your ACK.

---- >8
From: Nicolas Pitre <[email protected]>
Subject: [PATCH] ARM: idmap: use flush_cache_louis() and flush TLBs only when necessary

Flushing the cache is needed for the hardware to see the idmap table and therefore can be done at init time. On ARMv7 it is not necessary to flush L2 so flush_cache_louis() is used here instead.

There is no point flushing the cache in setup_mm_for_reboot() as the caller should, and already is, taking care of this. If switching the memory map requires a cache flush, then cpu_switch_mm() already includes that operation.

What is not done by cpu_switch_mm() on ASID capable CPUs is TLB flushing as the whole point of the ASID is to tag the TLBs and avoid flushing them on a context switch. Since we don't have a clean ASID for the identity mapping, we need to flush the TLB explicitly in that case. Otherwise this is already performed by cpu_switch_mm().

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/mm/idmap.c +3 -3

index ab88ed4f8e..99db769307 100644
@@ -92,6 +92,9 @@ static int __init init_static_idmap(void)
 		(long long)idmap_start, (long long)idmap_end);
 	identity_mapping_add(idmap_pgd, idmap_start, idmap_end);
 
+	/* Flush L1 for the hardware to see this page table content */
+	flush_cache_louis();
+
 	return 0;
 }
 early_initcall(init_static_idmap);

@@ -103,12 +106,15 @@ early_initcall(init_static_idmap);
  */
 void setup_mm_for_reboot(void)
 {
-	/* Clean and invalidate L1. */
-	flush_cache_all();
-
 	/* Switch to the identity mapping. */
 	cpu_switch_mm(idmap_pgd, &init_mm);

I checked all of the switch_mm implementations and it looks like !CONFIG_CPU_HAS_ASID implies the TLB flush in all cases, so this looks fine to me.

Acked-by Will Deacon <[email protected]>