From: Samuel Holland <samuel.holland@sifive.com> To: Alexandre Ghiti <alexghiti@rivosinc.com> Cc: "Björn Töpel" <bjorn@rivosinc.com>, "Paul Walmsley" <paul.walmsley@sifive.com>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Albert Ou" <aou@eecs.berkeley.edu>, "Andrea Parri" <andrea@rivosinc.com>, "Anup Patel" <anup@brainfault.org>, "Steven Rostedt" <rostedt@goodmis.org>, "Masami Hiramatsu" <mhiramat@kernel.org>, "Mark Rutland" <mark.rutland@arm.com>, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] riscv: Fix text patching when IPI are used Date: Wed, 28 Feb 2024 12:21:11 -0600 [thread overview] Message-ID: <74d7adee-8293-4fad-82c3-f4458b2e3960@sifive.com> (raw) In-Reply-To: <20240228175149.162646-3-alexghiti@rivosinc.com> 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 <bjorn@rivosinc.com> > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > Signed-off-by: Alexandre Ghiti <alexghiti@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.holland@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);
WARNING: multiple messages have this Message-ID (diff)
From: Samuel Holland <samuel.holland@sifive.com> To: Alexandre Ghiti <alexghiti@rivosinc.com> Cc: "Björn Töpel" <bjorn@rivosinc.com>, "Paul Walmsley" <paul.walmsley@sifive.com>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Albert Ou" <aou@eecs.berkeley.edu>, "Andrea Parri" <andrea@rivosinc.com>, "Anup Patel" <anup@brainfault.org>, "Steven Rostedt" <rostedt@goodmis.org>, "Masami Hiramatsu" <mhiramat@kernel.org>, "Mark Rutland" <mark.rutland@arm.com>, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] riscv: Fix text patching when IPI are used Date: Wed, 28 Feb 2024 12:21:11 -0600 [thread overview] Message-ID: <74d7adee-8293-4fad-82c3-f4458b2e3960@sifive.com> (raw) In-Reply-To: <20240228175149.162646-3-alexghiti@rivosinc.com> 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 <bjorn@rivosinc.com> > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > Signed-off-by: Alexandre Ghiti <alexghiti@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.holland@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); _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2024-02-28 18:21 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-02-28 17:51 [PATCH 0/2] riscv: fix patching with IPI Alexandre Ghiti 2024-02-28 17:51 ` Alexandre Ghiti 2024-02-28 17:51 ` [PATCH 1/2] riscv: Remove superfluous smp_mb() Alexandre Ghiti 2024-02-28 17:51 ` Alexandre Ghiti 2024-02-29 0:17 ` Andrea Parri 2024-02-29 0:17 ` Andrea Parri 2024-02-28 17:51 ` [PATCH 2/2] riscv: Fix text patching when IPI are used Alexandre Ghiti 2024-02-28 17:51 ` Alexandre Ghiti 2024-02-28 18:21 ` Samuel Holland [this message] 2024-02-28 18:21 ` Samuel Holland 2024-02-28 19:02 ` Alexandre Ghiti 2024-02-28 19:02 ` Alexandre Ghiti 2024-02-29 0:42 ` Andrea Parri 2024-02-29 0:42 ` Andrea Parri
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=74d7adee-8293-4fad-82c3-f4458b2e3960@sifive.com \ --to=samuel.holland@sifive.com \ --cc=alexghiti@rivosinc.com \ --cc=andrea@rivosinc.com \ --cc=anup@brainfault.org \ --cc=aou@eecs.berkeley.edu \ --cc=bjorn@rivosinc.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=linux-trace-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mhiramat@kernel.org \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=rostedt@goodmis.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.