RFC patches sent for review, more testing is working in progress.
Tiezhu Yang (3):
LoongArch: Move three functions from kprobes.c to inst.h
LoongArch: Add larch_insn_gen_break() to generate break insn
LoongArch: Add uprobes support
arch/loongarch/Kconfig | 3 +
arch/loongarch/include/asm/inst.h | 63 ++++++++++++++++
arch/loongarch/include/asm/kprobes.h | 2 +-
arch/loongarch/include/asm/uprobes.h | 36 +++++++++
arch/loongarch/kernel/Makefile | 1 +
arch/loongarch/kernel/inst.c | 9 +++
arch/loongarch/kernel/kprobes.c | 63 ++--------------
arch/loongarch/kernel/traps.c | 7 +-
arch/loongarch/kernel/uprobes.c | 138 +++++++++++++++++++++++++++++++++++
9 files changed, 259 insertions(+), 63 deletions(-)
create mode 100644 arch/loongarch/include/asm/uprobes.h
create mode 100644 arch/loongarch/kernel/uprobes.c
--
2.1.0
There exist various break insns such as BRK_KPROBE_BP, BRK_KPROBE_SSTEPBP,
BRK_UPROBE_BP and BRK_UPROBE_XOLBP, add larch_insn_gen_break() to generate
break insns simpler and easier.
This is preparation for later patch, no functionality change.
Signed-off-by: Tiezhu Yang <[email protected]>
---
arch/loongarch/include/asm/inst.h | 30 +++++++++++++++++++++++++++---
arch/loongarch/include/asm/kprobes.h | 2 +-
arch/loongarch/kernel/inst.c | 9 +++++++++
arch/loongarch/kernel/kprobes.c | 17 +++--------------
4 files changed, 40 insertions(+), 18 deletions(-)
diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
index af494b5..a0fce06 100644
--- a/arch/loongarch/include/asm/inst.h
+++ b/arch/loongarch/include/asm/inst.h
@@ -409,8 +409,12 @@ static inline bool is_self_loop_ins(union loongarch_instruction *ip, struct pt_r
void simu_pc(struct pt_regs *regs, union loongarch_instruction insn);
void simu_branch(struct pt_regs *regs, union loongarch_instruction insn);
-static inline bool insns_not_supported(union loongarch_instruction insn)
+static inline bool insns_not_supported(u32 code)
{
+ union loongarch_instruction insn;
+
+ insn.word = code;
+
switch (insn.reg2i14_format.opcode) {
case llw_op:
case lld_op:
@@ -429,8 +433,12 @@ static inline bool insns_not_supported(union loongarch_instruction insn)
return false;
}
-static inline bool insns_need_simulation(union loongarch_instruction insn)
+static inline bool insns_need_simulation(u32 code)
{
+ union loongarch_instruction insn;
+
+ insn.word = code;
+
if (is_pc_ins(&insn))
return true;
@@ -440,8 +448,12 @@ static inline bool insns_need_simulation(union loongarch_instruction insn)
return false;
}
-static inline void arch_simulate_insn(union loongarch_instruction insn, struct pt_regs *regs)
+static inline void arch_simulate_insn(u32 code, struct pt_regs *regs)
{
+ union loongarch_instruction insn;
+
+ insn.word = code;
+
if (is_pc_ins(&insn))
simu_pc(regs, insn);
else if (is_branch_ins(&insn))
@@ -456,6 +468,8 @@ u32 larch_insn_gen_nop(void);
u32 larch_insn_gen_b(unsigned long pc, unsigned long dest);
u32 larch_insn_gen_bl(unsigned long pc, unsigned long dest);
+u32 larch_insn_gen_break(int imm);
+
u32 larch_insn_gen_or(enum loongarch_gpr rd, enum loongarch_gpr rj, enum loongarch_gpr rk);
u32 larch_insn_gen_move(enum loongarch_gpr rd, enum loongarch_gpr rj);
@@ -474,6 +488,16 @@ static inline bool unsigned_imm_check(unsigned long val, unsigned int bit)
return val < (1UL << bit);
}
+#define DEF_EMIT_REG0I15_FORMAT(NAME, OP) \
+static inline void emit_##NAME(union loongarch_instruction *insn, \
+ int imm) \
+{ \
+ insn->reg0i15_format.opcode = OP; \
+ insn->reg0i15_format.immediate = imm; \
+}
+
+DEF_EMIT_REG0I15_FORMAT(break, break_op)
+
#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/kprobes.h b/arch/loongarch/include/asm/kprobes.h
index 798020a..7ef7a0f 100644
--- a/arch/loongarch/include/asm/kprobes.h
+++ b/arch/loongarch/include/asm/kprobes.h
@@ -22,7 +22,7 @@ do { \
#define kretprobe_blacklist_size 0
-typedef union loongarch_instruction kprobe_opcode_t;
+typedef u32 kprobe_opcode_t;
/* Architecture specific copy of original instruction */
struct arch_specific_insn {
diff --git a/arch/loongarch/kernel/inst.c b/arch/loongarch/kernel/inst.c
index 258ef26..31b8efe 100644
--- a/arch/loongarch/kernel/inst.c
+++ b/arch/loongarch/kernel/inst.c
@@ -208,6 +208,15 @@ u32 larch_insn_gen_bl(unsigned long pc, unsigned long dest)
return insn.word;
}
+u32 larch_insn_gen_break(int imm)
+{
+ union loongarch_instruction insn;
+
+ emit_break(&insn, imm);
+
+ return insn.word;
+}
+
u32 larch_insn_gen_or(enum loongarch_gpr rd, enum loongarch_gpr rj, enum loongarch_gpr rk)
{
union loongarch_instruction insn;
diff --git a/arch/loongarch/kernel/kprobes.c b/arch/loongarch/kernel/kprobes.c
index 08c78d2..a5c3712 100644
--- a/arch/loongarch/kernel/kprobes.c
+++ b/arch/loongarch/kernel/kprobes.c
@@ -4,19 +4,8 @@
#include <linux/preempt.h>
#include <asm/break.h>
-static const union loongarch_instruction breakpoint_insn = {
- .reg0i15_format = {
- .opcode = break_op,
- .immediate = BRK_KPROBE_BP,
- }
-};
-
-static const union loongarch_instruction singlestep_insn = {
- .reg0i15_format = {
- .opcode = break_op,
- .immediate = BRK_KPROBE_SSTEPBP,
- }
-};
+#define breakpoint_insn larch_insn_gen_break(BRK_KPROBE_BP)
+#define singlestep_insn larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
DEFINE_PER_CPU(struct kprobe *, current_kprobe);
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -253,7 +242,7 @@ bool kprobe_breakpoint_handler(struct pt_regs *regs)
}
}
- if (addr->word != breakpoint_insn.word) {
+ if (*addr != breakpoint_insn) {
/*
* The breakpoint instruction was removed right
* after we hit it. Another cpu has removed
--
2.1.0
The following three functions will be used for uprobes, move them
from kprobes.c to inst.h:
insns_not_supported()
insns_need_simulation()
arch_simulate_insn()
This is preparation for later patch, no functionality change.
Signed-off-by: Tiezhu Yang <[email protected]>
---
arch/loongarch/include/asm/inst.h | 39 +++++++++++++++++++++++++++++++++
arch/loongarch/kernel/kprobes.c | 46 ++-------------------------------------
2 files changed, 41 insertions(+), 44 deletions(-)
diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
index a04fe75..af494b5 100644
--- a/arch/loongarch/include/asm/inst.h
+++ b/arch/loongarch/include/asm/inst.h
@@ -409,6 +409,45 @@ static inline bool is_self_loop_ins(union loongarch_instruction *ip, struct pt_r
void simu_pc(struct pt_regs *regs, union loongarch_instruction insn);
void simu_branch(struct pt_regs *regs, union loongarch_instruction insn);
+static inline bool insns_not_supported(union loongarch_instruction insn)
+{
+ switch (insn.reg2i14_format.opcode) {
+ case llw_op:
+ case lld_op:
+ case scw_op:
+ case scd_op:
+ pr_notice("kprobe: ll and sc instructions are not supported\n");
+ return true;
+ }
+
+ switch (insn.reg1i21_format.opcode) {
+ case bceqz_op:
+ pr_notice("kprobe: bceqz and bcnez instructions are not supported\n");
+ return true;
+ }
+
+ return false;
+}
+
+static inline bool insns_need_simulation(union loongarch_instruction insn)
+{
+ if (is_pc_ins(&insn))
+ return true;
+
+ if (is_branch_ins(&insn))
+ return true;
+
+ return false;
+}
+
+static inline void arch_simulate_insn(union loongarch_instruction insn, struct pt_regs *regs)
+{
+ if (is_pc_ins(&insn))
+ simu_pc(regs, insn);
+ else if (is_branch_ins(&insn))
+ simu_branch(regs, insn);
+}
+
int larch_insn_read(void *addr, u32 *insnp);
int larch_insn_write(void *addr, u32 insn);
int larch_insn_patch_text(void *addr, u32 insn);
diff --git a/arch/loongarch/kernel/kprobes.c b/arch/loongarch/kernel/kprobes.c
index 56c8c4b..08c78d2 100644
--- a/arch/loongarch/kernel/kprobes.c
+++ b/arch/loongarch/kernel/kprobes.c
@@ -21,48 +21,6 @@ static const union loongarch_instruction singlestep_insn = {
DEFINE_PER_CPU(struct kprobe *, current_kprobe);
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
-static bool insns_not_supported(union loongarch_instruction insn)
-{
- switch (insn.reg2i14_format.opcode) {
- case llw_op:
- case lld_op:
- case scw_op:
- case scd_op:
- pr_notice("kprobe: ll and sc instructions are not supported\n");
- return true;
- }
-
- switch (insn.reg1i21_format.opcode) {
- case bceqz_op:
- pr_notice("kprobe: bceqz and bcnez instructions are not supported\n");
- return true;
- }
-
- return false;
-}
-NOKPROBE_SYMBOL(insns_not_supported);
-
-static bool insns_need_simulation(struct kprobe *p)
-{
- if (is_pc_ins(&p->opcode))
- return true;
-
- if (is_branch_ins(&p->opcode))
- return true;
-
- return false;
-}
-NOKPROBE_SYMBOL(insns_need_simulation);
-
-static void arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
-{
- if (is_pc_ins(&p->opcode))
- simu_pc(regs, p->opcode);
- else if (is_branch_ins(&p->opcode))
- simu_branch(regs, p->opcode);
-}
-NOKPROBE_SYMBOL(arch_simulate_insn);
-
static void arch_prepare_ss_slot(struct kprobe *p)
{
p->ainsn.insn[0] = *p->addr;
@@ -89,7 +47,7 @@ int arch_prepare_kprobe(struct kprobe *p)
if (insns_not_supported(p->opcode))
return -EINVAL;
- if (insns_need_simulation(p)) {
+ if (insns_need_simulation(p->opcode)) {
p->ainsn.insn = NULL;
} else {
p->ainsn.insn = get_insn_slot();
@@ -220,7 +178,7 @@ static void setup_singlestep(struct kprobe *p, struct pt_regs *regs,
regs->csr_era = (unsigned long)p->ainsn.insn;
} else {
/* simulate single steping */
- arch_simulate_insn(p, regs);
+ arch_simulate_insn(p->opcode, regs);
/* now go for post processing */
post_kprobe_handler(p, kcb, regs);
}
--
2.1.0
Uprobes is the user-space counterpart to kprobes, this commit
adds uprobes support for LoongArch.
Signed-off-by: Tiezhu Yang <[email protected]>
---
arch/loongarch/Kconfig | 3 +
arch/loongarch/include/asm/uprobes.h | 36 +++++++++
arch/loongarch/kernel/Makefile | 1 +
arch/loongarch/kernel/traps.c | 7 +-
arch/loongarch/kernel/uprobes.c | 138 +++++++++++++++++++++++++++++++++++
5 files changed, 181 insertions(+), 4 deletions(-)
create mode 100644 arch/loongarch/include/asm/uprobes.h
create mode 100644 arch/loongarch/kernel/uprobes.c
diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index 7fd5125..ab66ad2 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -574,6 +574,9 @@ config ARCH_MMAP_RND_BITS_MIN
config ARCH_MMAP_RND_BITS_MAX
default 18
+config ARCH_SUPPORTS_UPROBES
+ def_bool y
+
menu "Power management options"
config ARCH_SUSPEND_POSSIBLE
diff --git a/arch/loongarch/include/asm/uprobes.h b/arch/loongarch/include/asm/uprobes.h
new file mode 100644
index 0000000..c6ffcb7
--- /dev/null
+++ b/arch/loongarch/include/asm/uprobes.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_LOONGARCH_UPROBES_H
+#define __ASM_LOONGARCH_UPROBES_H
+
+#include <asm/inst.h>
+
+typedef u32 uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES LOONGARCH_INSN_SIZE
+#define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES
+
+#define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP)
+#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP)
+#define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE
+
+struct arch_uprobe {
+ union {
+ u32 insn[MAX_UINSN_BYTES];
+ u32 ixol[MAX_UINSN_BYTES];
+ };
+ bool simulate;
+};
+
+struct arch_uprobe_task {
+ unsigned long saved_trap_nr;
+};
+
+#ifdef CONFIG_UPROBES
+bool uprobe_breakpoint_handler(struct pt_regs *regs);
+bool uprobe_singlestep_handler(struct pt_regs *regs);
+#else /* !CONFIG_UPROBES */
+static inline bool uprobe_breakpoint_handler(struct pt_regs *regs) { return false; }
+static inline bool uprobe_singlestep_handler(struct pt_regs *regs) { return false; }
+#endif /* CONFIG_UPROBES */
+
+#endif /* __ASM_LOONGARCH_UPROBES_H */
diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
index 78d4e33..67b9c26 100644
--- a/arch/loongarch/kernel/Makefile
+++ b/arch/loongarch/kernel/Makefile
@@ -53,5 +53,6 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
obj-$(CONFIG_KPROBES) += kprobes.o kprobes_trampoline.o
+obj-$(CONFIG_UPROBES) += uprobes.o
CPPFLAGS_vmlinux.lds := $(KBUILD_CFLAGS)
diff --git a/arch/loongarch/kernel/traps.c b/arch/loongarch/kernel/traps.c
index de8ebe2..ef6b749 100644
--- a/arch/loongarch/kernel/traps.c
+++ b/arch/loongarch/kernel/traps.c
@@ -45,6 +45,7 @@
#include <asm/tlb.h>
#include <asm/types.h>
#include <asm/unwind.h>
+#include <asm/uprobes.h>
#include "access-helper.h"
@@ -462,14 +463,12 @@ asmlinkage void noinstr do_bp(struct pt_regs *regs)
else
break;
case BRK_UPROBE_BP:
- if (notify_die(DIE_UPROBE, "Uprobe", regs, bcode,
- current->thread.trap_nr, SIGTRAP) == NOTIFY_STOP)
+ if (uprobe_breakpoint_handler(regs))
goto out;
else
break;
case BRK_UPROBE_XOLBP:
- if (notify_die(DIE_UPROBE_XOL, "Uprobe_XOL", regs, bcode,
- current->thread.trap_nr, SIGTRAP) == NOTIFY_STOP)
+ if (uprobe_singlestep_handler(regs))
goto out;
else
break;
diff --git a/arch/loongarch/kernel/uprobes.c b/arch/loongarch/kernel/uprobes.c
new file mode 100644
index 0000000..afdeff3
--- /dev/null
+++ b/arch/loongarch/kernel/uprobes.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/highmem.h>
+#include <linux/ptrace.h>
+#include <linux/uprobes.h>
+#include <linux/sched.h>
+#include <asm/cacheflush.h>
+
+#define UPROBE_TRAP_NR UINT_MAX
+
+void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
+ void *src, unsigned long len)
+{
+ void *kaddr = kmap_local_page(page);
+ void *dst = kaddr + (vaddr & ~PAGE_MASK);
+
+ memcpy(dst, src, len);
+ flush_icache_range((unsigned long)dst, (unsigned long)dst + len);
+ kunmap_local(kaddr);
+}
+
+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
+{
+ return instruction_pointer(regs);
+}
+
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
+ struct mm_struct *mm, unsigned long addr)
+{
+ if (addr & 0x3)
+ return -EILSEQ;
+
+ if (insns_not_supported(auprobe->insn[0]))
+ return -EINVAL;
+
+ if (insns_need_simulation(auprobe->insn[0])) {
+ auprobe->ixol[0] = larch_insn_gen_nop();
+ auprobe->simulate = true;
+ } else {
+ auprobe->ixol[0] = auprobe->insn[0];
+ auprobe->simulate = false;
+ }
+
+ auprobe->ixol[1] = UPROBE_XOLBP_INSN;
+
+ return 0;
+}
+
+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ struct uprobe_task *utask = current->utask;
+
+ utask->autask.saved_trap_nr = current->thread.trap_nr;
+ current->thread.trap_nr = UPROBE_TRAP_NR;
+
+ instruction_pointer_set(regs, utask->xol_vaddr);
+
+ return 0;
+}
+
+int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ struct uprobe_task *utask = current->utask;
+
+ WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
+
+ instruction_pointer_set(regs, utask->vaddr + 4);
+
+ return 0;
+}
+
+void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ struct uprobe_task *utask = current->utask;
+
+ /* Task has received a fatal signal, so reset back to probed address. */
+ instruction_pointer_set(regs, utask->vaddr);
+}
+
+bool arch_uprobe_xol_was_trapped(struct task_struct *t)
+{
+ if (t->thread.trap_nr != UPROBE_TRAP_NR)
+ return true;
+
+ return false;
+}
+
+/* Return true if instruction was emulated, false otherwise. */
+bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ if (!auprobe->simulate)
+ return false;
+
+ arch_simulate_insn(auprobe->insn[0], regs);
+
+ return true;
+}
+
+unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
+ struct pt_regs *regs)
+{
+ unsigned long ra = regs->regs[1];
+
+ /* Replace the return addr with trampoline addr */
+ regs->regs[1] = trampoline_vaddr;
+
+ return ra;
+}
+
+bool arch_uretprobe_is_alive(struct return_instance *ret,
+ enum rp_check ctx, struct pt_regs *regs)
+{
+ if (ctx == RP_CHECK_CHAIN_CALL)
+ return regs->regs[3] <= ret->stack;
+ else
+ return regs->regs[3] < ret->stack;
+}
+
+int arch_uprobe_exception_notify(struct notifier_block *self,
+ unsigned long val, void *data)
+{
+ return NOTIFY_DONE;
+}
+
+bool uprobe_breakpoint_handler(struct pt_regs *regs)
+{
+ if (uprobe_pre_sstep_notifier(regs))
+ return true;
+
+ return false;
+}
+
+bool uprobe_singlestep_handler(struct pt_regs *regs)
+{
+ if (uprobe_post_sstep_notifier(regs))
+ return true;
+
+ return false;
+}
--
2.1.0
/* snip */
> diff --git a/arch/loongarch/kernel/kprobes.c b/arch/loongarch/kernel/kprobes.c
> index 08c78d2..a5c3712 100644
> --- a/arch/loongarch/kernel/kprobes.c
> +++ b/arch/loongarch/kernel/kprobes.c
> @@ -4,19 +4,8 @@
> #include <linux/preempt.h>
> #include <asm/break.h>
>
> -static const union loongarch_instruction breakpoint_insn = {
> - .reg0i15_format = {
> - .opcode = break_op,
> - .immediate = BRK_KPROBE_BP,
> - }
> -};
> -
> -static const union loongarch_instruction singlestep_insn = {
> - .reg0i15_format = {
> - .opcode = break_op,
> - .immediate = BRK_KPROBE_SSTEPBP,
> - }
> -};
> +#define breakpoint_insn larch_insn_gen_break(BRK_KPROBE_BP)
> +#define singlestep_insn larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
IMO, Defined as KPROBE_BP_INSN, KPROBE_SSTEPBP_INSN may be better.
Youling.
>
> DEFINE_PER_CPU(struct kprobe *, current_kprobe);
> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> @@ -253,7 +242,7 @@ bool kprobe_breakpoint_handler(struct pt_regs *regs)
> }
> }
>
> - if (addr->word != breakpoint_insn.word) {
> + if (*addr != breakpoint_insn) {
> /*
> * The breakpoint instruction was removed right
> * after we hit it. Another cpu has removed
>
On 2023/4/7 10:30, Youling Tang wrote:
> /* snip */
>
>> diff --git a/arch/loongarch/kernel/kprobes.c
>> b/arch/loongarch/kernel/kprobes.c
>> index 08c78d2..a5c3712 100644
>> --- a/arch/loongarch/kernel/kprobes.c
>> +++ b/arch/loongarch/kernel/kprobes.c
>> @@ -4,19 +4,8 @@
>> #include <linux/preempt.h>
>> #include <asm/break.h>
>>
>> -static const union loongarch_instruction breakpoint_insn = {
>> - .reg0i15_format = {
>> - .opcode = break_op,
>> - .immediate = BRK_KPROBE_BP,
>> - }
>> -};
>> -
>> -static const union loongarch_instruction singlestep_insn = {
>> - .reg0i15_format = {
>> - .opcode = break_op,
>> - .immediate = BRK_KPROBE_SSTEPBP,
>> - }
>> -};
>> +#define breakpoint_insn larch_insn_gen_break(BRK_KPROBE_BP)
>> +#define singlestep_insn larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
>
> IMO, Defined as KPROBE_BP_INSN, KPROBE_SSTEPBP_INSN may be better.
Are you suggesting to hardcode the instruction words for those two BREAK
flavors? I don't think it's better because even more structured info is
lost, and the compiler would generate the same code (if not, it's the
compiler that's to be fixed).
Actually, I don't know why this commit was necessary in the first place.
For the very least, it consisted of two logical changes (pass around
instruction words instead of unions; and change the BREAK insns to make
them words) that should get split; but again, the generated code should
be identical anyway, so it seems a lot of churn for no benefit and
reduced readability.
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
On 04/07/2023 05:51 PM, WANG Xuerui wrote:
> On 2023/4/7 10:30, Youling Tang wrote:
>> /* snip */
>>
>>> diff --git a/arch/loongarch/kernel/kprobes.c
>>> b/arch/loongarch/kernel/kprobes.c
>>> index 08c78d2..a5c3712 100644
>>> --- a/arch/loongarch/kernel/kprobes.c
>>> +++ b/arch/loongarch/kernel/kprobes.c
>>> @@ -4,19 +4,8 @@
>>> #include <linux/preempt.h>
>>> #include <asm/break.h>
>>>
>>> -static const union loongarch_instruction breakpoint_insn = {
>>> - .reg0i15_format = {
>>> - .opcode = break_op,
>>> - .immediate = BRK_KPROBE_BP,
>>> - }
>>> -};
>>> -
>>> -static const union loongarch_instruction singlestep_insn = {
>>> - .reg0i15_format = {
>>> - .opcode = break_op,
>>> - .immediate = BRK_KPROBE_SSTEPBP,
>>> - }
>>> -};
>>> +#define breakpoint_insn larch_insn_gen_break(BRK_KPROBE_BP)
>>> +#define singlestep_insn larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
>>
>> IMO, Defined as KPROBE_BP_INSN, KPROBE_SSTEPBP_INSN may be better.
>
> Are you suggesting to hardcode the instruction words for those two BREAK
> flavors?
I think what Youling said is:
#define KPROBE_BP_INSN larch_insn_gen_break(BRK_KPROBE_BP)
#define KPROBE_SSTEPBP_INSN larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
> I don't think it's better because even more structured info is
> lost, and the compiler would generate the same code (if not, it's the
> compiler that's to be fixed).
>
> Actually, I don't know why this commit was necessary in the first place.
> For the very least, it consisted of two logical changes (pass around
> instruction words instead of unions; and change the BREAK insns to make
> them words) that should get split;
Yes, thanks for your suggestion, I will split it into two patches
in the next version.
> but again, the generated code should
> be identical anyway, so it seems a lot of churn for no benefit and
> reduced readability.
>
Define and use larch_insn_gen_break() is to avoid hardcoding the
uprobe break instruction in patch #3.
We do not like the following definitions:
#define UPROBE_SWBP_INSN 0x002a000c
#define UPROBE_XOLBP_INSN 0x002a000d
Using larch_insn_gen_break() seems better:
#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP)
#define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP)
Thanks,
Tiezhu
On 2023/4/7 20:02, Tiezhu Yang wrote:
>
>
> On 04/07/2023 05:51 PM, WANG Xuerui wrote:
>> On 2023/4/7 10:30, Youling Tang wrote:
>>> /* snip */
>>>
>>>> diff --git a/arch/loongarch/kernel/kprobes.c
>>>> b/arch/loongarch/kernel/kprobes.c
>>>> index 08c78d2..a5c3712 100644
>>>> --- a/arch/loongarch/kernel/kprobes.c
>>>> +++ b/arch/loongarch/kernel/kprobes.c
>>>> @@ -4,19 +4,8 @@
>>>> #include <linux/preempt.h>
>>>> #include <asm/break.h>
>>>>
>>>> -static const union loongarch_instruction breakpoint_insn = {
>>>> - .reg0i15_format = {
>>>> - .opcode = break_op,
>>>> - .immediate = BRK_KPROBE_BP,
>>>> - }
>>>> -};
>>>> -
>>>> -static const union loongarch_instruction singlestep_insn = {
>>>> - .reg0i15_format = {
>>>> - .opcode = break_op,
>>>> - .immediate = BRK_KPROBE_SSTEPBP,
>>>> - }
>>>> -};
>>>> +#define breakpoint_insn larch_insn_gen_break(BRK_KPROBE_BP)
>>>> +#define singlestep_insn larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
>>>
>>> IMO, Defined as KPROBE_BP_INSN, KPROBE_SSTEPBP_INSN may be better.
>>
>> Are you suggesting to hardcode the instruction words for those two BREAK
>> flavors?
>
> I think what Youling said is:
>
> #define KPROBE_BP_INSN larch_insn_gen_break(BRK_KPROBE_BP)
> #define KPROBE_SSTEPBP_INSN larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
>
>> I don't think it's better because even more structured info is
>> lost, and the compiler would generate the same code (if not, it's the
>> compiler that's to be fixed).
>>
>> Actually, I don't know why this commit was necessary in the first place.
>> For the very least, it consisted of two logical changes (pass around
>> instruction words instead of unions; and change the BREAK insns to make
>> them words) that should get split;
>
> Yes, thanks for your suggestion, I will split it into two patches
> in the next version.
>
>> but again, the generated code should
>> be identical anyway, so it seems a lot of churn for no benefit and
>> reduced readability.
>>
>
> Define and use larch_insn_gen_break() is to avoid hardcoding the
> uprobe break instruction in patch #3.
>
> We do not like the following definitions:
>
> #define UPROBE_SWBP_INSN 0x002a000c
> #define UPROBE_XOLBP_INSN 0x002a000d
>
> Using larch_insn_gen_break() seems better:
>
> #define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP)
> #define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP)
Sorry, I meant *not* ditching the union-typed parameters. IMO they
should behave the same codegen-wise (i.e. unchanged performance), and
have the benefit of being clearly typed unlike plain u32's.
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/