| Message ID | 20240730135921.156508-1-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | e298a843bcfe2af6a5415002d7bce0b66d4f44b2 |
| Headers | show |
| Series | [-fixes] riscv: Re-introduce global icache flush in patch_text_XXX() | expand |
Hello: This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <palmer@rivosinc.com>: On Tue, 30 Jul 2024 15:59:21 +0200 you wrote: > commit edf2d546bfd6 ("riscv: patch: Flush the icache right after > patching to avoid illegal insns") mistakenly removed the global icache > flush in patch_text_nosync() and patch_text_set_nosync() functions, so > reintroduce them. > > Fixes: edf2d546bfd6 ("riscv: patch: Flush the icache right after patching to avoid illegal insns") > Reported-by: Samuel Holland <samuel.holland@sifive.com> > Closes: https://lore.kernel.org/linux-riscv/CAHVXubh8Adb4=-vN4cSh0FrZ16TeOKJbLj4AF09QC241bRk1Jg@mail.gmail.com/T/#m800757c26f72a1d45c240cb815650430166c82ea > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > [...] Here is the summary with links: - [-fixes] riscv: Re-introduce global icache flush in patch_text_XXX() https://git.kernel.org/riscv/c/e298a843bcfe You are awesome, thank you!
Hi Alex, On 2024-07-30 8:59 AM, Alexandre Ghiti wrote: > commit edf2d546bfd6 ("riscv: patch: Flush the icache right after > patching to avoid illegal insns") mistakenly removed the global icache > flush in patch_text_nosync() and patch_text_set_nosync() functions, so > reintroduce them. > > Fixes: edf2d546bfd6 ("riscv: patch: Flush the icache right after patching to avoid illegal insns") > Reported-by: Samuel Holland <samuel.holland@sifive.com> > Closes: https://lore.kernel.org/linux-riscv/CAHVXubh8Adb4=-vN4cSh0FrZ16TeOKJbLj4AF09QC241bRk1Jg@mail.gmail.com/T/#m800757c26f72a1d45c240cb815650430166c82ea Shouldn't this use the permalink for the specific message, not the thread? > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > arch/riscv/kernel/patch.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > index ab03732d06c4..91edfd764ed9 100644 > --- a/arch/riscv/kernel/patch.c > +++ b/arch/riscv/kernel/patch.c > @@ -205,6 +205,9 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len) > > ret = patch_insn_set(tp, c, len); > > + if (!ret) > + flush_icache_range((uintptr_t)tp, (uintptr_t)tp + len); This patch was based on an old tree from before https://git.kernel.org/riscv/c/47742484ee16 removed the "tp" variable. While it still compiles because flush_icache_range() is a macro that discards its arguments, it will be confusing to anyone reading the code. Regards, Samuel > + > return ret; > } > NOKPROBE_SYMBOL(patch_text_set_nosync); > @@ -237,6 +240,9 @@ int patch_text_nosync(void *addr, const void *insns, size_t len) > > ret = patch_insn_write(tp, insns, len); > > + if (!ret) > + flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len); > + > return ret; > } > NOKPROBE_SYMBOL(patch_text_nosync);
On Thu, 01 Aug 2024 10:32:28 PDT (-0700), samuel.holland@sifive.com wrote: > Hi Alex, > > On 2024-07-30 8:59 AM, Alexandre Ghiti wrote: >> commit edf2d546bfd6 ("riscv: patch: Flush the icache right after >> patching to avoid illegal insns") mistakenly removed the global icache >> flush in patch_text_nosync() and patch_text_set_nosync() functions, so >> reintroduce them. >> >> Fixes: edf2d546bfd6 ("riscv: patch: Flush the icache right after patching to avoid illegal insns") >> Reported-by: Samuel Holland <samuel.holland@sifive.com> >> Closes: https://lore.kernel.org/linux-riscv/CAHVXubh8Adb4=-vN4cSh0FrZ16TeOKJbLj4AF09QC241bRk1Jg@mail.gmail.com/T/#m800757c26f72a1d45c240cb815650430166c82ea > > Shouldn't this use the permalink for the specific message, not the thread? > >> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> >> --- >> arch/riscv/kernel/patch.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c >> index ab03732d06c4..91edfd764ed9 100644 >> --- a/arch/riscv/kernel/patch.c >> +++ b/arch/riscv/kernel/patch.c >> @@ -205,6 +205,9 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len) >> >> ret = patch_insn_set(tp, c, len); >> >> + if (!ret) >> + flush_icache_range((uintptr_t)tp, (uintptr_t)tp + len); > > This patch was based on an old tree from before > https://git.kernel.org/riscv/c/47742484ee16 removed the "tp" variable. While it > still compiles because flush_icache_range() is a macro that discards its > arguments, it will be confusing to anyone reading the code. Thanks. Alex is going to spin a new one, so I dropped this. > > Regards, > Samuel > >> + >> return ret; >> } >> NOKPROBE_SYMBOL(patch_text_set_nosync); >> @@ -237,6 +240,9 @@ int patch_text_nosync(void *addr, const void *insns, size_t len) >> >> ret = patch_insn_write(tp, insns, len); >> >> + if (!ret) >> + flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len); >> + >> return ret; >> } >> NOKPROBE_SYMBOL(patch_text_nosync);
Hi Samuel, On 01/08/2024 19:32, Samuel Holland wrote: > Hi Alex, > > On 2024-07-30 8:59 AM, Alexandre Ghiti wrote: >> commit edf2d546bfd6 ("riscv: patch: Flush the icache right after >> patching to avoid illegal insns") mistakenly removed the global icache >> flush in patch_text_nosync() and patch_text_set_nosync() functions, so >> reintroduce them. >> >> Fixes: edf2d546bfd6 ("riscv: patch: Flush the icache right after patching to avoid illegal insns") >> Reported-by: Samuel Holland <samuel.holland@sifive.com> >> Closes: https://lore.kernel.org/linux-riscv/CAHVXubh8Adb4=-vN4cSh0FrZ16TeOKJbLj4AF09QC241bRk1Jg@mail.gmail.com/T/#m800757c26f72a1d45c240cb815650430166c82ea > Shouldn't this use the permalink for the specific message, not the thread? I never paid attention to that, but the permalink is shorter so I'll change that, thanks > >> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> >> --- >> arch/riscv/kernel/patch.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c >> index ab03732d06c4..91edfd764ed9 100644 >> --- a/arch/riscv/kernel/patch.c >> +++ b/arch/riscv/kernel/patch.c >> @@ -205,6 +205,9 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len) >> >> ret = patch_insn_set(tp, c, len); >> >> + if (!ret) >> + flush_icache_range((uintptr_t)tp, (uintptr_t)tp + len); > This patch was based on an old tree from before > https://git.kernel.org/riscv/c/47742484ee16 removed the "tp" variable. While it > still compiles because flush_icache_range() is a macro that discards its > arguments, it will be confusing to anyone reading the code. Hmmm not the first time I hear this :) Sorry, I prepared the patch a couple of weeks ago and the rebase on 6.11-rc1 did not complain, I do that right now and send a new version tomorrow. Thanks, Alex > > Regards, > Samuel > >> + >> return ret; >> } >> NOKPROBE_SYMBOL(patch_text_set_nosync); >> @@ -237,6 +240,9 @@ int patch_text_nosync(void *addr, const void *insns, size_t len) >> >> ret = patch_insn_write(tp, insns, len); >> >> + if (!ret) >> + flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len); >> + >> return ret; >> } >> NOKPROBE_SYMBOL(patch_text_nosync); > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c index ab03732d06c4..91edfd764ed9 100644 --- a/arch/riscv/kernel/patch.c +++ b/arch/riscv/kernel/patch.c @@ -205,6 +205,9 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len) ret = patch_insn_set(tp, c, len); + if (!ret) + flush_icache_range((uintptr_t)tp, (uintptr_t)tp + len); + return ret; } NOKPROBE_SYMBOL(patch_text_set_nosync); @@ -237,6 +240,9 @@ int patch_text_nosync(void *addr, const void *insns, size_t len) ret = patch_insn_write(tp, insns, len); + if (!ret) + flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len); + return ret; } NOKPROBE_SYMBOL(patch_text_nosync);
commit edf2d546bfd6 ("riscv: patch: Flush the icache right after patching to avoid illegal insns") mistakenly removed the global icache flush in patch_text_nosync() and patch_text_set_nosync() functions, so reintroduce them. Fixes: edf2d546bfd6 ("riscv: patch: Flush the icache right after patching to avoid illegal insns") Reported-by: Samuel Holland <samuel.holland@sifive.com> Closes: https://lore.kernel.org/linux-riscv/CAHVXubh8Adb4=-vN4cSh0FrZ16TeOKJbLj4AF09QC241bRk1Jg@mail.gmail.com/T/#m800757c26f72a1d45c240cb815650430166c82ea Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- arch/riscv/kernel/patch.c | 6 ++++++ 1 file changed, 6 insertions(+)