Re: [PATCH 6/9] LoongArch: modules/ftrace: Initialize PLT at load time

[Qing Zhang]

On 2022/8/19 下午6:43, Jinyang He wrote:
> On 2022/8/19 16:16, Qing Zhang wrote:
>
> > To Implement ftrace trampiones through plt entry.
> >
> > Tested by forcing ftrace_make_call() to use the module PLT, and then
> > loading up a module after setting up ftrace with:
> >
> > | echo ":mod:<module-name>" > set_ftrace_filter;
> > | echo function > current_tracer;
> > | modprobe <module-name>
> >
> > Since FTRACE_ADDR/FTRACE_REGS_ADDR is only defined when 
> > CONFIG_DYNAMIC_FTRACE
> > is selected, we wrap its use along with most of 
> > module_init_ftrace_plt() with
> > ifdeffery rather than using IS_ENABLED().
> >
> > Signed-off-by: Qing Zhang <zhangqing@xxxxxxxxxxx>
> > ---
> >   arch/loongarch/include/asm/ftrace.h     |  4 ++
> >   arch/loongarch/include/asm/inst.h       |  2 +
> >   arch/loongarch/include/asm/module.h     | 14 +++--
> >   arch/loongarch/include/asm/module.lds.h |  1 +
> >   arch/loongarch/kernel/ftrace_dyn.c      | 79 +++++++++++++++++++++++++
> >   arch/loongarch/kernel/inst.c            | 12 ++++
> >   arch/loongarch/kernel/module-sections.c | 11 ++++
> >   arch/loongarch/kernel/module.c          | 48 +++++++++++++++
> >   8 files changed, 166 insertions(+), 5 deletions(-)
> [...]
> > @@ -36,14 +39,15 @@ Elf_Addr module_emit_plt_entry(struct module *mod, 
> > unsigned long val);
> >   static inline struct plt_entry emit_plt_entry(unsigned long val)
> >   {
> > -    u32 lu12iw, lu32id, lu52id, jirl;
> > +    u32 addu16id, lu32id, lu52id, jirl;
> > -    lu12iw = (lu12iw_op << 25 | (((val >> 12) & 0xfffff) << 5) | 
> > LOONGARCH_GPR_T1);
> > +    addu16id = larch_insn_gen_addu16id(LOONGARCH_GPR_T1, 
> > LOONGARCH_GPR_ZERO,
> > +        ADDR_IMM(val, ADDU16ID));
>
> Hi, Qing,
>
> Why change lu12iw to addu16id? They have same effect here.
Hi. Jinyang

Yes, the effect is the same.
I will modify it to lu12iw in V2, which can be consistent with the 
user's plt style.

Thanks,
- Qing

> >       lu32id = larch_insn_gen_lu32id(LOONGARCH_GPR_T1, ADDR_IMM(val, 
> > LU32ID));
> >       lu52id = larch_insn_gen_lu52id(LOONGARCH_GPR_T1, 
> > LOONGARCH_GPR_T1, ADDR_IMM(val, LU52ID));
> > -    jirl = larch_insn_gen_jirl(0, LOONGARCH_GPR_T1, 0, (val & 0xfff));
> > +    jirl = larch_insn_gen_jirl(0, LOONGARCH_GPR_T1, 0, (val & 0xffff));
> > -    return (struct plt_entry) { lu12iw, lu32id, lu52id, jirl };
> > +    return (struct plt_entry) { addu16id, lu32id, lu52id, jirl };
> >   }
> [...]
> > +static int module_init_ftrace_plt(const Elf_Ehdr *hdr, const Elf_Shdr 
> > *sechdrs,
> > +                  struct module *mod)
> > +{
> > +#ifdef CONFIG_DYNAMIC_FTRACE
> > +    const Elf_Shdr *s;
> > +    struct plt_entry *ftrace_plts;
> > +
> > +    s = find_section(hdr, sechdrs, ".ftrace_trampoline");
> > +    if (!s)
> > +        return -ENOEXEC;
>
> Return value is not used later, may we do something or drop returning 
> value.
Yes, Should return module_init_ftrace_plt(hdr, sechdrs, mod) here.

Thanks,
-Qing
> Thanks,
>
> Jinyang
>
> > +
> > +    ftrace_plts = (void *)s->sh_addr;
> > +
> > +    ftrace_plts[FTRACE_PLT_IDX] = emit_plt_entry(FTRACE_ADDR);
> > +
> > +    if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS))
> > +        ftrace_plts[FTRACE_REGS_PLT_IDX] = 
> > emit_plt_entry(FTRACE_REGS_ADDR);
> > +
> > +    mod->arch.ftrace_trampolines = ftrace_plts;
> > +#endif
> > +    return 0;
> > +}
> > +
> > +int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, 
> > struct module *mod)
> > +{
> > +    module_init_ftrace_plt(hdr, sechdrs, mod);
> > +
> > +    return 0;
> > +}