arch/loongarch/include/asm/uprobes.h | 6 ++++-- arch/loongarch/kernel/uprobes.c | 8 ++++++++ 2 files changed, 12 insertions(+), 2 deletions(-)
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.g51ebf55On 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: ... > > > +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 > > >
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.g51ebf55On 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
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
>
>
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
>
>
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.
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>
© 2016 - 2025 Red Hat, Inc.