Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6fad1ac6-b5ee-39a6-4a7d-2ed2f7efa198@loongson.cn>
Date:   Fri, 19 Aug 2022 18:43:29 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH 6/9] LoongArch: modules/ftrace: Initialize PLT at load
 time
Content-Language: en-US
To:     Qing Zhang <zhangqing@loongson.cn>,
        Huacai Chen <chenhuacai@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>
Cc:     WANG Xuerui <kernel@xen0n.name>, loongarch@lists.linux.dev,
        linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
        Jiaxun Yang <jiaxun.yang@flygoat.com>
References: <20220819081657.7254-1-zhangqing@loongson.cn>
From:   Jinyang He <hejinyang@loongson.cn>
In-Reply-To: <20220819081657.7254-1-zhangqing@loongson.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@loongson.cn>
> ---
>   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.


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

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