[PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Jiri Olsa]

Nathan reported compilation fail for loongarch arch:

  kernel/events/uprobes.c: In function 'arch_uprobe_trampoline':
  arch/loongarch/include/asm/uprobes.h:12:33: error: initializer element is not constant
     12 | #define UPROBE_SWBP_INSN        larch_insn_gen_break(BRK_UPROBE_BP)
        |                                 ^~~~~~~~~~~~~~~~~~~~
  kernel/events/uprobes.c:1479:39: note: in expansion of macro 'UPROBE_SWBP_INSN'
   1479 |         static uprobe_opcode_t insn = UPROBE_SWBP_INSN;

Loongarch defines UPROBE_SWBP_INSN as function call, so we can't
use it to initialize static variable.

Cc: Oleg Nesterov <oleg@redhat.com>
Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/uprobes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2816e65729ac..6986bd993702 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1476,8 +1476,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 
 void * __weak arch_uprobe_trampoline(unsigned long *psize)
 {
-	static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+	static uprobe_opcode_t insn;
 
+	insn = insn ?: UPROBE_SWBP_INSN;
 	*psize = UPROBE_SWBP_INSN_SIZE;
 	return &insn;
 }
-- 
2.45.1

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Andrii Nakryiko]

On Tue, Jun 18, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Nathan reported compilation fail for loongarch arch:
>
>   kernel/events/uprobes.c: In function 'arch_uprobe_trampoline':
>   arch/loongarch/include/asm/uprobes.h:12:33: error: initializer element is not constant
>      12 | #define UPROBE_SWBP_INSN        larch_insn_gen_break(BRK_UPROBE_BP)
>         |                                 ^~~~~~~~~~~~~~~~~~~~
>   kernel/events/uprobes.c:1479:39: note: in expansion of macro 'UPROBE_SWBP_INSN'
>    1479 |         static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
>
> Loongarch defines UPROBE_SWBP_INSN as function call, so we can't
> use it to initialize static variable.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/uprobes.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Can we instead ask loongarch folks to rewrite it to be a constant?
Having this as a function call is both an inconvenience and potential
performance problem (a minor one, but still). I would imagine it's not
hard to hard-code an instruction as a constant here.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 2816e65729ac..6986bd993702 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1476,8 +1476,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
>
>  void * __weak arch_uprobe_trampoline(unsigned long *psize)
>  {
> -       static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> +       static uprobe_opcode_t insn;
>
> +       insn = insn ?: UPROBE_SWBP_INSN;
>         *psize = UPROBE_SWBP_INSN_SIZE;
>         return &insn;
>  }
> --
> 2.45.1
>

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Oleg Nesterov]

On 06/20, Andrii Nakryiko wrote:
>
> Can we instead ask loongarch folks to rewrite it to be a constant?
> Having this as a function call is both an inconvenience and potential
> performance problem (a minor one, but still). I would imagine it's not
> hard to hard-code an instruction as a constant here.

I was going to ask the same question when I saw the bug report ;)
The same for other users of larch_insn_gen_break().

But I can't understand what does it do, it calls emit_break() and
git grep -w emit_break finds nothing.

Oleg.

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Andrii Nakryiko]

On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 06/20, Andrii Nakryiko wrote:
> >
> > Can we instead ask loongarch folks to rewrite it to be a constant?
> > Having this as a function call is both an inconvenience and potential
> > performance problem (a minor one, but still). I would imagine it's not
> > hard to hard-code an instruction as a constant here.
>
> I was going to ask the same question when I saw the bug report ;)
> The same for other users of larch_insn_gen_break().
>
> But I can't understand what does it do, it calls emit_break() and
> git grep -w emit_break finds nothing.
>

It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in
arch/loongarch/include/asm/inst.h

A bunch of macro magic, but in the end it produces some constant
value, of course.

> Oleg.
>

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Oleg Nesterov]

On 06/20, Andrii Nakryiko wrote:
>
> On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > But I can't understand what does it do, it calls emit_break() and
> > git grep -w emit_break finds nothing.
> >
>
> It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in
> arch/loongarch/include/asm/inst.h
>
> A bunch of macro magic, but in the end it produces some constant
> value, of course.

I see, thanks!

Then perhaps something like below?

Oleg.


--- x/arch/loongarch/include/asm/uprobes.h
+++ x/arch/loongarch/include/asm/uprobes.h
@@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t;
 #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	(uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15))
 #define UPROBE_SWBP_INSN_SIZE	LOONGARCH_INSN_SIZE
 
 #define UPROBE_XOLBP_INSN	larch_insn_gen_break(BRK_UPROBE_XOLBP)
--- x/arch/loongarch/kernel/uprobes.c
+++ x/arch/loongarch/kernel/uprobes.c
@@ -7,6 +7,13 @@
 
 #define UPROBE_TRAP_NR	UINT_MAX
 
+static __init int __ck_insn(void)
+{
+	BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP));
+	return 0;
+}
+late_initcall(__ck_insn);
+
 int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
 			     struct mm_struct *mm, unsigned long addr)
 {

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Jiri Olsa]

On Fri, Jun 21, 2024 at 02:01:50PM +0200, Oleg Nesterov wrote:
> On 06/20, Andrii Nakryiko wrote:
> >
> > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > But I can't understand what does it do, it calls emit_break() and
> > > git grep -w emit_break finds nothing.
> > >
> >
> > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in
> > arch/loongarch/include/asm/inst.h
> >
> > A bunch of macro magic, but in the end it produces some constant
> > value, of course.
> 
> I see, thanks!
> 
> Then perhaps something like below?

lgtm, added loong arch list/folks

for context:
  https://lore.kernel.org/bpf/20240614174822.GA1185149@thelio-3990X/

thanks,
jirka

> 
> Oleg.
> 
> 
> --- x/arch/loongarch/include/asm/uprobes.h
> +++ x/arch/loongarch/include/asm/uprobes.h
> @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t;
>  #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	(uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15))
>  #define UPROBE_SWBP_INSN_SIZE	LOONGARCH_INSN_SIZE
>  
>  #define UPROBE_XOLBP_INSN	larch_insn_gen_break(BRK_UPROBE_XOLBP)
> --- x/arch/loongarch/kernel/uprobes.c
> +++ x/arch/loongarch/kernel/uprobes.c
> @@ -7,6 +7,13 @@
>  
>  #define UPROBE_TRAP_NR	UINT_MAX
>  
> +static __init int __ck_insn(void)
> +{
> +	BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP));
> +	return 0;
> +}
> +late_initcall(__ck_insn);
> +
>  int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>  			     struct mm_struct *mm, unsigned long addr)
>  {
> 

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Jiri Olsa]

On Fri, Jun 21, 2024 at 03:17:58PM +0200, Jiri Olsa wrote:
> On Fri, Jun 21, 2024 at 02:01:50PM +0200, Oleg Nesterov wrote:
> > On 06/20, Andrii Nakryiko wrote:
> > >
> > > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
> > > >
> > > > But I can't understand what does it do, it calls emit_break() and
> > > > git grep -w emit_break finds nothing.
> > > >
> > >
> > > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in
> > > arch/loongarch/include/asm/inst.h
> > >
> > > A bunch of macro magic, but in the end it produces some constant
> > > value, of course.
> > 
> > I see, thanks!
> > 
> > Then perhaps something like below?
> 
> lgtm, added loong arch list/folks

ping

Oleg, do you want to send formal patch?

thanks,
jirka

> 
> for context:
>   https://lore.kernel.org/bpf/20240614174822.GA1185149@thelio-3990X/
> 
> thanks,
> jirka
> 
> > 
> > Oleg.
> > 
> > 
> > --- x/arch/loongarch/include/asm/uprobes.h
> > +++ x/arch/loongarch/include/asm/uprobes.h
> > @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t;
> >  #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	(uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15))
> >  #define UPROBE_SWBP_INSN_SIZE	LOONGARCH_INSN_SIZE
> >  
> >  #define UPROBE_XOLBP_INSN	larch_insn_gen_break(BRK_UPROBE_XOLBP)
> > --- x/arch/loongarch/kernel/uprobes.c
> > +++ x/arch/loongarch/kernel/uprobes.c
> > @@ -7,6 +7,13 @@
> >  
> >  #define UPROBE_TRAP_NR	UINT_MAX
> >  
> > +static __init int __ck_insn(void)
> > +{
> > +	BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP));
> > +	return 0;
> > +}
> > +late_initcall(__ck_insn);
> > +
> >  int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
> >  			     struct mm_struct *mm, unsigned long addr)
> >  {
> > 

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Masami Hiramatsu]

On Thu, 27 Jun 2024 15:44:16 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Fri, Jun 21, 2024 at 03:17:58PM +0200, Jiri Olsa wrote:
> > On Fri, Jun 21, 2024 at 02:01:50PM +0200, Oleg Nesterov wrote:
> > > On 06/20, Andrii Nakryiko wrote:
> > > >
> > > > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
> > > > >
> > > > > But I can't understand what does it do, it calls emit_break() and
> > > > > git grep -w emit_break finds nothing.
> > > > >
> > > >
> > > > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in
> > > > arch/loongarch/include/asm/inst.h
> > > >
> > > > A bunch of macro magic, but in the end it produces some constant
> > > > value, of course.
> > > 
> > > I see, thanks!
> > > 
> > > Then perhaps something like below?
> > 
> > lgtm, added loong arch list/folks
> 
> ping
> 
> Oleg, do you want to send formal patch?
> 
> thanks,
> jirka

Yes, can you send v2 patch?

Thank you,

> 
> > 
> > for context:
> >   https://lore.kernel.org/bpf/20240614174822.GA1185149@thelio-3990X/
> > 
> > thanks,
> > jirka
> > 
> > > 
> > > Oleg.
> > > 
> > > 
> > > --- x/arch/loongarch/include/asm/uprobes.h
> > > +++ x/arch/loongarch/include/asm/uprobes.h
> > > @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t;
> > >  #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	(uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15))
> > >  #define UPROBE_SWBP_INSN_SIZE	LOONGARCH_INSN_SIZE
> > >  
> > >  #define UPROBE_XOLBP_INSN	larch_insn_gen_break(BRK_UPROBE_XOLBP)
> > > --- x/arch/loongarch/kernel/uprobes.c
> > > +++ x/arch/loongarch/kernel/uprobes.c
> > > @@ -7,6 +7,13 @@
> > >  
> > >  #define UPROBE_TRAP_NR	UINT_MAX
> > >  
> > > +static __init int __ck_insn(void)
> > > +{
> > > +	BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP));
> > > +	return 0;
> > > +}
> > > +late_initcall(__ck_insn);
> > > +
> > >  int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
> > >  			     struct mm_struct *mm, unsigned long addr)
> > >  {
> > > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] uprobe: Do not use UPROBE_SWBP_INSN as static initializer

[Oleg Nesterov]

On 06/27, Masami Hiramatsu wrote:
>
> On Thu, 27 Jun 2024 15:44:16 +0200
> Jiri Olsa <olsajiri@gmail.com> wrote:
>
> > Oleg, do you want to send formal patch?
> >
> > thanks,
> > jirka
>
> Yes, can you send v2 patch?

I was waiting for the comments from loongarch maintainers...

OK, will do today, but the patch won't be even compile tested.

Oleg.

[PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Oleg Nesterov]

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(-)

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)
 {
-- 
2.25.1.362.g51ebf55

Re: [PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Andrii Nakryiko]

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
>
>

Re: [PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Oleg Nesterov]

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.

Re: [PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Masami Hiramatsu]

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.
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Tiezhu Yang]

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

Re: [PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Oleg Nesterov]

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
>

Re: [PATCH] LoongArch: uprobes: make UPROBE_SWBP_INSN/UPROBE_XOLBP_INSN constant

[Huacai Chen]

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
> >
>

[PATCH] LoongArch: make the users of larch_insn_gen_break() constant

[Oleg Nesterov]

LoongArch defines UPROBE_SWBP_INSN as a function call and this breaks
arch_uprobe_trampoline() which uses it to initialize a static variable.

Add the new "__builtin_constant_p" helper, __emit_break(), and redefine
the current users of larch_insn_gen_break() to use it.

The patch adds check_emit_break() into kprobes.c and uprobes.c to test
this change. They can be removed if LoongArch boots at least once, but
otoh these 2 __init functions will be discarded by free_initmem().

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/inst.h    |  3 +++
 arch/loongarch/include/asm/uprobes.h |  4 ++--
 arch/loongarch/kernel/kprobes.c      | 12 ++++++++++--
 arch/loongarch/kernel/uprobes.c      |  8 ++++++++
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
index c3993fd88aba..944482063f14 100644
--- a/arch/loongarch/include/asm/inst.h
+++ b/arch/loongarch/include/asm/inst.h
@@ -532,6 +532,9 @@ static inline void emit_##NAME(union loongarch_instruction *insn,	\
 
 DEF_EMIT_REG0I15_FORMAT(break, break_op)
 
+/* like emit_break(imm) but returns a constant expression */
+#define __emit_break(imm)	((u32)((imm) | (break_op << 15)))
+
 #define DEF_EMIT_REG0I26_FORMAT(NAME, OP)				\
 static inline void emit_##NAME(union loongarch_instruction *insn,	\
 			       int offset)				\
diff --git a/arch/loongarch/include/asm/uprobes.h b/arch/loongarch/include/asm/uprobes.h
index c8f59983f702..99a0d198927f 100644
--- a/arch/loongarch/include/asm/uprobes.h
+++ b/arch/loongarch/include/asm/uprobes.h
@@ -9,10 +9,10 @@ typedef u32 uprobe_opcode_t;
 #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/kprobes.c b/arch/loongarch/kernel/kprobes.c
index 17b040bd6067..78cfaac52748 100644
--- a/arch/loongarch/kernel/kprobes.c
+++ b/arch/loongarch/kernel/kprobes.c
@@ -4,8 +4,16 @@
 #include <linux/preempt.h>
 #include <asm/break.h>
 
-#define KPROBE_BP_INSN		larch_insn_gen_break(BRK_KPROBE_BP)
-#define KPROBE_SSTEPBP_INSN	larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
+#define KPROBE_BP_INSN		__emit_break(BRK_KPROBE_BP)
+#define KPROBE_SSTEPBP_INSN	__emit_break(BRK_KPROBE_SSTEPBP)
+
+static __init int check_emit_break(void)
+{
+	BUG_ON(KPROBE_BP_INSN      != larch_insn_gen_break(BRK_KPROBE_BP));
+	BUG_ON(KPROBE_SSTEPBP_INSN != larch_insn_gen_break(BRK_KPROBE_SSTEPBP));
+	return 0;
+}
+arch_initcall(check_emit_break);
 
 DEFINE_PER_CPU(struct kprobe *, current_kprobe);
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
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)
 {
-- 
2.25.1.362.g51ebf55

Re: [PATCH] LoongArch: make the users of larch_insn_gen_break() constant

[Huacai Chen]

Hi, Oleg,

On Sat, Jun 29, 2024 at 11:05 PM 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.
>
> Add the new "__builtin_constant_p" helper, __emit_break(), and redefine
> the current users of larch_insn_gen_break() to use it.
>
> The patch adds check_emit_break() into kprobes.c and uprobes.c to test
> this change. They can be removed if LoongArch boots at least once, but
> otoh these 2 __init functions will be discarded by free_initmem().
>
> 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/inst.h    |  3 +++
>  arch/loongarch/include/asm/uprobes.h |  4 ++--
>  arch/loongarch/kernel/kprobes.c      | 12 ++++++++++--
>  arch/loongarch/kernel/uprobes.c      |  8 ++++++++
>  4 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
> index c3993fd88aba..944482063f14 100644
> --- a/arch/loongarch/include/asm/inst.h
> +++ b/arch/loongarch/include/asm/inst.h
> @@ -532,6 +532,9 @@ static inline void emit_##NAME(union loongarch_instruction *insn,   \
>
>  DEF_EMIT_REG0I15_FORMAT(break, break_op)
>
> +/* like emit_break(imm) but returns a constant expression */
> +#define __emit_break(imm)      ((u32)((imm) | (break_op << 15)))
> +
>  #define DEF_EMIT_REG0I26_FORMAT(NAME, OP)                              \
>  static inline void emit_##NAME(union loongarch_instruction *insn,      \
>                                int offset)                              \
> diff --git a/arch/loongarch/include/asm/uprobes.h b/arch/loongarch/include/asm/uprobes.h
> index c8f59983f702..99a0d198927f 100644
> --- a/arch/loongarch/include/asm/uprobes.h
> +++ b/arch/loongarch/include/asm/uprobes.h
> @@ -9,10 +9,10 @@ typedef u32 uprobe_opcode_t;
>  #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/kprobes.c b/arch/loongarch/kernel/kprobes.c
> index 17b040bd6067..78cfaac52748 100644
> --- a/arch/loongarch/kernel/kprobes.c
> +++ b/arch/loongarch/kernel/kprobes.c
> @@ -4,8 +4,16 @@
>  #include <linux/preempt.h>
>  #include <asm/break.h>
>
> -#define KPROBE_BP_INSN         larch_insn_gen_break(BRK_KPROBE_BP)
> -#define KPROBE_SSTEPBP_INSN    larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
> +#define KPROBE_BP_INSN         __emit_break(BRK_KPROBE_BP)
> +#define KPROBE_SSTEPBP_INSN    __emit_break(BRK_KPROBE_SSTEPBP)
> +
> +static __init int check_emit_break(void)
> +{
> +       BUG_ON(KPROBE_BP_INSN      != larch_insn_gen_break(BRK_KPROBE_BP));
> +       BUG_ON(KPROBE_SSTEPBP_INSN != larch_insn_gen_break(BRK_KPROBE_SSTEPBP));
> +       return 0;
> +}
> +arch_initcall(check_emit_break);
>
>  DEFINE_PER_CPU(struct kprobe *, current_kprobe);
>  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> 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);
Do you mind if I remove the runtime checking after Tiezhu tests the correctness?

Huacai

> +
>  int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>                              struct mm_struct *mm, unsigned long addr)
>  {
> --
> 2.25.1.362.g51ebf55
>
>

Re: [PATCH] LoongArch: make the users of larch_insn_gen_break() constant

[Oleg Nesterov]

On 06/30, Huacai Chen wrote:
>
> > +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);
> Do you mind if I remove the runtime checking after Tiezhu tests the correctness?

Sure, please remove, thanks.

Oleg.

Re: [PATCH] LoongArch: make the users of larch_insn_gen_break() constant

[Tiezhu Yang]

On 06/29/2024 11:03 PM, Oleg Nesterov wrote:
> LoongArch defines UPROBE_SWBP_INSN as a function call and this breaks
> arch_uprobe_trampoline() which uses it to initialize a static variable.
>
> Add the new "__builtin_constant_p" helper, __emit_break(), and redefine
> the current users of larch_insn_gen_break() to use it.
>
> The patch adds check_emit_break() into kprobes.c and uprobes.c to test
> this change. They can be removed if LoongArch boots at least once, but
> otoh these 2 __init functions will be discarded by free_initmem().
>
> 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>

Tested on LoongArch machine with Loongson-3A5000 and Loongson-3A6000
CPU, based on 6.10-rc3,

KPROBE_BP_INSN == larch_insn_gen_break(BRK_KPROBE_BP)
KPROBE_SSTEPBP_INSN == larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
UPROBE_SWBP_INSN  == larch_insn_gen_break(BRK_UPROBE_BP)
UPROBE_XOLBP_INSN == larch_insn_gen_break(BRK_UPROBE_XOLBP)

The two functions check_emit_break() can be removed in
arch/loongarch/kernel/kprobes.c and arch/loongarch/kernel/uprobes.c

Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn>

Thanks,
Tiezhu

Re: [PATCH] LoongArch: make the users of larch_insn_gen_break() constant

[Huacai Chen]

On Mon, Jul 1, 2024 at 2:22 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> On 06/29/2024 11:03 PM, Oleg Nesterov wrote:
> > LoongArch defines UPROBE_SWBP_INSN as a function call and this breaks
> > arch_uprobe_trampoline() which uses it to initialize a static variable.
> >
> > Add the new "__builtin_constant_p" helper, __emit_break(), and redefine
> > the current users of larch_insn_gen_break() to use it.
> >
> > The patch adds check_emit_break() into kprobes.c and uprobes.c to test
> > this change. They can be removed if LoongArch boots at least once, but
> > otoh these 2 __init functions will be discarded by free_initmem().
> >
> > 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>
>
> Tested on LoongArch machine with Loongson-3A5000 and Loongson-3A6000
> CPU, based on 6.10-rc3,
>
> KPROBE_BP_INSN == larch_insn_gen_break(BRK_KPROBE_BP)
> KPROBE_SSTEPBP_INSN == larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
> UPROBE_SWBP_INSN  == larch_insn_gen_break(BRK_UPROBE_BP)
> UPROBE_XOLBP_INSN == larch_insn_gen_break(BRK_UPROBE_XOLBP)
>
> The two functions check_emit_break() can be removed in
> arch/loongarch/kernel/kprobes.c and arch/loongarch/kernel/uprobes.c
>
> Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn>
Queued, thanks.

Huacai
>
> Thanks,
> Tiezhu
>
>