[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[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 <nico@linaro.org>

diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index ab88ed4f8e..2c61085a10 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -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();
 
 	/* Switch to the identity mapping. */
 	cpu_switch_mm(idmap_pgd, &init_mm);

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Will Deacon]

On Tue, Nov 06, 2012 at 09:12:27PM +0000, Nicolas Pitre wrote:
> 
> ... 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 <nico@linaro.org>
> 
> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> index ab88ed4f8e..2c61085a10 100644
> --- a/arch/arm/mm/idmap.c
> +++ b/arch/arm/mm/idmap.c
> @@ -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();
>  
>  	/* Switch to the identity mapping. */
>  	cpu_switch_mm(idmap_pgd, &init_mm);

We could probably even predicate this on the half-implemented
TLB_CAN_READ_FROM_L1_CACHE option, but that can come later.

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Santosh Shilimkar]

On Tuesday 06 November 2012 03:12 PM, Nicolas Pitre wrote:
>
> ... 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 <nico@linaro.org>
>
> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> index ab88ed4f8e..2c61085a10 100644
> --- a/arch/arm/mm/idmap.c
> +++ b/arch/arm/mm/idmap.c
> @@ -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();
>
>   	/* 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.

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Russell King - ARM Linux]

On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote:
> 
> ... 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.

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Nicolas Pitre]

On Tue, 6 Nov 2012, Russell King - ARM Linux wrote:

> On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote:
> > 
> > ... 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).

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

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


Nicolas

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Lorenzo Pieralisi]

On Tue, Nov 06, 2012 at 09:57:17PM +0000, Santosh Shilimkar wrote:
> On Tuesday 06 November 2012 03:12 PM, Nicolas Pitre wrote:
> >
> > ... 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 <nico@linaro.org>
> >
> > diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> > index ab88ed4f8e..2c61085a10 100644
> > --- a/arch/arm/mm/idmap.c
> > +++ b/arch/arm/mm/idmap.c
> > @@ -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();
> >
> >   	/* 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.

Lorenzo

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Will Deacon]

On Tue, Nov 06, 2012 at 10:04:58PM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote:
> > 
> > ... 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.

Will

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Russell King - ARM Linux]

On Tue, Nov 06, 2012 at 05:53:20PM -0500, Nicolas Pitre wrote:
> On Tue, 6 Nov 2012, Russell King - ARM Linux wrote:
> 
> > On Tue, Nov 06, 2012 at 04:12:27PM -0500, Nicolas Pitre wrote:
> > > 
> > > ... 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).
> 
> > 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.
> 
> > 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.

I think I included all the relevant information in the original email.

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Russell King - ARM Linux]

On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote:
> 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.

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Will Deacon]

On Wed, Nov 07, 2012 at 09:56:35AM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote:
> > 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?

Will

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Santosh Shilimkar]

On Wednesday 07 November 2012 03:47 AM, Lorenzo Pieralisi wrote:
> On Tue, Nov 06, 2012 at 09:57:17PM +0000, Santosh Shilimkar wrote:
>> On Tuesday 06 November 2012 03:12 PM, Nicolas Pitre wrote:
>>>
>>> ... 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 <nico@linaro.org>
>>>
>>> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
>>> index ab88ed4f8e..2c61085a10 100644
>>> --- a/arch/arm/mm/idmap.c
>>> +++ b/arch/arm/mm/idmap.c
>>> @@ -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();
>>>
>>>    	/* 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.
>
Indeed. I remember our discussion on this part now. Thanks Lorenzo
for clarification.

Regards
Santosh

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Nicolas Pitre]

On Wed, 7 Nov 2012, Will Deacon wrote:

> On Wed, Nov 07, 2012 at 09:56:35AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote:
> > > 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?

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:

diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index ab88ed4f8e..1ab429761c 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -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,12 @@ 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);
 
-	/* 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();
 }

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Will Deacon]

On Wed, Nov 07, 2012 at 06:03:40PM +0000, Nicolas Pitre wrote:
> On Wed, 7 Nov 2012, Will Deacon wrote:
> > On Wed, Nov 07, 2012 at 09:56:35AM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote:
> > > > 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?
> 
> 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.

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

> What about this alternate patch:
> 
> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> index ab88ed4f8e..1ab429761c 100644
> --- a/arch/arm/mm/idmap.c
> +++ b/arch/arm/mm/idmap.c
> @@ -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.

>  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();
> -
>  	/* 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 <will.deacon@arm.com>

Cheers,

Will

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Nicolas Pitre]

On Wed, 7 Nov 2012, Will Deacon wrote:

> On Wed, Nov 07, 2012 at 06:03:40PM +0000, Nicolas Pitre wrote:
> > On Wed, 7 Nov 2012, Will Deacon wrote:
> > > On Wed, Nov 07, 2012 at 09:56:35AM +0000, Russell King - ARM Linux wrote:
> > > > On Wed, Nov 07, 2012 at 09:51:06AM +0000, Will Deacon wrote:
> > > > > 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?
> > 
> > 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.
> 
> > 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).
> 
> > What about this alternate patch:
> > 
> > diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> > index ab88ed4f8e..1ab429761c 100644
> > --- a/arch/arm/mm/idmap.c
> > +++ b/arch/arm/mm/idmap.c
> > @@ -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.
> 
> >  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();
> > -
> >  	/* 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 <will.deacon@arm.com>

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

---- >8
From: Nicolas Pitre <nicolas.pitre@linaro.org>
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 <nico@linaro.org>

diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index ab88ed4f8e..99db769307 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -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);
 
-	/* Flush the TLB. */
+#ifdef CONFIG_CPU_HAS_ASID
+	/*
+	 * 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.
+	 */
 	local_flush_tlb_all();
+#endif
 }

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Will Deacon]

On Wed, Nov 07, 2012 at 08:10:49PM +0000, Nicolas Pitre wrote:
> Here's my latest version.  I made the tlb flush conditional.  Please 
> review again before I add your ACK.

Ok.

> ---- >8
> From: Nicolas Pitre <nicolas.pitre@linaro.org>
> 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 <nico@linaro.org>
> 
> diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
> index ab88ed4f8e..99db769307 100644
> --- a/arch/arm/mm/idmap.c
> +++ b/arch/arm/mm/idmap.c
> @@ -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);
>  
> -	/* Flush the TLB. */
> +#ifdef CONFIG_CPU_HAS_ASID
> +	/*
> +	 * 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.
> +	 */
>  	local_flush_tlb_all();
> +#endif

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 <will.deacon@arm.com>

Cheers,

Will

[PATCH] ARM: setup_mm_for_reboot(): use flush_cache_louis()

[Nicolas Pitre]

On Thu, 8 Nov 2012, Will Deacon wrote:

> 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 <will.deacon@arm.com>

Submitted as patch 7573/1.

Thanks!


Nicolas