diff mbox series

[RFC,2/3] LoongArch: Add larch_insn_gen_break() to generate break insn

Message ID 1680833701-1727-3-git-send-email-yangtiezhu@loongson.cn
State New
Headers
Series Add uprobes support for LoongArch |

Commit Message

Tiezhu Yang April 7, 2023, 2:15 a.m. UTC 
  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 <yangtiezhu@loongson.cn>
---
 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(-)

Comments

Youling Tang April 7, 2023, 2:30 a.m. UTC | #1 
/* 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
>
WANG Xuerui April 7, 2023, 9:51 a.m. UTC | #2 
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.
Tiezhu Yang April 7, 2023, 12:02 p.m. UTC | #3 
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
WANG Xuerui April 10, 2023, 2:16 p.m. UTC | #4 
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.
diff mbox series

Patch

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