Re: [PATCH 15/17] powerpc/ftrace: Implement ftrace_replace_code()

2023-06-22 Thread Christophe Leroy


Le 19/06/2023 à 11:47, Naveen N Rao a écrit :
> Implement ftrace_replace_code() to consolidate logic from the different
> ftrace patching routines: ftrace_make_nop(), ftrace_make_call() and
> ftrace_modify_call(). Note that ftrace_make_call() is still required
> primarily to handle patching modules during their load time. The other
> two routines should no longer be called.
> 
> This lays the groundwork to enable better control in patching ftrace
> locations, including the ability to nop-out preceding profiling
> instructions when ftrace is disabled.
> 
> Signed-off-by: Naveen N Rao 

Reviewed-by: Christophe Leroy 


> ---
>   arch/powerpc/kernel/trace/ftrace.c | 173 -
>   1 file changed, 96 insertions(+), 77 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/trace/ftrace.c 
> b/arch/powerpc/kernel/trace/ftrace.c
> index 422dd760fbe013..cf9dce77527920 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -94,104 +94,123 @@ static unsigned long find_ftrace_tramp(unsigned long ip)
>   return 0;
>   }
>   
> +static int ftrace_get_call_inst(struct dyn_ftrace *rec, unsigned long addr, 
> ppc_inst_t *call_inst)
> +{
> + unsigned long ip = rec->ip;
> + unsigned long stub;
> +
> + if (is_offset_in_branch_range(addr - ip)) {
> + /* Within range */
> + stub = addr;
> +#ifdef CONFIG_MODULES
> + } else if (rec->arch.mod) {
> + /* Module code would be going to one of the module stubs */
> + stub = (addr == (unsigned long)ftrace_caller ? 
> rec->arch.mod->arch.tramp :
> +
> rec->arch.mod->arch.tramp_regs);
> +#endif
> + } else if (core_kernel_text(ip)) {
> + /* We would be branching to one of our ftrace stubs */
> + stub = find_ftrace_tramp(ip);
> + if (!stub) {
> + pr_err("0x%lx: No ftrace stubs reachable\n", ip);
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
> +
> + *call_inst = ftrace_create_branch_inst(ip, stub, 1);
> + return 0;
> +}
> +
>   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>   int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, 
> unsigned long addr)
>   {
> - unsigned long tramp, tramp_old, ip = rec->ip;
> - ppc_inst_t old, new;
> - struct module *mod;
> -
> - if (is_offset_in_branch_range(old_addr - ip) && 
> is_offset_in_branch_range(addr - ip)) {
> - /* Within range */
> - old = ftrace_create_branch_inst(ip, old_addr, 1);
> - new = ftrace_create_branch_inst(ip, addr, 1);
> - return ftrace_modify_code(ip, old, new);
> - } else if (core_kernel_text(ip)) {
> - /*
> -  * We always patch out of range locations to go to the regs
> -  * variant, so there is nothing to do here
> -  */
> - return 0;
> - } else if (IS_ENABLED(CONFIG_MODULES)) {
> - /* Module code would be going to one of the module stubs */
> - mod = rec->arch.mod;
> - if (addr == (unsigned long)ftrace_caller) {
> - tramp_old = mod->arch.tramp_regs;
> - tramp = mod->arch.tramp;
> - } else {
> - tramp_old = mod->arch.tramp;
> - tramp = mod->arch.tramp_regs;
> - }
> - old = ftrace_create_branch_inst(ip, tramp_old, 1);
> - new = ftrace_create_branch_inst(ip, tramp, 1);
> - return ftrace_modify_code(ip, old, new);
> - }
> -
> + /* This should never be called since we override ftrace_replace_code() 
> */
> + WARN_ON(1);
>   return -EINVAL;
>   }
>   #endif
>   
>   int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>   {
> - unsigned long tramp, ip = rec->ip;
>   ppc_inst_t old, new;
> - struct module *mod;
> + int ret;
> +
> + /* This can only ever be called during module load */
> + if (WARN_ON(!IS_ENABLED(CONFIG_MODULES) || core_kernel_text(rec->ip)))
> + return -EINVAL;
>   
>   old = ppc_inst(PPC_RAW_NOP());
> - if (is_offset_in_branch_range(addr - ip)) {
> - /* Within range */
> - new = ftrace_create_branch_inst(ip, addr, 1);
> - return ftrace_modify_code(ip, old, new);
> - } else if (core_kernel_text(ip)) {
> - /* We would be branching to one of our ftrace tramps */
> - tramp = find_ftrace_tramp(ip);
> - if (!tramp) {
> - pr_err("0x%lx: No ftrace trampolines reachable\n", ip);
> - return -EINVAL;
> - }
> - new = ftrace_create_branch_inst(ip, tramp, 1);
> - return ftrace_modify_code(ip, old, new);
> - } else if (IS_ENABLED(CONFIG_MODULES)) {
> - /* 

[PATCH 15/17] powerpc/ftrace: Implement ftrace_replace_code()

2023-06-19 Thread Naveen N Rao
Implement ftrace_replace_code() to consolidate logic from the different
ftrace patching routines: ftrace_make_nop(), ftrace_make_call() and
ftrace_modify_call(). Note that ftrace_make_call() is still required
primarily to handle patching modules during their load time. The other
two routines should no longer be called.

This lays the groundwork to enable better control in patching ftrace
locations, including the ability to nop-out preceding profiling
instructions when ftrace is disabled.

Signed-off-by: Naveen N Rao 
---
 arch/powerpc/kernel/trace/ftrace.c | 173 -
 1 file changed, 96 insertions(+), 77 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c 
b/arch/powerpc/kernel/trace/ftrace.c
index 422dd760fbe013..cf9dce77527920 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -94,104 +94,123 @@ static unsigned long find_ftrace_tramp(unsigned long ip)
return 0;
 }
 
+static int ftrace_get_call_inst(struct dyn_ftrace *rec, unsigned long addr, 
ppc_inst_t *call_inst)
+{
+   unsigned long ip = rec->ip;
+   unsigned long stub;
+
+   if (is_offset_in_branch_range(addr - ip)) {
+   /* Within range */
+   stub = addr;
+#ifdef CONFIG_MODULES
+   } else if (rec->arch.mod) {
+   /* Module code would be going to one of the module stubs */
+   stub = (addr == (unsigned long)ftrace_caller ? 
rec->arch.mod->arch.tramp :
+  
rec->arch.mod->arch.tramp_regs);
+#endif
+   } else if (core_kernel_text(ip)) {
+   /* We would be branching to one of our ftrace stubs */
+   stub = find_ftrace_tramp(ip);
+   if (!stub) {
+   pr_err("0x%lx: No ftrace stubs reachable\n", ip);
+   return -EINVAL;
+   }
+   } else {
+   return -EINVAL;
+   }
+
+   *call_inst = ftrace_create_branch_inst(ip, stub, 1);
+   return 0;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, 
unsigned long addr)
 {
-   unsigned long tramp, tramp_old, ip = rec->ip;
-   ppc_inst_t old, new;
-   struct module *mod;
-
-   if (is_offset_in_branch_range(old_addr - ip) && 
is_offset_in_branch_range(addr - ip)) {
-   /* Within range */
-   old = ftrace_create_branch_inst(ip, old_addr, 1);
-   new = ftrace_create_branch_inst(ip, addr, 1);
-   return ftrace_modify_code(ip, old, new);
-   } else if (core_kernel_text(ip)) {
-   /*
-* We always patch out of range locations to go to the regs
-* variant, so there is nothing to do here
-*/
-   return 0;
-   } else if (IS_ENABLED(CONFIG_MODULES)) {
-   /* Module code would be going to one of the module stubs */
-   mod = rec->arch.mod;
-   if (addr == (unsigned long)ftrace_caller) {
-   tramp_old = mod->arch.tramp_regs;
-   tramp = mod->arch.tramp;
-   } else {
-   tramp_old = mod->arch.tramp;
-   tramp = mod->arch.tramp_regs;
-   }
-   old = ftrace_create_branch_inst(ip, tramp_old, 1);
-   new = ftrace_create_branch_inst(ip, tramp, 1);
-   return ftrace_modify_code(ip, old, new);
-   }
-
+   /* This should never be called since we override ftrace_replace_code() 
*/
+   WARN_ON(1);
return -EINVAL;
 }
 #endif
 
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
-   unsigned long tramp, ip = rec->ip;
ppc_inst_t old, new;
-   struct module *mod;
+   int ret;
+
+   /* This can only ever be called during module load */
+   if (WARN_ON(!IS_ENABLED(CONFIG_MODULES) || core_kernel_text(rec->ip)))
+   return -EINVAL;
 
old = ppc_inst(PPC_RAW_NOP());
-   if (is_offset_in_branch_range(addr - ip)) {
-   /* Within range */
-   new = ftrace_create_branch_inst(ip, addr, 1);
-   return ftrace_modify_code(ip, old, new);
-   } else if (core_kernel_text(ip)) {
-   /* We would be branching to one of our ftrace tramps */
-   tramp = find_ftrace_tramp(ip);
-   if (!tramp) {
-   pr_err("0x%lx: No ftrace trampolines reachable\n", ip);
-   return -EINVAL;
-   }
-   new = ftrace_create_branch_inst(ip, tramp, 1);
-   return ftrace_modify_code(ip, old, new);
-   } else if (IS_ENABLED(CONFIG_MODULES)) {
-   /* Module code would be going to one of the module stubs */
-   mod = rec->arch.mod;
-   tramp = (addr == (unsigned long)ftrace_caller ? mod->arch.tramp 
: