Re: [PATCH v1 08/22] powerpc/ftrace: Make __ftrace_make_{nop/call}() common to PPC32 and PPC64

[Christophe Leroy]

Le 18/04/2022 à 08:40, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Since c93d4f6ecf4b ("powerpc/ftrace: Add module_trampoline_target()
>> for PPC32"), __ftrace_make_nop() for PPC32 is very similar to the
>> one for PPC64.
>>
>> Same for __ftrace_make_call().
>>
>> Make them common.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>> ---
>>  arch/powerpc/kernel/trace/ftrace.c | 108 +++--------------------------
>>  1 file changed, 8 insertions(+), 100 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/trace/ftrace.c 
>> b/arch/powerpc/kernel/trace/ftrace.c
>> index 1b05d33f96c6..2c7e42e439bb 100644
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -114,7 +114,6 @@ static unsigned long find_bl_target(unsigned long 
>> ip, ppc_inst_t op)
>>  }
>>
>>  #ifdef CONFIG_MODULES
>> -#ifdef CONFIG_PPC64
>>  static int
>>  __ftrace_make_nop(struct module *mod,
>>            struct dyn_ftrace *rec, unsigned long addr)
>> @@ -154,10 +153,11 @@ __ftrace_make_nop(struct module *mod,
>>          return -EINVAL;
>>      }
>>
>> -#ifdef CONFIG_MPROFILE_KERNEL
>> -    /* When using -mkernel_profile there is no load to jump over */
>> +    /* When using -mkernel_profile or PPC32 there is no load to jump 
>> over */
>                       -mprofile-kernel
> 
> Since you are modifying that line anyway ^^

ok


> 
> 
>>      pop = ppc_inst(PPC_RAW_NOP());
>>
>> +#ifdef CONFIG_PPC64
>> +#ifdef CONFIG_MPROFILE_KERNEL
>>      if (copy_inst_from_kernel_nofault(&op, (void *)(ip - 4))) {
>>          pr_err("Fetching instruction at %lx failed.\n", ip - 4);
>>          return -EFAULT;
>> @@ -201,6 +201,7 @@ __ftrace_make_nop(struct module *mod,
>>          return -EINVAL;
>>      }
>>  #endif /* CONFIG_MPROFILE_KERNEL */
>> +#endif /* PPC64 */
>>
>>      if (patch_instruction((u32 *)ip, pop)) {
>>          pr_err("Patching NOP failed.\n");
>> @@ -209,48 +210,6 @@ __ftrace_make_nop(struct module *mod,
>>
>>      return 0;
>>  }
>> -
>> -#else /* !PPC64 */
>> -static int
>> -__ftrace_make_nop(struct module *mod,
>> -          struct dyn_ftrace *rec, unsigned long addr)
>> -{
>> -    ppc_inst_t op;
>> -    unsigned long ip = rec->ip;
>> -    unsigned long tramp, ptr;
>> -
>> -    if (copy_from_kernel_nofault(&op, (void *)ip, MCOUNT_INSN_SIZE))
>> -        return -EFAULT;
>> -
>> -    /* Make sure that that this is still a 24bit jump */
>> -    if (!is_bl_op(op)) {
>> -        pr_err("Not expected bl: opcode is %s\n", ppc_inst_as_str(op));
>> -        return -EINVAL;
>> -    }
>> -
>> -    /* lets find where the pointer goes */
>> -    tramp = find_bl_target(ip, op);
>> -
>> -    /* Find where the trampoline jumps to */
>> -    if (module_trampoline_target(mod, tramp, &ptr)) {
>> -        pr_err("Failed to get trampoline target\n");
>> -        return -EFAULT;
>> -    }
>> -
>> -    if (ptr != addr) {
>> -        pr_err("Trampoline location %08lx does not match addr\n",
>> -               tramp);
>> -        return -EINVAL;
>> -    }
>> -
>> -    op = ppc_inst(PPC_RAW_NOP());
>> -
>> -    if (patch_instruction((u32 *)ip, op))
>> -        return -EPERM;
>> -
>> -    return 0;
>> -}
>> -#endif /* PPC64 */
>>  #endif /* CONFIG_MODULES */
>>
>>  static unsigned long find_ftrace_tramp(unsigned long ip)
>> @@ -437,13 +396,12 @@ int ftrace_make_nop(struct module *mod,
>>  }
>>
>>  #ifdef CONFIG_MODULES
>> -#ifdef CONFIG_PPC64
>>  /*
>>   * Examine the existing instructions for __ftrace_make_call.
>>   * They should effectively be a NOP, and follow formal constraints,
>>   * depending on the ABI. Return false if they don't.
>>   */
>> -#ifndef CONFIG_MPROFILE_KERNEL
>> +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> 
> It is better to gate this on PPC64_ELF_ABI_v1

Well ... let then first change this to a CONFIG_ item, it should 
simplify some stuff here and there.

> 
>>  static int
>>  expected_nop_sequence(void *ip, ppc_inst_t op0, ppc_inst_t op1)
>>  {
>> @@ -465,7 +423,7 @@ expected_nop_sequence(void *ip, ppc_inst_t op0, 
>> ppc_inst_t op1)
>>  static int
>>  expected_nop_sequence(void *ip, ppc_inst_t op0, ppc_inst_t op1)
>>  {
>> -    /* look for patched "NOP" on ppc64 with -mprofile-kernel */
>> +    /* look for patched "NOP" on ppc64 with -mprofile-kernel or ppc32 */
>>      if (!ppc_inst_equal(op0, ppc_inst(PPC_RAW_NOP())))
>>          return 0;
>>      return 1;
>> @@ -484,8 +442,10 @@ __ftrace_make_call(struct dyn_ftrace *rec, 
>> unsigned long addr)
>>      if (copy_inst_from_kernel_nofault(op, ip))
>>          return -EFAULT;
>>
>> +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>      if (copy_inst_from_kernel_nofault(op + 1, ip + 4))
>>          return -EFAULT;
>> +#endif
> 
> Here too...
> 

Ok

Christophe