Re: [PATCH 09/17] powerpc/ftrace: Stop re-purposing linker generated long branches for ftrace

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 19/06/2023 à 11:47, Naveen N Rao a écrit :
> Commit 67361cf8071286 ("powerpc/ftrace: Handle large kernel configs")
> added ftrace support for ppc64 kernel images with a text section larger
> than 32MB. The patch did two things:
> 1. Add stubs at the end of .text to branch into ftrace_[regs_]caller for
>     functions that were out of branch range.
> 2. Re-purpose linker-generated long branches to _mcount to instead branch
>     to ftrace_[regs_]caller.
> 
> Before that, we only supported kernel .text up to ~32MB. With the above,
> we now support up to ~96MB:
> - The first 32MB of kernel text can branch directly into
>    ftrace_[regs_]caller since that symbol is usually at the beginning.
> - The modified long_branch from (2) above is used by the next 32MB of
>    kernel text.
> - The next 32MB of kernel text can use the stub at the end of text to
>    branch back to ftrace_[regs_]caller.
> 
> While re-purposing the long branch works in practice, it still restricts
> ftrace to kernel text up to ~96MB. The stub at the end of kernel text
> from (1) already enables us to extend ftrace support for kernel text
> up to 64MB, which fulfils the original requirement. Further, once we
> switch to -fpatchable-function-entry, there will not be a long branch
> that we can use.
> 
> Stop re-purposing the linker-generated long branches for ftrace to
> simplify the code. If there are good reasons to support ftrace on
> kernels beyond 64MB, we can consider adding support by using
> -fpatchable-function-entry.
> 
> Signed-off-by: Naveen N Rao <naveen@kernel.org>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   arch/powerpc/kernel/trace/ftrace.c | 110 +++++------------------------
>   1 file changed, 17 insertions(+), 93 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index ef4e49c2c37781..278bf8e52b6e89 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -28,13 +28,7 @@
>   #include <asm/syscall.h>
>   #include <asm/inst.h>
>   
> -/*
> - * We generally only have a single long_branch tramp and at most 2 or 3 plt
> - * tramps generated. But, we don't use the plt tramps currently. We also allot
> - * 2 tramps after .text and .init.text. So, we only end up with around 3 usable
> - * tramps in total. Set aside 8 just to be sure.
> - */
> -#define	NUM_FTRACE_TRAMPS	8
> +#define	NUM_FTRACE_TRAMPS	2
>   static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
>   
>   static ppc_inst_t
> @@ -100,11 +94,6 @@ static int is_bl_op(ppc_inst_t op)
>   	return (ppc_inst_val(op) & ~PPC_LI_MASK) == PPC_RAW_BL(0);
>   }
>   
> -static int is_b_op(ppc_inst_t op)
> -{
> -	return (ppc_inst_val(op) & ~PPC_LI_MASK) == PPC_RAW_BRANCH(0);
> -}
> -
>   static unsigned long find_bl_target(unsigned long ip, ppc_inst_t op)
>   {
>   	int offset;
> @@ -227,11 +216,7 @@ static unsigned long find_ftrace_tramp(unsigned long ip)
>   {
>   	int i;
>   
> -	/*
> -	 * We have the compiler generated long_branch tramps at the end
> -	 * and we prefer those
> -	 */
> -	for (i = NUM_FTRACE_TRAMPS - 1; i >= 0; i--)
> +	for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
>   		if (!ftrace_tramps[i])
>   			continue;
>   		else if (is_offset_in_branch_range(ftrace_tramps[i] - ip))
> @@ -240,75 +225,6 @@ static unsigned long find_ftrace_tramp(unsigned long ip)
>   	return 0;
>   }
>   
> -static int add_ftrace_tramp(unsigned long tramp)
> -{
> -	int i;
> -
> -	for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
> -		if (!ftrace_tramps[i]) {
> -			ftrace_tramps[i] = tramp;
> -			return 0;
> -		}
> -
> -	return -1;
> -}
> -
> -/*
> - * If this is a compiler generated long_branch trampoline (essentially, a
> - * trampoline that has a branch to _mcount()), we re-write the branch to
> - * instead go to ftrace_[regs_]caller() and note down the location of this
> - * trampoline.
> - */
> -static int setup_mcount_compiler_tramp(unsigned long tramp)
> -{
> -	int i;
> -	ppc_inst_t op;
> -	unsigned long ptr;
> -
> -	/* Is this a known long jump tramp? */
> -	for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
> -		if (ftrace_tramps[i] == tramp)
> -			return 0;
> -
> -	/* New trampoline -- read where this goes */
> -	if (copy_inst_from_kernel_nofault(&op, (void *)tramp)) {
> -		pr_debug("Fetching opcode failed.\n");
> -		return -1;
> -	}
> -
> -	/* Is this a 24 bit branch? */
> -	if (!is_b_op(op)) {
> -		pr_debug("Trampoline is not a long branch tramp.\n");
> -		return -1;
> -	}
> -
> -	/* lets find where the pointer goes */
> -	ptr = find_bl_target(tramp, op);
> -
> -	if (ptr != ppc_global_function_entry((void *)_mcount)) {
> -		pr_debug("Trampoline target %p is not _mcount\n", (void *)ptr);
> -		return -1;
> -	}
> -
> -	/* Let's re-write the tramp to go to ftrace_[regs_]caller */
> -	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
> -		ptr = ppc_global_function_entry((void *)ftrace_regs_caller);
> -	else
> -		ptr = ppc_global_function_entry((void *)ftrace_caller);
> -
> -	if (patch_branch((u32 *)tramp, ptr, 0)) {
> -		pr_debug("REL24 out of range!\n");
> -		return -1;
> -	}
> -
> -	if (add_ftrace_tramp(tramp)) {
> -		pr_debug("No tramp locations left\n");
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
>   static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
>   {
>   	unsigned long tramp, ip = rec->ip;
> @@ -331,13 +247,10 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
>   
>   	pr_devel("ip:%lx jumps to %lx", ip, tramp);
>   
> -	if (setup_mcount_compiler_tramp(tramp)) {
> -		/* Are other trampolines reachable? */
> -		if (!find_ftrace_tramp(ip)) {
> -			pr_err("No ftrace trampolines reachable from %ps\n",
> -					(void *)ip);
> -			return -EINVAL;
> -		}
> +	/* Are ftrace trampolines reachable? */
> +	if (!find_ftrace_tramp(ip)) {
> +		pr_err("No ftrace trampolines reachable from %ps\n", (void *)ip);
> +		return -EINVAL;
>   	}
>   
>   	if (patch_instruction((u32 *)ip, ppc_inst(PPC_RAW_NOP()))) {
> @@ -725,6 +638,17 @@ void ftrace_free_init_tramp(void)
>   		}
>   }
>   
> +static void __init add_ftrace_tramp(unsigned long tramp)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
> +		if (!ftrace_tramps[i]) {
> +			ftrace_tramps[i] = tramp;
> +			return;
> +		}
> +}
> +
>   int __init ftrace_dyn_arch_init(void)
>   {
>   	unsigned int *tramp[] = { ftrace_tramp_text, ftrace_tramp_init };