diff mbox series

[v1,3/4] LoongArch: Use larch_insn_gen_break() for kprobes

Message ID 1681293901-24918-4-git-send-email-yangtiezhu@loongson.cn
State New
Headers
Series Add uprobes support for LoongArch |

Commit Message

Tiezhu Yang April 12, 2023, 10:05 a.m. UTC 
  For now, we can use larch_insn_gen_break() to define KPROBE_BP_INSN and
KPROBE_SSTEPBP_INSN. Because larch_insn_gen_break() returns instruction
word, pass around instruction word instead of union for the functions
insns_not_supported(), insns_need_simulation() and arch_simulate_insn(),
no functional change intended.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/include/asm/inst.h    | 18 +++++++++++++++---
 arch/loongarch/include/asm/kprobes.h |  2 +-
 arch/loongarch/kernel/kprobes.c      | 21 +++++----------------
 3 files changed, 21 insertions(+), 20 deletions(-)

Comments

Xi Ruoyao April 12, 2023, 11:08 a.m. UTC | #1 
On Wed, 2023-04-12 at 18:05 +0800, Tiezhu Yang wrote:
> -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;
> +

I remember Xuerui disliked this change.  Maybe we can add
__attribute__ (__transparent_union__) [1] for union
loongarch_instruction instead?

[1]: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-transparent_005funion-type-attribute
Tiezhu Yang April 13, 2023, 1:58 a.m. UTC | #2 
On 04/12/2023 07:08 PM, Xi Ruoyao wrote:
> On Wed, 2023-04-12 at 18:05 +0800, Tiezhu Yang wrote:
>> -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;
>> +
>
> I remember Xuerui disliked this change.  Maybe we can add
> __attribute__ (__transparent_union__) [1] for union
> loongarch_instruction instead?
>
> [1]: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-transparent_005funion-type-attribute
>

OK, let me pass union parameter, then define a local variable
to do type conversion for callers, the changes are small and
then the code may be more readable, thank you.

Thanks,
Tiezhu
diff mbox series

Patch

diff --git a/arch/loongarch/include/asm/inst.h b/arch/loongarch/include/asm/inst.h
index 95b3c20..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))
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/kprobes.c b/arch/loongarch/kernel/kprobes.c
index 08c78d2..9f699f0 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 KPROBE_BP_INSN		larch_insn_gen_break(BRK_KPROBE_BP)
+#define KPROBE_SSTEPBP_INSN	larch_insn_gen_break(BRK_KPROBE_SSTEPBP)
 
 DEFINE_PER_CPU(struct kprobe *, current_kprobe);
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -24,7 +13,7 @@  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 static void arch_prepare_ss_slot(struct kprobe *p)
 {
 	p->ainsn.insn[0] = *p->addr;
-	p->ainsn.insn[1] = singlestep_insn;
+	p->ainsn.insn[1] = KPROBE_SSTEPBP_INSN;
 	p->ainsn.restore = (unsigned long)p->addr + LOONGARCH_INSN_SIZE;
 }
 NOKPROBE_SYMBOL(arch_prepare_ss_slot);
@@ -68,7 +57,7 @@  NOKPROBE_SYMBOL(arch_prepare_kprobe);
 /* Install breakpoint in text */
 void arch_arm_kprobe(struct kprobe *p)
 {
-	*p->addr = breakpoint_insn;
+	*p->addr = KPROBE_BP_INSN;
 	flush_insn_slot(p);
 }
 NOKPROBE_SYMBOL(arch_arm_kprobe);
@@ -253,7 +242,7 @@  bool kprobe_breakpoint_handler(struct pt_regs *regs)
 		}
 	}
 
-	if (addr->word != breakpoint_insn.word) {
+	if (*addr != KPROBE_BP_INSN) {
 		/*
 		 * The breakpoint instruction was removed right
 		 * after we hit it.  Another cpu has removed