* [PATCH V3] MIPS: flush TLB handlers directly after writing them
@ 2013-06-21 18:48 Jonas Gorski
2013-06-21 19:12 ` David Daney
0 siblings, 1 reply; 3+ messages in thread
From: Jonas Gorski @ 2013-06-21 18:48 UTC (permalink / raw)
To: linux-mips, Ralf Baechle; +Cc: Steven J. Hill, Jayachandran C, David Daney
When having enabled MIPS_PGD_C0_CONTEXT, trap_init() might call the
generated tlbmiss_handler_setup_pgd before it was committed to memory,
causing boot failures:
trap_init()
|- per_cpu_trap_init()
| |- TLBMISS_HANDLER_SETUP()
| |- tlbmiss_handler_setup_pgd()
|- flush_tlb_handlers()
To avoid this, move flush_tlb_handlers() into build_tlb_refill_handler()
right after they were generated. We can do this as the cache handling is
initialized just before creating the tlb handlers.
This issue was introduced in 3d8bfdd0307223de678962f1c1907a7cec549136
("MIPS: Use C0_KScratch (if present) to hold PGD pointer.").
Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
@Ralf, this is a direct replacement for V2.
V2 -> V3:
* Move flush_tlb_handlers() call into build_tlb_refill_handler() as
suggested by Jayachandran C.
* Make it static as now doesn't need to be called from external anymore.
* Move it on top of build_tlb_refill_handler() to avoid having to add a
prototype for it.
V1 -> V2:
* Move flush_tlb_handlers into per_cpu_trap_init() to also fix it for
!boot_cpu.
arch/mips/kernel/traps.c | 2 --
arch/mips/mm/tlbex.c | 30 ++++++++++++++++--------------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 142d2be..f44366d 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -1668,7 +1668,6 @@ void *set_vi_handler(int n, vi_handler_t addr)
}
extern void tlb_init(void);
-extern void flush_tlb_handlers(void);
/*
* Timer interrupt
@@ -1997,7 +1996,6 @@ void __init trap_init(void)
set_handler(0x080, &except_vec3_generic, 0x80);
local_flush_icache_range(ebase, ebase + 0x400);
- flush_tlb_handlers();
sort_extable(__start___dbe_table, __stop___dbe_table);
diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index f1eabe7..02b1b22 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -2192,6 +2192,20 @@ static void __cpuinit build_r4000_tlb_modify_handler(void)
dump_handler("r4000_tlb_modify", handle_tlbm, ARRAY_SIZE(handle_tlbm));
}
+static void __cpuinit flush_tlb_handlers(void)
+{
+ local_flush_icache_range((unsigned long)handle_tlbl,
+ (unsigned long)handle_tlbl + sizeof(handle_tlbl));
+ local_flush_icache_range((unsigned long)handle_tlbs,
+ (unsigned long)handle_tlbs + sizeof(handle_tlbs));
+ local_flush_icache_range((unsigned long)handle_tlbm,
+ (unsigned long)handle_tlbm + sizeof(handle_tlbm));
+#ifdef CONFIG_MIPS_PGD_C0_CONTEXT
+ local_flush_icache_range((unsigned long)tlbmiss_handler_setup_pgd_array,
+ (unsigned long)tlbmiss_handler_setup_pgd_array + sizeof(handle_tlbm));
+#endif
+}
+
void __cpuinit build_tlb_refill_handler(void)
{
/*
@@ -2224,6 +2238,7 @@ void __cpuinit build_tlb_refill_handler(void)
build_r3000_tlb_load_handler();
build_r3000_tlb_store_handler();
build_r3000_tlb_modify_handler();
+ flush_tlb_handlers();
run_once++;
}
#else
@@ -2251,23 +2266,10 @@ void __cpuinit build_tlb_refill_handler(void)
build_r4000_tlb_modify_handler();
if (!cpu_has_local_ebase)
build_r4000_tlb_refill_handler();
+ flush_tlb_handlers();
run_once++;
}
if (cpu_has_local_ebase)
build_r4000_tlb_refill_handler();
}
}
-
-void __cpuinit flush_tlb_handlers(void)
-{
- local_flush_icache_range((unsigned long)handle_tlbl,
- (unsigned long)handle_tlbl + sizeof(handle_tlbl));
- local_flush_icache_range((unsigned long)handle_tlbs,
- (unsigned long)handle_tlbs + sizeof(handle_tlbs));
- local_flush_icache_range((unsigned long)handle_tlbm,
- (unsigned long)handle_tlbm + sizeof(handle_tlbm));
-#ifdef CONFIG_MIPS_PGD_C0_CONTEXT
- local_flush_icache_range((unsigned long)tlbmiss_handler_setup_pgd_array,
- (unsigned long)tlbmiss_handler_setup_pgd_array + sizeof(handle_tlbm));
-#endif
-}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH V3] MIPS: flush TLB handlers directly after writing them
2013-06-21 18:48 [PATCH V3] MIPS: flush TLB handlers directly after writing them Jonas Gorski
@ 2013-06-21 19:12 ` David Daney
2013-06-21 20:11 ` Jonas Gorski
0 siblings, 1 reply; 3+ messages in thread
From: David Daney @ 2013-06-21 19:12 UTC (permalink / raw)
To: Jonas Gorski, Ralf Baechle
Cc: linux-mips, Steven J. Hill, Jayachandran C, David Daney
A couple of thoughts about this...
On 06/21/2013 11:48 AM, Jonas Gorski wrote:
> When having enabled MIPS_PGD_C0_CONTEXT, trap_init() might call the
> generated tlbmiss_handler_setup_pgd before it was committed to memory,
> causing boot failures:
>
> trap_init()
> |- per_cpu_trap_init()
> | |- TLBMISS_HANDLER_SETUP()
> | |- tlbmiss_handler_setup_pgd()
> |- flush_tlb_handlers()
>
> To avoid this, move flush_tlb_handlers() into build_tlb_refill_handler()
> right after they were generated. We can do this as the cache handling is
> initialized just before creating the tlb handlers.
>
> This issue was introduced in 3d8bfdd0307223de678962f1c1907a7cec549136
> ("MIPS: Use C0_KScratch (if present) to hold PGD pointer.").
>
> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> ---
> @Ralf, this is a direct replacement for V2.
>
> V2 -> V3:
> * Move flush_tlb_handlers() call into build_tlb_refill_handler() as
> suggested by Jayachandran C.
> * Make it static as now doesn't need to be called from external anymore.
> * Move it on top of build_tlb_refill_handler() to avoid having to add a
> prototype for it.
>
> V1 -> V2:
> * Move flush_tlb_handlers into per_cpu_trap_init() to also fix it for
> !boot_cpu.
>
> arch/mips/kernel/traps.c | 2 --
> arch/mips/mm/tlbex.c | 30 ++++++++++++++++--------------
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index 142d2be..f44366d 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -1668,7 +1668,6 @@ void *set_vi_handler(int n, vi_handler_t addr)
> }
>
> extern void tlb_init(void);
> -extern void flush_tlb_handlers(void);
>
> /*
> * Timer interrupt
> @@ -1997,7 +1996,6 @@ void __init trap_init(void)
> set_handler(0x080, &except_vec3_generic, 0x80);
>
> local_flush_icache_range(ebase, ebase + 0x400);
> - flush_tlb_handlers();
>
> sort_extable(__start___dbe_table, __stop___dbe_table);
>
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index f1eabe7..02b1b22 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -2192,6 +2192,20 @@ static void __cpuinit build_r4000_tlb_modify_handler(void)
> dump_handler("r4000_tlb_modify", handle_tlbm, ARRAY_SIZE(handle_tlbm));
> }
>
> +static void __cpuinit flush_tlb_handlers(void)
> +{
> + local_flush_icache_range((unsigned long)handle_tlbl,
> + (unsigned long)handle_tlbl + sizeof(handle_tlbl));
> + local_flush_icache_range((unsigned long)handle_tlbs,
> + (unsigned long)handle_tlbs + sizeof(handle_tlbs));
> + local_flush_icache_range((unsigned long)handle_tlbm,
> + (unsigned long)handle_tlbm + sizeof(handle_tlbm));
> +#ifdef CONFIG_MIPS_PGD_C0_CONTEXT
> + local_flush_icache_range((unsigned long)tlbmiss_handler_setup_pgd_array,
> + (unsigned long)tlbmiss_handler_setup_pgd_array + sizeof(handle_tlbm));
> +#endif
> +}
> +
1) Why not move the local_flush_icache_range() into the build_*
function right after the point where the instructions are written
to their final location?
Having this out-of-line flusher seems like it is patching up
something we forgot to do when we generated the code.
2) Also what happens on the other CPUs? Don't they need their icache
invalidated as well? How is that handled?
3) Why is is called local_flush_icache_range? It seems like
local_invalidate... would be better
> void __cpuinit build_tlb_refill_handler(void)
> {
> /*
> @@ -2224,6 +2238,7 @@ void __cpuinit build_tlb_refill_handler(void)
> build_r3000_tlb_load_handler();
> build_r3000_tlb_store_handler();
> build_r3000_tlb_modify_handler();
> + flush_tlb_handlers();
> run_once++;
> }
> #else
> @@ -2251,23 +2266,10 @@ void __cpuinit build_tlb_refill_handler(void)
> build_r4000_tlb_modify_handler();
> if (!cpu_has_local_ebase)
> build_r4000_tlb_refill_handler();
> + flush_tlb_handlers();
> run_once++;
> }
> if (cpu_has_local_ebase)
> build_r4000_tlb_refill_handler();
> }
> }
> -
> -void __cpuinit flush_tlb_handlers(void)
> -{
> - local_flush_icache_range((unsigned long)handle_tlbl,
> - (unsigned long)handle_tlbl + sizeof(handle_tlbl));
> - local_flush_icache_range((unsigned long)handle_tlbs,
> - (unsigned long)handle_tlbs + sizeof(handle_tlbs));
> - local_flush_icache_range((unsigned long)handle_tlbm,
> - (unsigned long)handle_tlbm + sizeof(handle_tlbm));
> -#ifdef CONFIG_MIPS_PGD_C0_CONTEXT
> - local_flush_icache_range((unsigned long)tlbmiss_handler_setup_pgd_array,
> - (unsigned long)tlbmiss_handler_setup_pgd_array + sizeof(handle_tlbm));
> -#endif
> -}
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH V3] MIPS: flush TLB handlers directly after writing them
2013-06-21 19:12 ` David Daney
@ 2013-06-21 20:11 ` Jonas Gorski
0 siblings, 0 replies; 3+ messages in thread
From: Jonas Gorski @ 2013-06-21 20:11 UTC (permalink / raw)
To: David Daney
Cc: Ralf Baechle, linux-mips, Steven J. Hill, Jayachandran C,
David Daney
On Fri, 21 Jun 2013 12:12:37 -0700
David Daney <ddaney.cavm@gmail.com> wrote:
> A couple of thoughts about this...
>
> On 06/21/2013 11:48 AM, Jonas Gorski wrote:
> > When having enabled MIPS_PGD_C0_CONTEXT, trap_init() might call the
> > generated tlbmiss_handler_setup_pgd before it was committed to memory,
> > causing boot failures:
> >
> > trap_init()
> > |- per_cpu_trap_init()
> > | |- TLBMISS_HANDLER_SETUP()
> > | |- tlbmiss_handler_setup_pgd()
> > |- flush_tlb_handlers()
> >
> > To avoid this, move flush_tlb_handlers() into build_tlb_refill_handler()
> > right after they were generated. We can do this as the cache handling is
> > initialized just before creating the tlb handlers.
> >
> > This issue was introduced in 3d8bfdd0307223de678962f1c1907a7cec549136
> > ("MIPS: Use C0_KScratch (if present) to hold PGD pointer.").
> >
> > Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> > ---
> > @Ralf, this is a direct replacement for V2.
> >
> > V2 -> V3:
> > * Move flush_tlb_handlers() call into build_tlb_refill_handler() as
> > suggested by Jayachandran C.
> > * Make it static as now doesn't need to be called from external anymore.
> > * Move it on top of build_tlb_refill_handler() to avoid having to add a
> > prototype for it.
> >
> > V1 -> V2:
> > * Move flush_tlb_handlers into per_cpu_trap_init() to also fix it for
> > !boot_cpu.
> >
> > arch/mips/kernel/traps.c | 2 --
> > arch/mips/mm/tlbex.c | 30 ++++++++++++++++--------------
> > 2 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> > index 142d2be..f44366d 100644
> > --- a/arch/mips/kernel/traps.c
> > +++ b/arch/mips/kernel/traps.c
> > @@ -1668,7 +1668,6 @@ void *set_vi_handler(int n, vi_handler_t addr)
> > }
> >
> > extern void tlb_init(void);
> > -extern void flush_tlb_handlers(void);
> >
> > /*
> > * Timer interrupt
> > @@ -1997,7 +1996,6 @@ void __init trap_init(void)
> > set_handler(0x080, &except_vec3_generic, 0x80);
> >
> > local_flush_icache_range(ebase, ebase + 0x400);
> > - flush_tlb_handlers();
> >
> > sort_extable(__start___dbe_table, __stop___dbe_table);
> >
> > diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> > index f1eabe7..02b1b22 100644
> > --- a/arch/mips/mm/tlbex.c
> > +++ b/arch/mips/mm/tlbex.c
> > @@ -2192,6 +2192,20 @@ static void __cpuinit build_r4000_tlb_modify_handler(void)
> > dump_handler("r4000_tlb_modify", handle_tlbm, ARRAY_SIZE(handle_tlbm));
> > }
> >
> > +static void __cpuinit flush_tlb_handlers(void)
> > +{
> > + local_flush_icache_range((unsigned long)handle_tlbl,
> > + (unsigned long)handle_tlbl + sizeof(handle_tlbl));
> > + local_flush_icache_range((unsigned long)handle_tlbs,
> > + (unsigned long)handle_tlbs + sizeof(handle_tlbs));
> > + local_flush_icache_range((unsigned long)handle_tlbm,
> > + (unsigned long)handle_tlbm + sizeof(handle_tlbm));
> > +#ifdef CONFIG_MIPS_PGD_C0_CONTEXT
> > + local_flush_icache_range((unsigned long)tlbmiss_handler_setup_pgd_array,
> > + (unsigned long)tlbmiss_handler_setup_pgd_array + sizeof(handle_tlbm));
> > +#endif
> > +}
> > +
>
> 1) Why not move the local_flush_icache_range() into the build_*
> function right after the point where the instructions are written
> to their final location?
>
> Having this out-of-line flusher seems like it is patching up
> something we forgot to do when we generated the code.
This was done this way for historical reasons, at least I was told so.
but whatever was resonsible for that isn't true anymore.
This sounds like a good clean up, but I think this is rather something
for a follow up patch (just like fixing the end argument for the last
cache flush that uses the wrong array for sizeof).
> 2) Also what happens on the other CPUs? Don't they need their icache
> invalidated as well? How is that handled?
The cache_init() called directly before flush_tlb_handlers() flushes
all caches, except on the boot cpu, where the flush was done before the
handlers are build.
> 3) Why is is called local_flush_icache_range? It seems like
> local_invalidate... would be better
local_flush_icache_range() also flushes the dcache if necessary. So perhaps it
should be called
local_invalidate_icache_and_maybe_flush_dcache_range ;-).
It seems all icache invalidate functions are named "flush". Not sure if
it's worth "fixing" the naming.
Jonas
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-06-21 20:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-21 18:48 [PATCH V3] MIPS: flush TLB handlers directly after writing them Jonas Gorski
2013-06-21 19:12 ` David Daney
2013-06-21 20:11 ` Jonas Gorski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox