[RFC] riscv: restore the irq save/restore logic for nosync code patching

[Wu Zhangjin]

Hi, Ren

Thanks very much for your review.

>
>
> Sorry, I misunderstood your patch, here is the new review feedback.
>
> On Sat, Jun 11, 2022 at 10:31 AM Wu Zhangjin <falcon at tinylab.org> wrote:
> >
> > The commit '0ff7c3b331276f584bde3ae9a16bacd8fa3d01e6' removed the
> > old patch_lock together with the irq save/restore logic.
> >
> > But the patching interface: patch_text_nosync() has new users like
> > Jump Label, which doesn't use stop_machine(), here restores the irq
> > save/restore logic for such users, just as the other architectures do.
> Let's give protection out of patch_text_nosync.
>
>         mutex_lock(&text_mutex);
>         patch_text_nosync(addr, &insn, sizeof(insn));
>         mutex_unlock(&text_mutex);
>
> Why mutex isn't enough for patch_text?

The interrupt may come during code modification or between code modification
and icache flush.

Just recheck the jump label code in arch/riscv/kernel/jump_labe.c, both of the
nop and jal instructions are 4 bytes, so, the interrupt may not be possible to
come just in the code modification procedure and therefore impossible to
execute only part of the instructions, but may be possible to come in just
after code modification and before icache flush. is this a risk? If not, this
irq save/restore part can be simply ignored.

>
> > Move lockdep assert to the required path too, the current stop_machine()
> > based ftrace implementation should use the new added patch_text_noirq()
> > without this lockdep assert, it has its own mutex, but not named as
> > text_mutex.
> Do you mean we should use irq_save&restore instead of text_mutex?
>

Sorry (and to zong li), since 'grep text_mutex kernel/trace/ftrace.c' returns
nothing and my kernel image built with lockdep configured failed to boot on
qemu and I just missed that the old added global functions:
ftrace_arch_code_modify_prepare() and ftrace_arch_code_modify_post_process()
have added text_mutex and they have overrided the weak ones defined in
kernel/trace/ftrace.c, so, this code update path (even with the new
implementation without stop_machine) have been protected with text_mutex too,
lockdep assert here is therefore perfectly ok, we can not move it away.

BR,
Falcon

>
> >
> > As the latest maillist shows, a new ftrace support without
> > stop_machine() is in review, which requires the nosync version with this
> > irq save/restore logic or the new added noirq version (as we can see it
> > also calls patch_insn_write) with explicit text_mutex and irq
> > save/restore logic.
> >
> > Signed-off-by: Wu Zhangjin <falcon at tinylab.org>
> > ---
> >  arch/riscv/kernel/patch.c | 32 +++++++++++++++++++++++---------
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index 765004b60513..03f03832e077 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
> >         bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
> >         int ret;
> >
> > -       /*
> > -        * Before reaching here, it was expected to lock the text_mutex
> > -        * already, so we don't need to give another lock here and could
> > -        * ensure that it was safe between each cores.
> > -        */
> > -       lockdep_assert_held(&text_mutex);
> > -
> >         if (across_pages)
> >                 patch_map(addr + len, FIX_TEXT_POKE1);
> >
> > @@ -85,7 +78,8 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
> >  NOKPROBE_SYMBOL(patch_insn_write);
> >  #endif /* CONFIG_MMU */
> >
> > -int patch_text_nosync(void *addr, const void *insns, size_t len)
> > +/* For stop_machine() or the other cases which have already disabled irq and have locked among cpu cores */
> > +int patch_text_noirq(void *addr, const void *insns, size_t len)
> >  {
> >         u32 *tp = addr;
> >         int ret;
> > @@ -97,6 +91,26 @@ int patch_text_nosync(void *addr, const void *insns, size_t len)
> >
> >         return ret;
> >  }
> > +NOKPROBE_SYMBOL(patch_text_noirq);
> > +
> > +int patch_text_nosync(void *addr, const void *insns, size_t len)
> > +{
> > +       int ret;
> > +       unsigned long flags;
> > +
> > +       /*
> > +        * Before reaching here, it was expected to lock the text_mutex
> > +        * already, so we don't need to give another lock here and could
> > +        * ensure that it was safe between each cores.
> > +        */
> > +       lockdep_assert_held(&text_mutex);
> > +
> > +       local_irq_save(flags);
> > +       ret = patch_text_noirq(addr, insns, len);
> > +       local_irq_restore(flags);
> > +
> > +       return ret;
> > +}
> >  NOKPROBE_SYMBOL(patch_text_nosync);
> >
> >  static int patch_text_cb(void *data)
> > @@ -106,7 +120,7 @@ static int patch_text_cb(void *data)
> >
> >         if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) {
> >                 ret =
> > -                   patch_text_nosync(patch->addr, &patch->insn,
> > +                   patch_text_noirq(patch->addr, &patch->insn,
> >                                             GET_INSN_LENGTH(patch->insn));
> >                 atomic_inc(&patch->cpu_count);
> >         } else {
> > --
> > 2.35.1
> >
>
>
> --
> Best Regards
>  Guo Ren