| Message ID | 20240627160255.GA25374@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant | expand |
On Thu, Jun 27, 2024 at 9:04 AM Oleg Nesterov <oleg@redhat.com> wrote: > > LoongArch defines UPROBE_SWBP_INSN as a function call and this breaks > arch_uprobe_trampoline() which uses it to initialize a static variable. > > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe") > Reported-by: Nathan Chancellor <nathan@kernel.org> > Closes: https://lore.kernel.org/all/20240614174822.GA1185149@thelio-3990X/ > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > arch/loongarch/include/asm/uprobes.h | 6 ++++-- > arch/loongarch/kernel/uprobes.c | 8 ++++++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > LGTM. Acked-by: Andrii Nakryiko <andrii@kernel.org> > diff --git a/arch/loongarch/include/asm/uprobes.h b/arch/loongarch/include/asm/uprobes.h > index c8f59983f702..18221eb9a8b0 100644 > --- a/arch/loongarch/include/asm/uprobes.h > +++ b/arch/loongarch/include/asm/uprobes.h > @@ -6,13 +6,15 @@ > > typedef u32 uprobe_opcode_t; > > +#define __emit_break(imm) (uprobe_opcode_t)((imm) | (break_op << 15)) > + > #define MAX_UINSN_BYTES 8 > #define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES > > -#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) > +#define UPROBE_SWBP_INSN __emit_break(BRK_UPROBE_BP) > #define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE > > -#define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP) > +#define UPROBE_XOLBP_INSN __emit_break(BRK_UPROBE_XOLBP) > this looks correct (but based on pure code inspection) > struct arch_uprobe { > unsigned long resume_era; > diff --git a/arch/loongarch/kernel/uprobes.c b/arch/loongarch/kernel/uprobes.c > index 87abc7137b73..90462d94c28f 100644 > --- a/arch/loongarch/kernel/uprobes.c > +++ b/arch/loongarch/kernel/uprobes.c > @@ -7,6 +7,14 @@ > > #define UPROBE_TRAP_NR UINT_MAX > > +static __init int check_emit_break(void) > +{ > + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); > + BUG_ON(UPROBE_XOLBP_INSN != larch_insn_gen_break(BRK_UPROBE_XOLBP)); > + return 0; > +} > +arch_initcall(check_emit_break); > + I wouldn't even bother with this, but whatever. > int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > struct mm_struct *mm, unsigned long addr) > { > -- > 2.25.1.362.g51ebf55 > >
On 06/27, Andrii Nakryiko wrote: > > Acked-by: Andrii Nakryiko <andrii@kernel.org> Thanks! > > --- a/arch/loongarch/kernel/uprobes.c > > +++ b/arch/loongarch/kernel/uprobes.c > > @@ -7,6 +7,14 @@ > > > > #define UPROBE_TRAP_NR UINT_MAX > > > > +static __init int check_emit_break(void) > > +{ > > + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); > > + BUG_ON(UPROBE_XOLBP_INSN != larch_insn_gen_break(BRK_UPROBE_XOLBP)); > > + return 0; > > +} > > +arch_initcall(check_emit_break); > > + > > I wouldn't even bother with this, but whatever. Agreed, this looks a bit ugly. I did this only because I can not test this (hopefully trivial) patch and the maintainers didn't reply. If LoongArch boots at least once with this change, this run-time check can be removed. And just in case... I didn't dare to make a more "generic" change, but perhaps KPROBE_BP_INSN and KPROBE_SSTEPBP_INSN should be redefined the same way for micro-optimization. In this case __emit_break() should be probably moved into arch/loongarch/include/asm/inst.h. Oleg.
On Thu, 27 Jun 2024 19:38:06 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > On 06/27, Andrii Nakryiko wrote: > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Thanks! > > > > --- a/arch/loongarch/kernel/uprobes.c > > > +++ b/arch/loongarch/kernel/uprobes.c > > > @@ -7,6 +7,14 @@ > > > > > > #define UPROBE_TRAP_NR UINT_MAX > > > > > > +static __init int check_emit_break(void) > > > +{ > > > + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); > > > + BUG_ON(UPROBE_XOLBP_INSN != larch_insn_gen_break(BRK_UPROBE_XOLBP)); > > > + return 0; > > > +} > > > +arch_initcall(check_emit_break); > > > + > > > > I wouldn't even bother with this, but whatever. > > Agreed, this looks a bit ugly. I did this only because I can not test > this (hopefully trivial) patch and the maintainers didn't reply. > > If LoongArch boots at least once with this change, this run-time check > can be removed. > > And just in case... I didn't dare to make a more "generic" change, but > perhaps KPROBE_BP_INSN and KPROBE_SSTEPBP_INSN should be redefined the > same way for micro-optimization. In this case __emit_break() should be > probably moved into arch/loongarch/include/asm/inst.h. That idea sounds good to me too. If it is good to loongarch maintainers, (e.g. breakpoint instruction is stable), it is better to define in asm/insn.h. Thank you, > > Oleg. > >
On Thu, 27 Jun 2024 19:38:06 +0200 Oleg Nesterov <oleg@redhat.com> wrote: ... > > > +arch_initcall(check_emit_break); > > > + > > > > I wouldn't even bother with this, but whatever. > > Agreed, this looks a bit ugly. I did this only because I can not test > this (hopefully trivial) patch and the maintainers didn't reply. The LoongArch maintainer Huacai told me offline to reply this thread today. > If LoongArch boots at least once with this change, this run-time check > can be removed. I will test it next Monday. > And just in case... I didn't dare to make a more "generic" change, but > perhaps KPROBE_BP_INSN and KPROBE_SSTEPBP_INSN should be redefined the > same way for micro-optimization. In this case __emit_break() should be > probably moved into arch/loongarch/include/asm/inst.h. Yeah. I think so too. Thanks, Tiezhu
On 06/29, Tiezhu Yang wrote: > > On Thu, 27 Jun 2024 19:38:06 +0200 > Oleg Nesterov <oleg@redhat.com> wrote: > > ... > > > > > +arch_initcall(check_emit_break); > > > > + > > > > > > I wouldn't even bother with this, but whatever. > > > > Agreed, this looks a bit ugly. I did this only because I can not test > > this (hopefully trivial) patch and the maintainers didn't reply. > > The LoongArch maintainer Huacai told me offline to reply this thread today. > > > If LoongArch boots at least once with this change, this run-time check > > can be removed. > > I will test it next Monday. Thanks! > > And just in case... I didn't dare to make a more "generic" change, but > > perhaps KPROBE_BP_INSN and KPROBE_SSTEPBP_INSN should be redefined the > > same way for micro-optimization. In this case __emit_break() should be > > probably moved into arch/loongarch/include/asm/inst.h. > > Yeah. I think so too. OK... should I send v2? Or another change which does this on top of this patch? Or will you do it yourself? > > Thanks, > Tiezhu >
On Sat, Jun 29, 2024 at 9:40 PM Oleg Nesterov <oleg@redhat.com> wrote: > > On 06/29, Tiezhu Yang wrote: > > > > On Thu, 27 Jun 2024 19:38:06 +0200 > > Oleg Nesterov <oleg@redhat.com> wrote: > > > > ... > > > > > > > +arch_initcall(check_emit_break); > > > > > + > > > > > > > > I wouldn't even bother with this, but whatever. > > > > > > Agreed, this looks a bit ugly. I did this only because I can not test > > > this (hopefully trivial) patch and the maintainers didn't reply. > > > > The LoongArch maintainer Huacai told me offline to reply this thread today. > > > > > If LoongArch boots at least once with this change, this run-time check > > > can be removed. > > > > I will test it next Monday. > > Thanks! > > > > And just in case... I didn't dare to make a more "generic" change, but > > > perhaps KPROBE_BP_INSN and KPROBE_SSTEPBP_INSN should be redefined the > > > same way for micro-optimization. In this case __emit_break() should be > > > probably moved into arch/loongarch/include/asm/inst.h. > > > > Yeah. I think so too. > > OK... should I send v2? Or another change which does this on top of this > patch? Or will you do it yourself? I prefer V2. Huacai > > > > > Thanks, > > Tiezhu > > >
diff --git a/arch/loongarch/include/asm/uprobes.h b/arch/loongarch/include/asm/uprobes.h index c8f59983f702..18221eb9a8b0 100644 --- a/arch/loongarch/include/asm/uprobes.h +++ b/arch/loongarch/include/asm/uprobes.h @@ -6,13 +6,15 @@ typedef u32 uprobe_opcode_t; +#define __emit_break(imm) (uprobe_opcode_t)((imm) | (break_op << 15)) + #define MAX_UINSN_BYTES 8 #define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES -#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) +#define UPROBE_SWBP_INSN __emit_break(BRK_UPROBE_BP) #define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE -#define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP) +#define UPROBE_XOLBP_INSN __emit_break(BRK_UPROBE_XOLBP) struct arch_uprobe { unsigned long resume_era; diff --git a/arch/loongarch/kernel/uprobes.c b/arch/loongarch/kernel/uprobes.c index 87abc7137b73..90462d94c28f 100644 --- a/arch/loongarch/kernel/uprobes.c +++ b/arch/loongarch/kernel/uprobes.c @@ -7,6 +7,14 @@ #define UPROBE_TRAP_NR UINT_MAX +static __init int check_emit_break(void) +{ + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); + BUG_ON(UPROBE_XOLBP_INSN != larch_insn_gen_break(BRK_UPROBE_XOLBP)); + return 0; +} +arch_initcall(check_emit_break); + int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr) {
LoongArch defines UPROBE_SWBP_INSN as a function call and this breaks arch_uprobe_trampoline() which uses it to initialize a static variable. Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe") Reported-by: Nathan Chancellor <nathan@kernel.org> Closes: https://lore.kernel.org/all/20240614174822.GA1185149@thelio-3990X/ Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- arch/loongarch/include/asm/uprobes.h | 6 ++++-- arch/loongarch/kernel/uprobes.c | 8 ++++++++ 2 files changed, 12 insertions(+), 2 deletions(-)