Hi Alex,
On 2024-02-28 11:51 AM, Alexandre Ghiti wrote:
> For now, we use stop_machine() to patch the text and when we use IPIs for
> remote icache flushes (which is emitted in patch_text_nosync()), the system
> hangs.
>
> So instead, make sure every cpu executes the stop_machine() patching
> function and emit a local icache flush there.
>
> Co-developed-by: Björn Töpel <bj...@rivosinc.com>
> Signed-off-by: Björn Töpel <bj...@rivosinc.com>
> Signed-off-by: Alexandre Ghiti <alexgh...@rivosinc.com>
> ---
> arch/riscv/include/asm/patch.h | 1 +
> arch/riscv/kernel/ftrace.c | 42 ++++++++++++++++++++++++++++++----
> arch/riscv/kernel/patch.c | 18 +++++++++------
> 3 files changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
> index e88b52d39eac..9f5d6e14c405 100644
> --- a/arch/riscv/include/asm/patch.h
> +++ b/arch/riscv/include/asm/patch.h
> @@ -6,6 +6,7 @@
> #ifndef _ASM_RISCV_PATCH_H
> #define _ASM_RISCV_PATCH_H
>
> +int patch_insn_write(void *addr, const void *insn, size_t len);
> int patch_text_nosync(void *addr, const void *insns, size_t len);
> int patch_text_set_nosync(void *addr, u8 c, size_t len);
> int patch_text(void *addr, u32 *insns, int ninsns);
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index f5aa24d9e1c1..5654966c4e7d 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -8,6 +8,7 @@
> #include <linux/ftrace.h>
> #include <linux/uaccess.h>
> #include <linux/memory.h>
> +#include <linux/stop_machine.h>
> #include <asm/cacheflush.h>
> #include <asm/patch.h>
>
> @@ -75,8 +76,7 @@ static int __ftrace_modify_call(unsigned long hook_pos,
> unsigned long target,
> make_call_t0(hook_pos, target, call);
>
> /* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
> - if (patch_text_nosync
> - ((void *)hook_pos, enable ? call : nops, MCOUNT_INSN_SIZE))
> + if (patch_insn_write((void *)hook_pos, enable ? call : nops,
> MCOUNT_INSN_SIZE))
> return -EPERM;
>
> return 0;
> @@ -88,7 +88,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long
> addr)
>
> make_call_t0(rec->ip, addr, call);
>
> - if (patch_text_nosync((void *)rec->ip, call, MCOUNT_INSN_SIZE))
> + if (patch_insn_write((void *)rec->ip, call, MCOUNT_INSN_SIZE))
> return -EPERM;
>
> return 0;
> @@ -99,7 +99,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace
> *rec,
> {
> unsigned int nops[2] = {NOP4, NOP4};
>
> - if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> + if (patch_insn_write((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> return -EPERM;
>
> return 0;
> @@ -134,6 +134,40 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
>
> return ret;
> }
> +
> +struct ftrace_modify_param {
> + int command;
> + atomic_t cpu_count;
> +};
> +
> +static int __ftrace_modify_code(void *data)
> +{
> + struct ftrace_modify_param *param = data;
> +
> + if (atomic_inc_return(¶m->cpu_count) == num_online_cpus()) {
> + ftrace_modify_all_code(param->command);
> + /*
> + * Make sure the patching store is effective *before* we
> + * increment the counter which releases all waiting cpus
> + * by using the release version of atomic increment.
> + */
> + atomic_inc_return_release(¶m->cpu_count);
> + } else {
> + while (atomic_read(¶m->cpu_count) <= num_online_cpus())
> + cpu_relax();
> + }
> +
> + local_flush_icache_all();
> +
> + return 0;
> +}
> +
> +void arch_ftrace_update_code(int command)
> +{
> + struct ftrace_modify_param param = { command, ATOMIC_INIT(0) };
> +
> + stop_machine(__ftrace_modify_code, ¶m, cpu_online_mask);
> +}
> #endif
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 0b5c16dfe3f4..82d8508c765b 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -188,7 +188,7 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len)
> }
> NOKPROBE_SYMBOL(patch_text_set_nosync);
>
> -static int patch_insn_write(void *addr, const void *insn, size_t len)
> +int patch_insn_write(void *addr, const void *insn, size_t len)
> {
> size_t patched = 0;
> size_t size;
> @@ -211,11 +211,9 @@ NOKPROBE_SYMBOL(patch_insn_write);
>
> int patch_text_nosync(void *addr, const void *insns, size_t len)
> {
> - u32 *tp = addr;
> int ret;
>
> - ret = patch_insn_write(tp, insns, len);
> -
> + ret = patch_insn_write(addr, insns, len);
> if (!ret)
> flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
This only happens to compile because flush_icache_range() is a macro that
ignores its parameters. You could replace tp with addr in this line as well, but
that seems like more of a cosmetic change and should be a separate patch (like
in [1] which covers both related functions) if you respin this.
Regards,
Samuel
[1]:
https://lore.kernel.org/linux-riscv/20240212025529.1971876-8-samuel.holl...@sifive.com/
>
> @@ -232,15 +230,21 @@ static int patch_text_cb(void *data)
> if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) {
> for (i = 0; ret == 0 && i < patch->ninsns; i++) {
> len = GET_INSN_LENGTH(patch->insns[i]);
> - ret = patch_text_nosync(patch->addr + i * len,
> - &patch->insns[i], len);
> + ret = patch_insn_write(patch->addr + i * len,
> &patch->insns[i], len);
> }
> - atomic_inc(&patch->cpu_count);
> + /*
> + * Make sure the patching store is effective *before* we
> + * increment the counter which releases all waiting cpus
> + * by using the release version of atomic increment.
> + */
> + atomic_inc_return_release(&patch->cpu_count);
> } else {
> while (atomic_read(&patch->cpu_count) <= num_online_cpus())
> cpu_relax();
> }
>
> + local_flush_icache_all();
> +
> return ret;
> }
> NOKPROBE_SYMBOL(patch_text_cb);
