Re: [PATCH V3] MIPS: flush TLB handlers directly after writing them

[David Daney <ddaney.cavm@gmail.com>]

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