This patchset adds initial support for hardware breakpoints and watchpoints to the RISC-V architecture. The framework is built on top of perf subsystem and SBI debug trigger extension.
Currently following features are not supported and are in works: - icount for single stepping - Virtualization of debug triggers - kernel space debug triggers
The SBI debug trigger extension can be found at: https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-debug-tri...
The Sdtrig ISA is part of RISC-V debug specification which can be found at: https://github.com/riscv/riscv-debug-spec
based off the original RFC by Himanshu Chauhan here: https://lore.kernel.org/lkml/20240222125059.13331-1-hchauhan@ventanamicro.co...
Second RFC by Jesse Taube here: https://lore.kernel.org/lkml/20250722173829.984082-1-jesse@rivosinc.com/
Himanshu Chauhan (2): riscv: Add SBI debug trigger extension and function ids riscv: Introduce support for hardware break/watchpoints
Jesse Taube (6): riscv: Add insn.c, consolidate instruction decoding riscv: insn: Add get_insn_nofault riscv: hw_breakpoint: Use icount for single stepping riscv: ptrace: Add hw breakpoint support riscv: ptrace: Add hw breakpoint regset selftests: riscv: Add test for hardware breakpoints
arch/riscv/Kconfig | 2 + arch/riscv/include/asm/bug.h | 12 - arch/riscv/include/asm/hw_breakpoint.h | 59 ++ arch/riscv/include/asm/insn.h | 132 ++- arch/riscv/include/asm/kdebug.h | 3 +- arch/riscv/include/asm/processor.h | 4 + arch/riscv/include/asm/sbi.h | 33 +- arch/riscv/include/uapi/asm/ptrace.h | 9 + arch/riscv/kernel/Makefile | 2 + arch/riscv/kernel/hw_breakpoint.c | 763 ++++++++++++++++++ arch/riscv/kernel/insn.c | 165 ++++ arch/riscv/kernel/kgdb.c | 102 +-- arch/riscv/kernel/probes/kprobes.c | 1 + arch/riscv/kernel/process.c | 4 + arch/riscv/kernel/ptrace.c | 169 ++++ arch/riscv/kernel/traps.c | 11 +- arch/riscv/kernel/traps_misaligned.c | 93 +-- include/uapi/linux/elf.h | 2 + tools/include/uapi/linux/elf.h | 1 + tools/perf/tests/tests.h | 3 +- tools/testing/selftests/riscv/Makefile | 2 +- .../selftests/riscv/breakpoints/.gitignore | 1 + .../selftests/riscv/breakpoints/Makefile | 13 + .../riscv/breakpoints/breakpoint_test.c | 246 ++++++ 24 files changed, 1641 insertions(+), 191 deletions(-) create mode 100644 arch/riscv/include/asm/hw_breakpoint.h create mode 100644 arch/riscv/kernel/hw_breakpoint.c create mode 100644 arch/riscv/kernel/insn.c create mode 100644 tools/testing/selftests/riscv/breakpoints/.gitignore create mode 100644 tools/testing/selftests/riscv/breakpoints/Makefile create mode 100644 tools/testing/selftests/riscv/breakpoints/breakpoint_test.c
Various parts of the kernel decode and read instruction from memory. Functions like get_insn, GET_INSN_LENGTH and riscv_insn_is_c are defined in multiple places. Consolidate these functions into the insn.h and the newly added insn.c.
Signed-off-by: Jesse Taube jesse@rivosinc.com --- RFC -> V1: - No change V2 -> V1: - No change --- arch/riscv/include/asm/bug.h | 12 --- arch/riscv/include/asm/insn.h | 131 ++++++++++++++++++++++- arch/riscv/kernel/Makefile | 1 + arch/riscv/kernel/insn.c | 151 +++++++++++++++++++++++++++ arch/riscv/kernel/kgdb.c | 102 +----------------- arch/riscv/kernel/probes/kprobes.c | 1 + arch/riscv/kernel/traps.c | 5 +- arch/riscv/kernel/traps_misaligned.c | 93 ++++------------- 8 files changed, 309 insertions(+), 187 deletions(-) create mode 100644 arch/riscv/kernel/insn.c
diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h index 1aaea81fb141..a2777eb67ad1 100644 --- a/arch/riscv/include/asm/bug.h +++ b/arch/riscv/include/asm/bug.h @@ -12,21 +12,9 @@
#include <asm/asm.h>
-#define __INSN_LENGTH_MASK _UL(0x3) -#define __INSN_LENGTH_32 _UL(0x3) -#define __COMPRESSED_INSN_MASK _UL(0xffff) - #define __BUG_INSN_32 _UL(0x00100073) /* ebreak */ #define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */
-#define GET_INSN_LENGTH(insn) \ -({ \ - unsigned long __len; \ - __len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? \ - 4UL : 2UL; \ - __len; \ -}) - typedef u32 bug_insn_t;
#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h index 09fde95a5e8f..ba74e5b8262c 100644 --- a/arch/riscv/include/asm/insn.h +++ b/arch/riscv/include/asm/insn.h @@ -64,6 +64,7 @@ #define RVG_RS2_OPOFF 20 #define RVG_RD_OPOFF 7 #define RVG_RS1_MASK GENMASK(4, 0) +#define RVG_RS2_MASK GENMASK(4, 0) #define RVG_RD_MASK GENMASK(4, 0)
/* The bit field of immediate value in RVC J instruction */ @@ -121,17 +122,27 @@ #define RVC_C0_RS1_OPOFF 7 #define RVC_C0_RS2_OPOFF 2 #define RVC_C0_RD_OPOFF 2 +#define RVC_C0_RS1_MASK GENMASK(2, 0) +#define RVC_C0_RS2_MASK GENMASK(2, 0) +#define RVC_C0_RD_MASK GENMASK(2, 0) +#define RVC_C0_REG_OFFSET 8
/* The register offset in RVC op=C1 instruction */ #define RVC_C1_RS1_OPOFF 7 #define RVC_C1_RS2_OPOFF 2 #define RVC_C1_RD_OPOFF 7 +#define RVC_C1_RS1_MASK GENMASK(2, 0) +#define RVC_C1_RS2_MASK GENMASK(2, 0) +#define RVC_C1_RD_MASK GENMASK(2, 0) +#define RVC_C1_REG_OFFSET 8
/* The register offset in RVC op=C2 instruction */ #define RVC_C2_RS1_OPOFF 7 #define RVC_C2_RS2_OPOFF 2 #define RVC_C2_RD_OPOFF 7 #define RVC_C2_RS1_MASK GENMASK(4, 0) +#define RVC_C2_RS2_MASK GENMASK(4, 0) +#define RVC_C2_RD_MASK GENMASK(4, 0)
/* parts of opcode for RVG*/ #define RVG_OPCODE_FENCE 0x0f @@ -226,12 +237,26 @@ #define RVC_MASK_C_EBREAK 0xffff #define RVG_MASK_EBREAK 0xffffffff #define RVG_MASK_SRET 0xffffffff +#define RVC_MASK_C GENMASK(15, 0)
#define __INSN_LENGTH_MASK _UL(0x3) #define __INSN_LENGTH_GE_32 _UL(0x3) #define __INSN_OPCODE_MASK _UL(0x7F) #define __INSN_BRANCH_OPCODE _UL(RVG_OPCODE_BRANCH)
+#define GET_INSN_LENGTH(insn) \ +({ \ + unsigned long __len; \ + __len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_GE_32) ? \ + 4UL : 2UL; \ + __len; \ +}) + +static __always_inline bool riscv_insn_is_c(u32 code) +{ + return (code & (__INSN_LENGTH_MASK)) != (__INSN_LENGTH_GE_32); +} + #define __RISCV_INSN_FUNCS(name, mask, val) \ static __always_inline bool riscv_insn_is_##name(u32 code) \ { \ @@ -260,7 +285,7 @@ __RISCV_INSN_FUNCS(c_bnez, RVC_MASK_C_BNEZ, RVC_MATCH_C_BNEZ) __RISCV_INSN_FUNCS(c_ebreak, RVC_MASK_C_EBREAK, RVC_MATCH_C_EBREAK) __RISCV_INSN_FUNCS(ebreak, RVG_MASK_EBREAK, RVG_MATCH_EBREAK) __RISCV_INSN_FUNCS(sret, RVG_MASK_SRET, RVG_MATCH_SRET) -__RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE); +__RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE)
/* special case to catch _any_ system instruction */ static __always_inline bool riscv_insn_is_system(u32 code) @@ -295,6 +320,10 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code) ({typeof(x) x_ = (x); \ (RV_X(x_, RVG_RS1_OPOFF, RVG_RS1_MASK)); })
+#define RV_EXTRACT_RS2_REG(x) \ + ({typeof(x) x_ = (x); \ + (RV_X(x_, RVG_RS2_OPOFF, RVG_RS2_MASK)); }) + #define RV_EXTRACT_RD_REG(x) \ ({typeof(x) x_ = (x); \ (RV_X(x_, RVG_RD_OPOFF, RVG_RD_MASK)); }) @@ -322,9 +351,41 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code) (RV_X(x_, RV_B_IMM_11_OPOFF, RV_B_IMM_11_MASK) << RV_B_IMM_11_OFF) | \ (RV_IMM_SIGN(x_) << RV_B_IMM_SIGN_OFF); })
+#define RVC_EXTRACT_C0_RS1_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C0_RS1_OPOFF, RVC_C0_RS1_MASK)); }) + +#define RVC_EXTRACT_C0_RS2_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C0_RS2_OPOFF, RVC_C0_RS2_MASK)); }) + +#define RVC_EXTRACT_C0_RD_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C0_RD_OPOFF, RVC_C0_RD_MASK)); }) + +#define RVC_EXTRACT_C1_RS1_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C1_RS1_OPOFF, RVC_C1_RS1_MASK)); }) + +#define RVC_EXTRACT_C1_RS2_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C1_RS2_OPOFF, RVC_C1_RS2_MASK)); }) + +#define RVC_EXTRACT_C1_RD_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C1_RD_OPOFF, RVC_C1_RD_MASK)); }) + #define RVC_EXTRACT_C2_RS1_REG(x) \ ({typeof(x) x_ = (x); \ - (RV_X(x_, RVC_C2_RS1_OPOFF, RVC_C2_RS1_MASK)); }) + (RVC_X(x_, RVC_C2_RS1_OPOFF, RVC_C2_RS1_MASK)); }) + +#define RVC_EXTRACT_C2_RS2_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C2_RS2_OPOFF, RVC_C2_RS2_MASK)); }) + +#define RVC_EXTRACT_C2_RD_REG(x) \ + ({typeof(x) x_ = (x); \ + (RVC_X(x_, RVC_C2_RD_OPOFF, RVC_C2_RD_MASK)); })
#define RVC_EXTRACT_JTYPE_IMM(x) \ ({typeof(x) x_ = (x); \ @@ -354,6 +415,66 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code)
#define RVV_EXRACT_VL_VS_WIDTH(x) RVFDQ_EXTRACT_FL_FS_WIDTH(x)
+/* + * Get the rs1 register number from RV or RVC instruction. + * + * @insn: instruction to process + * Return: rs1 register + */ +static inline unsigned int riscv_insn_extract_rs1_reg(u32 insn) +{ + switch (RVC_INSN_OPCODE_MASK & insn) { + case RVC_OPCODE_C0: + return RVC_EXTRACT_C0_RS1_REG(insn) + RVC_C0_REG_OFFSET; + case RVC_OPCODE_C1: + return RVC_EXTRACT_C1_RS1_REG(insn) + RVC_C1_REG_OFFSET; + case RVC_OPCODE_C2: + return RVC_EXTRACT_C2_RS1_REG(insn); + default: + return RV_EXTRACT_RS1_REG(insn); + } +} + +/* + * Get the rs2 register number from RV or RVC instruction. + * + * @insn: instruction to process + * Return: rs2 register + */ +static inline unsigned int riscv_insn_extract_rs2_reg(u32 insn) +{ + switch (RVC_INSN_OPCODE_MASK & insn) { + case RVC_OPCODE_C0: + return RVC_EXTRACT_C0_RS2_REG(insn) + RVC_C0_REG_OFFSET; + case RVC_OPCODE_C1: + return RVC_EXTRACT_C1_RS2_REG(insn) + RVC_C1_REG_OFFSET; + case RVC_OPCODE_C2: + return RVC_EXTRACT_C2_RS2_REG(insn); + default: + return RV_EXTRACT_RS2_REG(insn); + } +} + +/* + * Get the rd register number from RV or RVC instruction. + * + * @insn: instruction to process + * Return: rd register + */ +static inline unsigned int riscv_insn_extract_rd_reg(u32 insn) +{ + switch (RVC_INSN_OPCODE_MASK & insn) { + case RVC_OPCODE_C0: + return RVC_EXTRACT_C0_RD_REG(insn) + RVC_C0_REG_OFFSET; + case RVC_OPCODE_C1: + return RVC_EXTRACT_C1_RD_REG(insn) + RVC_C1_REG_OFFSET; + case RVC_OPCODE_C2: + return RVC_EXTRACT_C2_RD_REG(insn); + default: + return RV_EXTRACT_RD_REG(insn); + } +} + /* * Get the immediate from a J-type instruction. * @@ -428,4 +549,10 @@ static inline void riscv_insn_insert_utype_itype_imm(u32 *utype_insn, u32 *itype *utype_insn |= (imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1); *itype_insn |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF); } + +#include <asm/ptrace.h> + +int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn); +unsigned long get_step_address(struct pt_regs *regs, u32 code); + #endif /* _ASM_RISCV_INSN_H */ diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index f7480c9c6f8d..4f719b09e5ad 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -51,6 +51,7 @@ obj-$(CONFIG_RISCV_ALTERNATIVE) += alternative.o obj-y += cpu.o obj-y += cpufeature.o obj-y += entry.o +obj-y += insn.o obj-y += irq.o obj-y += process.o obj-y += ptrace.o diff --git a/arch/riscv/kernel/insn.c b/arch/riscv/kernel/insn.c new file mode 100644 index 000000000000..dd2a6ef9fd25 --- /dev/null +++ b/arch/riscv/kernel/insn.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2025 Rivos, Inc + */ +#include <asm/insn.h> +#include <asm/ptrace.h> +#include <asm/uaccess.h> + +#define __read_insn(regs, insn, insn_addr, type) \ +({ \ + int __ret; \ + \ + if (user_mode(regs)) { \ + __ret = get_user(insn, (type __user *) insn_addr); \ + } else { \ + insn = *(type *)insn_addr; \ + __ret = 0; \ + } \ + \ + __ret; \ +}) + +/* + * Update a set of two instructions (U-type + I-type) with an immediate value. + * + * Used for example in auipc+jalrs pairs the U-type instructions contains + * a 20bit upper immediate representing bits[31:12], while the I-type + * instruction contains a 12bit immediate representing bits[11:0]. + * + * This also takes into account that both separate immediates are + * considered as signed values, so if the I-type immediate becomes + * negative (BIT(11) set) the U-type part gets adjusted. + * + * @regs: pointer to the utype instruction of the pair + * @epc: pointer to the itype instruction of the pair + * @r_insn: the immediate to insert into the two instructions + * Return: combined immediate + */ +int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn) +{ + ulong insn = 0; + + if (epc & 0x2) { + ulong tmp = 0; + + if (__read_insn(regs, insn, epc, u16)) + return -EFAULT; + /* __get_user() uses regular "lw" which sign extend the loaded + * value make sure to clear higher order bits in case we "or" it + * below with the upper 16 bits half. + */ + insn &= RVC_MASK_C; + if (riscv_insn_is_c(insn)) { + *r_insn = insn; + return 0; + } + epc += sizeof(u16); + if (__read_insn(regs, tmp, epc, u16)) + return -EFAULT; + *r_insn = (tmp << 16) | insn; + + return 0; + } else { + if (__read_insn(regs, insn, epc, u32)) + return -EFAULT; + if (!riscv_insn_is_c(insn)) { + *r_insn = insn; + return 0; + } + insn &= RVC_MASK_C; + *r_insn = insn; + + return 0; + } +} + +/* Calculate the new address for after a step */ +unsigned long get_step_address(struct pt_regs *regs, u32 code) +{ + unsigned long pc = regs->epc; + unsigned int rs1_num, rs2_num; + + if ((code & __INSN_LENGTH_MASK) != __INSN_LENGTH_GE_32) { + if (riscv_insn_is_c_jalr(code) || + riscv_insn_is_c_jr(code)) { + rs1_num = riscv_insn_extract_rs1_reg(code); + return regs_get_register(regs, rs1_num); + } else if (riscv_insn_is_c_j(code) || + riscv_insn_is_c_jal(code)) { + return RVC_EXTRACT_JTYPE_IMM(code) + pc; + } else if (riscv_insn_is_c_beqz(code)) { + rs1_num = riscv_insn_extract_rs1_reg(code); + if (!rs1_num || regs_get_register(regs, rs1_num) == 0) + return RVC_EXTRACT_BTYPE_IMM(code) + pc; + else + return pc + 2; + } else if (riscv_insn_is_c_bnez(code)) { + rs1_num = riscv_insn_extract_rs1_reg(RVC_C1_RS1_OPOFF); + if (rs1_num && regs_get_register(regs, rs1_num) != 0) + return RVC_EXTRACT_BTYPE_IMM(code) + pc; + else + return pc + 2; + } else { + return pc + 2; + } + } else { + if ((code & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE) { + bool result = false; + long imm = RV_EXTRACT_BTYPE_IMM(code); + unsigned long rs1_val = 0, rs2_val = 0; + + rs1_num = riscv_insn_extract_rs1_reg(code); + rs2_num = riscv_insn_extract_rs2_reg(code); + if (rs1_num) + rs1_val = regs_get_register(regs, rs1_num); + if (rs2_num) + rs2_val = regs_get_register(regs, rs2_num); + + if (riscv_insn_is_beq(code)) + result = (rs1_val == rs2_val) ? true : false; + else if (riscv_insn_is_bne(code)) + result = (rs1_val != rs2_val) ? true : false; + else if (riscv_insn_is_blt(code)) + result = + ((long)rs1_val < + (long)rs2_val) ? true : false; + else if (riscv_insn_is_bge(code)) + result = + ((long)rs1_val >= + (long)rs2_val) ? true : false; + else if (riscv_insn_is_bltu(code)) + result = (rs1_val < rs2_val) ? true : false; + else if (riscv_insn_is_bgeu(code)) + result = (rs1_val >= rs2_val) ? true : false; + if (result) + return imm + pc; + else + return pc + 4; + } else if (riscv_insn_is_jal(code)) { + return RV_EXTRACT_JTYPE_IMM(code) + pc; + } else if (riscv_insn_is_jalr(code)) { + rs1_num = riscv_insn_extract_rs1_reg(code); + return RV_EXTRACT_ITYPE_IMM(code) + + (rs1_num ? regs_get_register(regs, rs1_num) : 0); + } else if (riscv_insn_is_sret(code)) { + return pc; + } else { + return pc + 4; + } + } +} diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c index 9f3db3503dab..aafc1424fc81 100644 --- a/arch/riscv/kernel/kgdb.c +++ b/arch/riscv/kernel/kgdb.c @@ -23,111 +23,19 @@ enum { static unsigned long stepped_address; static unsigned int stepped_opcode;
-static int decode_register_index(unsigned long opcode, int offset) -{ - return (opcode >> offset) & 0x1F; -} - -static int decode_register_index_short(unsigned long opcode, int offset) -{ - return ((opcode >> offset) & 0x7) + 8; -} - -/* Calculate the new address for after a step */ -static int get_step_address(struct pt_regs *regs, unsigned long *next_addr) -{ - unsigned long pc = regs->epc; - unsigned long *regs_ptr = (unsigned long *)regs; - unsigned int rs1_num, rs2_num; - int op_code; - - if (get_kernel_nofault(op_code, (void *)pc)) - return -EINVAL; - if ((op_code & __INSN_LENGTH_MASK) != __INSN_LENGTH_GE_32) { - if (riscv_insn_is_c_jalr(op_code) || - riscv_insn_is_c_jr(op_code)) { - rs1_num = decode_register_index(op_code, RVC_C2_RS1_OPOFF); - *next_addr = regs_ptr[rs1_num]; - } else if (riscv_insn_is_c_j(op_code) || - riscv_insn_is_c_jal(op_code)) { - *next_addr = RVC_EXTRACT_JTYPE_IMM(op_code) + pc; - } else if (riscv_insn_is_c_beqz(op_code)) { - rs1_num = decode_register_index_short(op_code, - RVC_C1_RS1_OPOFF); - if (!rs1_num || regs_ptr[rs1_num] == 0) - *next_addr = RVC_EXTRACT_BTYPE_IMM(op_code) + pc; - else - *next_addr = pc + 2; - } else if (riscv_insn_is_c_bnez(op_code)) { - rs1_num = - decode_register_index_short(op_code, RVC_C1_RS1_OPOFF); - if (rs1_num && regs_ptr[rs1_num] != 0) - *next_addr = RVC_EXTRACT_BTYPE_IMM(op_code) + pc; - else - *next_addr = pc + 2; - } else { - *next_addr = pc + 2; - } - } else { - if ((op_code & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE) { - bool result = false; - long imm = RV_EXTRACT_BTYPE_IMM(op_code); - unsigned long rs1_val = 0, rs2_val = 0; - - rs1_num = decode_register_index(op_code, RVG_RS1_OPOFF); - rs2_num = decode_register_index(op_code, RVG_RS2_OPOFF); - if (rs1_num) - rs1_val = regs_ptr[rs1_num]; - if (rs2_num) - rs2_val = regs_ptr[rs2_num]; - - if (riscv_insn_is_beq(op_code)) - result = (rs1_val == rs2_val) ? true : false; - else if (riscv_insn_is_bne(op_code)) - result = (rs1_val != rs2_val) ? true : false; - else if (riscv_insn_is_blt(op_code)) - result = - ((long)rs1_val < - (long)rs2_val) ? true : false; - else if (riscv_insn_is_bge(op_code)) - result = - ((long)rs1_val >= - (long)rs2_val) ? true : false; - else if (riscv_insn_is_bltu(op_code)) - result = (rs1_val < rs2_val) ? true : false; - else if (riscv_insn_is_bgeu(op_code)) - result = (rs1_val >= rs2_val) ? true : false; - if (result) - *next_addr = imm + pc; - else - *next_addr = pc + 4; - } else if (riscv_insn_is_jal(op_code)) { - *next_addr = RV_EXTRACT_JTYPE_IMM(op_code) + pc; - } else if (riscv_insn_is_jalr(op_code)) { - rs1_num = decode_register_index(op_code, RVG_RS1_OPOFF); - if (rs1_num) - *next_addr = ((unsigned long *)regs)[rs1_num]; - *next_addr += RV_EXTRACT_ITYPE_IMM(op_code); - } else if (riscv_insn_is_sret(op_code)) { - *next_addr = pc; - } else { - *next_addr = pc + 4; - } - } - return 0; -} - static int do_single_step(struct pt_regs *regs) { /* Determine where the target instruction will send us to */ - unsigned long addr = 0; - int error = get_step_address(regs, &addr); + unsigned long addr, insn; + int error = get_insn(regs, regs->epc, &insn);
if (error) return error;
+ addr = get_step_address(regs, insn); + /* Store the op code in the stepped address */ - error = get_kernel_nofault(stepped_opcode, (void *)addr); + error = get_insn(regs, addr, stepped_opcode); if (error) return error;
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c index c0738d6c6498..6a9cfb0b664a 100644 --- a/arch/riscv/kernel/probes/kprobes.c +++ b/arch/riscv/kernel/probes/kprobes.c @@ -12,6 +12,7 @@ #include <asm/sections.h> #include <asm/cacheflush.h> #include <asm/bug.h> +#include <asm/insn.h> #include <asm/text-patching.h>
#include "decode-insn.h" diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 9c83848797a7..938a8b841f94 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -26,6 +26,7 @@ #include <asm/bug.h> #include <asm/cfi.h> #include <asm/csr.h> +#include <asm/insn.h> #include <asm/processor.h> #include <asm/ptrace.h> #include <asm/syscall.h> @@ -409,10 +410,10 @@ int is_valid_bugaddr(unsigned long pc) return 0; if (get_kernel_nofault(insn, (bug_insn_t *)pc)) return 0; - if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) + if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_GE_32) return (insn == __BUG_INSN_32); else - return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16); + return ((insn & RVC_MASK_C) == __BUG_INSN_16); } #endif /* CONFIG_GENERIC_BUG */
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c index 77c788660223..42a50e21b1d2 100644 --- a/arch/riscv/kernel/traps_misaligned.c +++ b/arch/riscv/kernel/traps_misaligned.c @@ -10,12 +10,13 @@ #include <linux/irq.h> #include <linux/stringify.h>
-#include <asm/processor.h> -#include <asm/ptrace.h> +#include <asm/cpufeature.h> #include <asm/csr.h> #include <asm/entry-common.h> #include <asm/hwprobe.h> -#include <asm/cpufeature.h> +#include <asm/insn.h> +#include <asm/processor.h> +#include <asm/ptrace.h> #include <asm/vector.h>
#define INSN_MATCH_LB 0x3 @@ -112,25 +113,22 @@ #define SH_RS2 20 #define SH_RS2C 2
-#define RV_X(x, s, n) (((x) >> (s)) & ((1 << (n)) - 1)) -#define RVC_LW_IMM(x) ((RV_X(x, 6, 1) << 2) | \ - (RV_X(x, 10, 3) << 3) | \ - (RV_X(x, 5, 1) << 6)) -#define RVC_LD_IMM(x) ((RV_X(x, 10, 3) << 3) | \ - (RV_X(x, 5, 2) << 6)) -#define RVC_LWSP_IMM(x) ((RV_X(x, 4, 3) << 2) | \ +#define RVC_LW_IMM(x) ((RV_X(x, 6, 0x1) << 2) | \ + (RV_X(x, 10, 0x7) << 3) | \ + (RV_X(x, 5, 0x1) << 6)) +#define RVC_LD_IMM(x) ((RV_X(x, 10, 0x7) << 3) | \ + (RV_X(x, 5, 0x3) << 6)) +#define RVC_LWSP_IMM(x) ((RV_X(x, 4, 0x7) << 2) | \ + (RV_X(x, 12, 0x1) << 5) | \ + (RV_X(x, 2, 0x3) << 6)) +#define RVC_LDSP_IMM(x) ((RV_X(x, 5, 0x3) << 3) | \ (RV_X(x, 12, 1) << 5) | \ - (RV_X(x, 2, 2) << 6)) -#define RVC_LDSP_IMM(x) ((RV_X(x, 5, 2) << 3) | \ - (RV_X(x, 12, 1) << 5) | \ - (RV_X(x, 2, 3) << 6)) -#define RVC_SWSP_IMM(x) ((RV_X(x, 9, 4) << 2) | \ - (RV_X(x, 7, 2) << 6)) -#define RVC_SDSP_IMM(x) ((RV_X(x, 10, 3) << 3) | \ - (RV_X(x, 7, 3) << 6)) -#define RVC_RS1S(insn) (8 + RV_X(insn, SH_RD, 3)) -#define RVC_RS2S(insn) (8 + RV_X(insn, SH_RS2C, 3)) -#define RVC_RS2(insn) RV_X(insn, SH_RS2C, 5) + (RV_X(x, 2, 0x7) << 6)) +#define RVC_SWSP_IMM(x) ((RV_X(x, 9, 0xf) << 2) | \ + (RV_X(x, 7, 0x3) << 6)) +#define RVC_SDSP_IMM(x) ((RV_X(x, 10, 0x7) << 3) | \ + (RV_X(x, 7, 0x7) << 6)) +#define RVC_RS2S(insn) (8 + RV_X(insn, SH_RS2C, 0x7))
#define SHIFT_RIGHT(x, y) \ ((y) < 0 ? ((x) << -(y)) : ((x) >> (y))) @@ -146,7 +144,6 @@
#define GET_RS1(insn, regs) (*REG_PTR(insn, SH_RS1, regs)) #define GET_RS2(insn, regs) (*REG_PTR(insn, SH_RS2, regs)) -#define GET_RS1S(insn, regs) (*REG_PTR(RVC_RS1S(insn), 0, regs)) #define GET_RS2S(insn, regs) (*REG_PTR(RVC_RS2S(insn), 0, regs)) #define GET_RS2C(insn, regs) (*REG_PTR(insn, SH_RS2C, regs)) #define GET_SP(regs) (*REG_PTR(2, 0, regs)) @@ -270,58 +267,6 @@ static unsigned long get_f32_rs(unsigned long insn, u8 fp_reg_offset, #define GET_F32_RS2C(insn, regs) (get_f32_rs(insn, 2, regs)) #define GET_F32_RS2S(insn, regs) (get_f32_rs(RVC_RS2S(insn), 0, regs))
-#define __read_insn(regs, insn, insn_addr, type) \ -({ \ - int __ret; \ - \ - if (user_mode(regs)) { \ - __ret = get_user(insn, (type __user *) insn_addr); \ - } else { \ - insn = *(type *)insn_addr; \ - __ret = 0; \ - } \ - \ - __ret; \ -}) - -static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn) -{ - ulong insn = 0; - - if (epc & 0x2) { - ulong tmp = 0; - - if (__read_insn(regs, insn, epc, u16)) - return -EFAULT; - /* __get_user() uses regular "lw" which sign extend the loaded - * value make sure to clear higher order bits in case we "or" it - * below with the upper 16 bits half. - */ - insn &= GENMASK(15, 0); - if ((insn & __INSN_LENGTH_MASK) != __INSN_LENGTH_32) { - *r_insn = insn; - return 0; - } - epc += sizeof(u16); - if (__read_insn(regs, tmp, epc, u16)) - return -EFAULT; - *r_insn = (tmp << 16) | insn; - - return 0; - } else { - if (__read_insn(regs, insn, epc, u32)) - return -EFAULT; - if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) { - *r_insn = insn; - return 0; - } - insn &= GENMASK(15, 0); - *r_insn = insn; - - return 0; - } -} - union reg_data { u8 data_bytes[8]; ulong data_ulong;
On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube jesse@rivosinc.com wrote:
Various parts of the kernel decode and read instruction from memory. Functions like get_insn, GET_INSN_LENGTH and riscv_insn_is_c are defined in multiple places. Consolidate these functions into the insn.h and the newly added insn.c.
Signed-off-by: Jesse Taube jesse@rivosinc.com
RFC -> V1:
- No change
V2 -> V1:
- No change
arch/riscv/include/asm/bug.h | 12 --- arch/riscv/include/asm/insn.h | 131 ++++++++++++++++++++++- arch/riscv/kernel/Makefile | 1 + arch/riscv/kernel/insn.c | 151 +++++++++++++++++++++++++++ arch/riscv/kernel/kgdb.c | 102 +----------------- arch/riscv/kernel/probes/kprobes.c | 1 + arch/riscv/kernel/traps.c | 5 +- arch/riscv/kernel/traps_misaligned.c | 93 ++++------------- 8 files changed, 309 insertions(+), 187 deletions(-) create mode 100644 arch/riscv/kernel/insn.c
diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h index 1aaea81fb141..a2777eb67ad1 100644 --- a/arch/riscv/include/asm/bug.h +++ b/arch/riscv/include/asm/bug.h @@ -12,21 +12,9 @@
#include <asm/asm.h>
-#define __INSN_LENGTH_MASK _UL(0x3) -#define __INSN_LENGTH_32 _UL(0x3) -#define __COMPRESSED_INSN_MASK _UL(0xffff)
#define __BUG_INSN_32 _UL(0x00100073) /* ebreak */ #define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */
-#define GET_INSN_LENGTH(insn) \ -({ \
unsigned long __len; \__len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? \4UL : 2UL; \__len; \-})
typedef u32 bug_insn_t;
#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h index 09fde95a5e8f..ba74e5b8262c 100644 --- a/arch/riscv/include/asm/insn.h +++ b/arch/riscv/include/asm/insn.h @@ -64,6 +64,7 @@ #define RVG_RS2_OPOFF 20 #define RVG_RD_OPOFF 7 #define RVG_RS1_MASK GENMASK(4, 0) +#define RVG_RS2_MASK GENMASK(4, 0) #define RVG_RD_MASK GENMASK(4, 0)
/* The bit field of immediate value in RVC J instruction */ @@ -121,17 +122,27 @@ #define RVC_C0_RS1_OPOFF 7 #define RVC_C0_RS2_OPOFF 2 #define RVC_C0_RD_OPOFF 2 +#define RVC_C0_RS1_MASK GENMASK(2, 0) +#define RVC_C0_RS2_MASK GENMASK(2, 0) +#define RVC_C0_RD_MASK GENMASK(2, 0) +#define RVC_C0_REG_OFFSET 8
/* The register offset in RVC op=C1 instruction */ #define RVC_C1_RS1_OPOFF 7 #define RVC_C1_RS2_OPOFF 2 #define RVC_C1_RD_OPOFF 7 +#define RVC_C1_RS1_MASK GENMASK(2, 0) +#define RVC_C1_RS2_MASK GENMASK(2, 0) +#define RVC_C1_RD_MASK GENMASK(2, 0) +#define RVC_C1_REG_OFFSET 8
/* The register offset in RVC op=C2 instruction */ #define RVC_C2_RS1_OPOFF 7 #define RVC_C2_RS2_OPOFF 2 #define RVC_C2_RD_OPOFF 7 #define RVC_C2_RS1_MASK GENMASK(4, 0) +#define RVC_C2_RS2_MASK GENMASK(4, 0) +#define RVC_C2_RD_MASK GENMASK(4, 0)
/* parts of opcode for RVG*/ #define RVG_OPCODE_FENCE 0x0f @@ -226,12 +237,26 @@ #define RVC_MASK_C_EBREAK 0xffff #define RVG_MASK_EBREAK 0xffffffff #define RVG_MASK_SRET 0xffffffff +#define RVC_MASK_C GENMASK(15, 0)
#define __INSN_LENGTH_MASK _UL(0x3) #define __INSN_LENGTH_GE_32 _UL(0x3) #define __INSN_OPCODE_MASK _UL(0x7F) #define __INSN_BRANCH_OPCODE _UL(RVG_OPCODE_BRANCH)
+#define GET_INSN_LENGTH(insn) \ +({ \
unsigned long __len; \__len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_GE_32) ? \4UL : 2UL; \__len; \+})
+static __always_inline bool riscv_insn_is_c(u32 code) +{
return (code & (__INSN_LENGTH_MASK)) != (__INSN_LENGTH_GE_32);+}
#define __RISCV_INSN_FUNCS(name, mask, val) \ static __always_inline bool riscv_insn_is_##name(u32 code) \ { \ @@ -260,7 +285,7 @@ __RISCV_INSN_FUNCS(c_bnez, RVC_MASK_C_BNEZ, RVC_MATCH_C_BNEZ) __RISCV_INSN_FUNCS(c_ebreak, RVC_MASK_C_EBREAK, RVC_MATCH_C_EBREAK) __RISCV_INSN_FUNCS(ebreak, RVG_MASK_EBREAK, RVG_MATCH_EBREAK) __RISCV_INSN_FUNCS(sret, RVG_MASK_SRET, RVG_MATCH_SRET) -__RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE); +__RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE)
/* special case to catch _any_ system instruction */ static __always_inline bool riscv_insn_is_system(u32 code) @@ -295,6 +320,10 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code) ({typeof(x) x_ = (x); \ (RV_X(x_, RVG_RS1_OPOFF, RVG_RS1_MASK)); })
+#define RV_EXTRACT_RS2_REG(x) \
({typeof(x) x_ = (x); \(RV_X(x_, RVG_RS2_OPOFF, RVG_RS2_MASK)); })#define RV_EXTRACT_RD_REG(x) \ ({typeof(x) x_ = (x); \ (RV_X(x_, RVG_RD_OPOFF, RVG_RD_MASK)); }) @@ -322,9 +351,41 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code) (RV_X(x_, RV_B_IMM_11_OPOFF, RV_B_IMM_11_MASK) << RV_B_IMM_11_OFF) | \ (RV_IMM_SIGN(x_) << RV_B_IMM_SIGN_OFF); })
+#define RVC_EXTRACT_C0_RS1_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C0_RS1_OPOFF, RVC_C0_RS1_MASK)); })+#define RVC_EXTRACT_C0_RS2_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C0_RS2_OPOFF, RVC_C0_RS2_MASK)); })+#define RVC_EXTRACT_C0_RD_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C0_RD_OPOFF, RVC_C0_RD_MASK)); })+#define RVC_EXTRACT_C1_RS1_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C1_RS1_OPOFF, RVC_C1_RS1_MASK)); })+#define RVC_EXTRACT_C1_RS2_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C1_RS2_OPOFF, RVC_C1_RS2_MASK)); })+#define RVC_EXTRACT_C1_RD_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C1_RD_OPOFF, RVC_C1_RD_MASK)); })#define RVC_EXTRACT_C2_RS1_REG(x) \ ({typeof(x) x_ = (x); \
(RV_X(x_, RVC_C2_RS1_OPOFF, RVC_C2_RS1_MASK)); })
(RVC_X(x_, RVC_C2_RS1_OPOFF, RVC_C2_RS1_MASK)); })+#define RVC_EXTRACT_C2_RS2_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C2_RS2_OPOFF, RVC_C2_RS2_MASK)); })+#define RVC_EXTRACT_C2_RD_REG(x) \
({typeof(x) x_ = (x); \(RVC_X(x_, RVC_C2_RD_OPOFF, RVC_C2_RD_MASK)); })#define RVC_EXTRACT_JTYPE_IMM(x) \ ({typeof(x) x_ = (x); \ @@ -354,6 +415,66 @@ static __always_inline bool riscv_insn_is_c_jalr(u32 code)
#define RVV_EXRACT_VL_VS_WIDTH(x) RVFDQ_EXTRACT_FL_FS_WIDTH(x)
+/*
- Get the rs1 register number from RV or RVC instruction.
- @insn: instruction to process
- Return: rs1 register
- */
+static inline unsigned int riscv_insn_extract_rs1_reg(u32 insn) +{
switch (RVC_INSN_OPCODE_MASK & insn) {case RVC_OPCODE_C0:return RVC_EXTRACT_C0_RS1_REG(insn) + RVC_C0_REG_OFFSET;case RVC_OPCODE_C1:return RVC_EXTRACT_C1_RS1_REG(insn) + RVC_C1_REG_OFFSET;case RVC_OPCODE_C2:return RVC_EXTRACT_C2_RS1_REG(insn);default:return RV_EXTRACT_RS1_REG(insn);}+}
+/*
- Get the rs2 register number from RV or RVC instruction.
- @insn: instruction to process
- Return: rs2 register
- */
+static inline unsigned int riscv_insn_extract_rs2_reg(u32 insn) +{
switch (RVC_INSN_OPCODE_MASK & insn) {case RVC_OPCODE_C0:return RVC_EXTRACT_C0_RS2_REG(insn) + RVC_C0_REG_OFFSET;case RVC_OPCODE_C1:return RVC_EXTRACT_C1_RS2_REG(insn) + RVC_C1_REG_OFFSET;case RVC_OPCODE_C2:return RVC_EXTRACT_C2_RS2_REG(insn);default:return RV_EXTRACT_RS2_REG(insn);}+}
+/*
- Get the rd register number from RV or RVC instruction.
- @insn: instruction to process
- Return: rd register
- */
+static inline unsigned int riscv_insn_extract_rd_reg(u32 insn) +{
switch (RVC_INSN_OPCODE_MASK & insn) {case RVC_OPCODE_C0:return RVC_EXTRACT_C0_RD_REG(insn) + RVC_C0_REG_OFFSET;case RVC_OPCODE_C1:return RVC_EXTRACT_C1_RD_REG(insn) + RVC_C1_REG_OFFSET;case RVC_OPCODE_C2:return RVC_EXTRACT_C2_RD_REG(insn);default:return RV_EXTRACT_RD_REG(insn);}+}
/*
- Get the immediate from a J-type instruction.
@@ -428,4 +549,10 @@ static inline void riscv_insn_insert_utype_itype_imm(u32 *utype_insn, u32 *itype *utype_insn |= (imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1); *itype_insn |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF); }
+#include <asm/ptrace.h>
+int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn); +unsigned long get_step_address(struct pt_regs *regs, u32 code);
#endif /* _ASM_RISCV_INSN_H */ diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index f7480c9c6f8d..4f719b09e5ad 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -51,6 +51,7 @@ obj-$(CONFIG_RISCV_ALTERNATIVE) += alternative.o obj-y += cpu.o obj-y += cpufeature.o obj-y += entry.o +obj-y += insn.o obj-y += irq.o obj-y += process.o obj-y += ptrace.o diff --git a/arch/riscv/kernel/insn.c b/arch/riscv/kernel/insn.c new file mode 100644 index 000000000000..dd2a6ef9fd25 --- /dev/null +++ b/arch/riscv/kernel/insn.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright 2025 Rivos, Inc
- */
+#include <asm/insn.h> +#include <asm/ptrace.h> +#include <asm/uaccess.h>
+#define __read_insn(regs, insn, insn_addr, type) \ +({ \
int __ret; \\if (user_mode(regs)) { \__ret = get_user(insn, (type __user *) insn_addr); \} else { \insn = *(type *)insn_addr; \__ret = 0; \} \\__ret; \+})
+/*
- Update a set of two instructions (U-type + I-type) with an immediate value.
- Used for example in auipc+jalrs pairs the U-type instructions contains
- a 20bit upper immediate representing bits[31:12], while the I-type
- instruction contains a 12bit immediate representing bits[11:0].
- This also takes into account that both separate immediates are
- considered as signed values, so if the I-type immediate becomes
- negative (BIT(11) set) the U-type part gets adjusted.
- @regs: pointer to the utype instruction of the pair
- @epc: pointer to the itype instruction of the pair
- @r_insn: the immediate to insert into the two instructions
- Return: combined immediate
- */
+int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn) +{
ulong insn = 0;if (epc & 0x2) {ulong tmp = 0;if (__read_insn(regs, insn, epc, u16))return -EFAULT;/* __get_user() uses regular "lw" which sign extend the loaded* value make sure to clear higher order bits in case we "or" it* below with the upper 16 bits half.*/insn &= RVC_MASK_C;if (riscv_insn_is_c(insn)) {*r_insn = insn;return 0;}epc += sizeof(u16);if (__read_insn(regs, tmp, epc, u16))return -EFAULT;*r_insn = (tmp << 16) | insn;return 0;} else {if (__read_insn(regs, insn, epc, u32))return -EFAULT;if (!riscv_insn_is_c(insn)) {*r_insn = insn;return 0;}insn &= RVC_MASK_C;*r_insn = insn;return 0;}+}
+/* Calculate the new address for after a step */ +unsigned long get_step_address(struct pt_regs *regs, u32 code) +{
unsigned long pc = regs->epc;unsigned int rs1_num, rs2_num;if ((code & __INSN_LENGTH_MASK) != __INSN_LENGTH_GE_32) {if (riscv_insn_is_c_jalr(code) ||riscv_insn_is_c_jr(code)) {rs1_num = riscv_insn_extract_rs1_reg(code);return regs_get_register(regs, rs1_num);} else if (riscv_insn_is_c_j(code) ||riscv_insn_is_c_jal(code)) {return RVC_EXTRACT_JTYPE_IMM(code) + pc;} else if (riscv_insn_is_c_beqz(code)) {rs1_num = riscv_insn_extract_rs1_reg(code);if (!rs1_num || regs_get_register(regs, rs1_num) == 0)return RVC_EXTRACT_BTYPE_IMM(code) + pc;elsereturn pc + 2;} else if (riscv_insn_is_c_bnez(code)) {rs1_num = riscv_insn_extract_rs1_reg(RVC_C1_RS1_OPOFF);if (rs1_num && regs_get_register(regs, rs1_num) != 0)return RVC_EXTRACT_BTYPE_IMM(code) + pc;elsereturn pc + 2;} else {return pc + 2;}} else {if ((code & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE) {bool result = false;long imm = RV_EXTRACT_BTYPE_IMM(code);unsigned long rs1_val = 0, rs2_val = 0;rs1_num = riscv_insn_extract_rs1_reg(code);rs2_num = riscv_insn_extract_rs2_reg(code);if (rs1_num)rs1_val = regs_get_register(regs, rs1_num);if (rs2_num)rs2_val = regs_get_register(regs, rs2_num);if (riscv_insn_is_beq(code))result = (rs1_val == rs2_val) ? true : false;else if (riscv_insn_is_bne(code))result = (rs1_val != rs2_val) ? true : false;else if (riscv_insn_is_blt(code))result =((long)rs1_val <(long)rs2_val) ? true : false;else if (riscv_insn_is_bge(code))result =((long)rs1_val >=(long)rs2_val) ? true : false;else if (riscv_insn_is_bltu(code))result = (rs1_val < rs2_val) ? true : false;else if (riscv_insn_is_bgeu(code))result = (rs1_val >= rs2_val) ? true : false;if (result)return imm + pc;elsereturn pc + 4;} else if (riscv_insn_is_jal(code)) {return RV_EXTRACT_JTYPE_IMM(code) + pc;} else if (riscv_insn_is_jalr(code)) {rs1_num = riscv_insn_extract_rs1_reg(code);return RV_EXTRACT_ITYPE_IMM(code) +(rs1_num ? regs_get_register(regs, rs1_num) : 0);} else if (riscv_insn_is_sret(code)) {return pc;} else {return pc + 4;}}+} diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c index 9f3db3503dab..aafc1424fc81 100644 --- a/arch/riscv/kernel/kgdb.c +++ b/arch/riscv/kernel/kgdb.c @@ -23,111 +23,19 @@ enum { static unsigned long stepped_address; static unsigned int stepped_opcode;
-static int decode_register_index(unsigned long opcode, int offset) -{
return (opcode >> offset) & 0x1F;-}
-static int decode_register_index_short(unsigned long opcode, int offset) -{
return ((opcode >> offset) & 0x7) + 8;-}
-/* Calculate the new address for after a step */ -static int get_step_address(struct pt_regs *regs, unsigned long *next_addr) -{
unsigned long pc = regs->epc;unsigned long *regs_ptr = (unsigned long *)regs;unsigned int rs1_num, rs2_num;int op_code;if (get_kernel_nofault(op_code, (void *)pc))return -EINVAL;if ((op_code & __INSN_LENGTH_MASK) != __INSN_LENGTH_GE_32) {if (riscv_insn_is_c_jalr(op_code) ||riscv_insn_is_c_jr(op_code)) {rs1_num = decode_register_index(op_code, RVC_C2_RS1_OPOFF);*next_addr = regs_ptr[rs1_num];} else if (riscv_insn_is_c_j(op_code) ||riscv_insn_is_c_jal(op_code)) {*next_addr = RVC_EXTRACT_JTYPE_IMM(op_code) + pc;} else if (riscv_insn_is_c_beqz(op_code)) {rs1_num = decode_register_index_short(op_code,RVC_C1_RS1_OPOFF);if (!rs1_num || regs_ptr[rs1_num] == 0)*next_addr = RVC_EXTRACT_BTYPE_IMM(op_code) + pc;else*next_addr = pc + 2;} else if (riscv_insn_is_c_bnez(op_code)) {rs1_num =decode_register_index_short(op_code, RVC_C1_RS1_OPOFF);if (rs1_num && regs_ptr[rs1_num] != 0)*next_addr = RVC_EXTRACT_BTYPE_IMM(op_code) + pc;else*next_addr = pc + 2;} else {*next_addr = pc + 2;}} else {if ((op_code & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE) {bool result = false;long imm = RV_EXTRACT_BTYPE_IMM(op_code);unsigned long rs1_val = 0, rs2_val = 0;rs1_num = decode_register_index(op_code, RVG_RS1_OPOFF);rs2_num = decode_register_index(op_code, RVG_RS2_OPOFF);if (rs1_num)rs1_val = regs_ptr[rs1_num];if (rs2_num)rs2_val = regs_ptr[rs2_num];if (riscv_insn_is_beq(op_code))result = (rs1_val == rs2_val) ? true : false;else if (riscv_insn_is_bne(op_code))result = (rs1_val != rs2_val) ? true : false;else if (riscv_insn_is_blt(op_code))result =((long)rs1_val <(long)rs2_val) ? true : false;else if (riscv_insn_is_bge(op_code))result =((long)rs1_val >=(long)rs2_val) ? true : false;else if (riscv_insn_is_bltu(op_code))result = (rs1_val < rs2_val) ? true : false;else if (riscv_insn_is_bgeu(op_code))result = (rs1_val >= rs2_val) ? true : false;if (result)*next_addr = imm + pc;else*next_addr = pc + 4;} else if (riscv_insn_is_jal(op_code)) {*next_addr = RV_EXTRACT_JTYPE_IMM(op_code) + pc;} else if (riscv_insn_is_jalr(op_code)) {rs1_num = decode_register_index(op_code, RVG_RS1_OPOFF);if (rs1_num)*next_addr = ((unsigned long *)regs)[rs1_num];*next_addr += RV_EXTRACT_ITYPE_IMM(op_code);} else if (riscv_insn_is_sret(op_code)) {*next_addr = pc;} else {*next_addr = pc + 4;}}return 0;-}
static int do_single_step(struct pt_regs *regs) { /* Determine where the target instruction will send us to */
unsigned long addr = 0;int error = get_step_address(regs, &addr);
unsigned long addr, insn;int error = get_insn(regs, regs->epc, &insn); if (error) return error;addr = get_step_address(regs, insn);/* Store the op code in the stepped address */
error = get_kernel_nofault(stepped_opcode, (void *)addr);
error = get_insn(regs, addr, stepped_opcode);
This line gives following compile error:
arch/riscv/kernel/kgdb.c: In function 'do_single_step': arch/riscv/kernel/kgdb.c:38:38: error: passing argument 3 of 'get_insn' makes pointer from integer without a cast [-Wint-conversion] 38 | error = get_insn(regs, addr, stepped_opcode); | ^~~~~~~~~~~~~~ | | | unsigned int In file included from arch/riscv/kernel/kgdb.c:14: ./arch/riscv/include/asm/insn.h:555:54: note: expected 'ulong *' {aka 'long unsigned int *'} but argument is of type 'unsigned int' 555 | int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn); | ~~~~~~~^~~~~~
If you are touching some source then at least compile test it.
Regards, Anup
From: Himanshu Chauhan hchauhan@ventanamicro.com
Debug trigger extension is a proposed SBI extension to support native debugging in S-mode and VS-mode.
The proposal for the extension can be found at: https://lists.riscv.org/g/sig-hypervisors/message/361
This patch adds the extension and the function IDs defined by the extension.
Signed-off-by: Himanshu Chauhan hchauhan@ventanamicro.com --- RFC -> V1: - No change V1 -> V2: - No change --- arch/riscv/include/asm/sbi.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h index 3d250824178b..be2ca8e8a49e 100644 --- a/arch/riscv/include/asm/sbi.h +++ b/arch/riscv/include/asm/sbi.h @@ -35,6 +35,7 @@ enum sbi_ext_id { SBI_EXT_DBCN = 0x4442434E, SBI_EXT_STA = 0x535441, SBI_EXT_NACL = 0x4E41434C, + SBI_EXT_DBTR = 0x44425452,
/* Experimentals extensions must lie within this range */ SBI_EXT_EXPERIMENTAL_START = 0x08000000, @@ -402,6 +403,34 @@ enum sbi_ext_nacl_feature { #define SBI_NACL_SHMEM_SRET_X(__i) ((__riscv_xlen / 8) * (__i)) #define SBI_NACL_SHMEM_SRET_X_LAST 31
+/* SBI debug triggers function IDs */ +enum sbi_ext_dbtr_fid { + SBI_EXT_DBTR_NUM_TRIGGERS = 0, + SBI_EXT_DBTR_SETUP_SHMEM, + SBI_EXT_DBTR_TRIG_READ, + SBI_EXT_DBTR_TRIG_INSTALL, + SBI_EXT_DBTR_TRIG_UPDATE, + SBI_EXT_DBTR_TRIG_UNINSTALL, + SBI_EXT_DBTR_TRIG_ENABLE, + SBI_EXT_DBTR_TRIG_DISABLE, +}; + +struct sbi_dbtr_data_msg { + unsigned long tstate; + unsigned long tdata1; + unsigned long tdata2; + unsigned long tdata3; +}; + +struct sbi_dbtr_id_msg { + unsigned long idx; +}; + +union sbi_dbtr_shmem_entry { + struct sbi_dbtr_data_msg data; + struct sbi_dbtr_id_msg id; +}; + /* SBI spec version fields */ #define SBI_SPEC_VERSION_DEFAULT 0x1 #define SBI_SPEC_VERSION_MAJOR_SHIFT 24
get_insn_nofault uses get_insn with pagefaults dissabled, allowing it to be called in an atomic context.
Signed-off-by: Jesse Taube jesse@rivosinc.com --- Unsure if copy_from_kernel_nofault is an acceptable replacement for direct dereference in __read_insn.
RFC -> V1: - Add new function instead of using copy_from_user_nofault V1 -> V2: - No change --- arch/riscv/include/asm/insn.h | 1 + arch/riscv/kernel/insn.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h index ba74e5b8262c..f87e0a48f786 100644 --- a/arch/riscv/include/asm/insn.h +++ b/arch/riscv/include/asm/insn.h @@ -553,6 +553,7 @@ static inline void riscv_insn_insert_utype_itype_imm(u32 *utype_insn, u32 *itype #include <asm/ptrace.h>
int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn); +int get_insn_nofault(struct pt_regs *regs, ulong epc, ulong *r_insn); unsigned long get_step_address(struct pt_regs *regs, u32 code);
#endif /* _ASM_RISCV_INSN_H */ diff --git a/arch/riscv/kernel/insn.c b/arch/riscv/kernel/insn.c index dd2a6ef9fd25..c8f77c0093c9 100644 --- a/arch/riscv/kernel/insn.c +++ b/arch/riscv/kernel/insn.c @@ -2,6 +2,9 @@ /* * Copyright 2025 Rivos, Inc */ + +#include <linux/uaccess.h> + #include <asm/insn.h> #include <asm/ptrace.h> #include <asm/uaccess.h> @@ -74,6 +77,17 @@ int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn) } }
+int get_insn_nofault(struct pt_regs *regs, ulong epc, ulong *r_insn) +{ + int ret; + + pagefault_disable(); + ret = get_insn(regs, epc, r_insn); + pagefault_enable(); + + return ret; +} + /* Calculate the new address for after a step */ unsigned long get_step_address(struct pt_regs *regs, u32 code) {
From: Himanshu Chauhan hchauhan@ventanamicro.com
RISC-V hardware breakpoint framework is built on top of perf subsystem and uses SBI debug trigger extension to install/uninstall/update/enable/disable hardware triggers as specified in Sdtrig ISA extension.
Signed-off-by: Himanshu Chauhan hchauhan@ventanamicro.com Signed-off-by: Jesse Taube jesse@rivosinc.com --- RFC -> V1: - Add dbtr_mode to rv_init_mcontrol(6)_trigger - Add select HAVE_MIXED_BREAKPOINTS_REGS - Add TDATA1_MCTRL_SZ and TDATA1_MCTRL6_SZ - Capitalize F in Fallback comment - Fix in_callback code to allow multiple breakpoints - Move perf_bp_event above setup_singlestep to save the correct state - Use sbi_err_map_linux_errno for arch_smp_teardown/setup_sbi_shmem V1 -> V2: - No change --- arch/riscv/Kconfig | 2 + arch/riscv/include/asm/hw_breakpoint.h | 59 +++ arch/riscv/include/asm/kdebug.h | 3 +- arch/riscv/include/asm/sbi.h | 4 +- arch/riscv/kernel/Makefile | 1 + arch/riscv/kernel/hw_breakpoint.c | 614 +++++++++++++++++++++++++ arch/riscv/kernel/traps.c | 6 + 7 files changed, 687 insertions(+), 2 deletions(-) create mode 100644 arch/riscv/include/asm/hw_breakpoint.h create mode 100644 arch/riscv/kernel/hw_breakpoint.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index bbec87b79309..fd8b62cdc6f5 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -163,6 +163,7 @@ config RISCV select HAVE_FUNCTION_ERROR_INJECTION select HAVE_GCC_PLUGINS select HAVE_GENERIC_VDSO if MMU && 64BIT + select HAVE_HW_BREAKPOINT if PERF_EVENTS && RISCV_SBI select HAVE_IRQ_TIME_ACCOUNTING select HAVE_KERNEL_BZIP2 if !XIP_KERNEL && !EFI_ZBOOT select HAVE_KERNEL_GZIP if !XIP_KERNEL && !EFI_ZBOOT @@ -176,6 +177,7 @@ config RISCV select HAVE_KRETPROBES if !XIP_KERNEL # https://github.com/ClangBuiltLinux/linux/issues/1881 select HAVE_LD_DEAD_CODE_DATA_ELIMINATION if !LD_IS_LLD + select HAVE_MIXED_BREAKPOINTS_REGS select HAVE_MOVE_PMD select HAVE_MOVE_PUD select HAVE_PAGE_SIZE_4KB diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h new file mode 100644 index 000000000000..cde6688b91d2 --- /dev/null +++ b/arch/riscv/include/asm/hw_breakpoint.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 Ventana Micro Systems Inc. + */ + +#ifndef __RISCV_HW_BREAKPOINT_H +#define __RISCV_HW_BREAKPOINT_H + +struct task_struct; + +#ifdef CONFIG_HAVE_HW_BREAKPOINT + +#include <uapi/linux/hw_breakpoint.h> + +#if __riscv_xlen == 64 +#define cpu_to_le cpu_to_le64 +#define le_to_cpu le64_to_cpu +#elif __riscv_xlen == 32 +#define cpu_to_le cpu_to_le32 +#define le_to_cpu le32_to_cpu +#else +#error "Unexpected __riscv_xlen" +#endif + +struct arch_hw_breakpoint { + unsigned long address; + unsigned long len; + + /* Callback info */ + unsigned long next_addr; + bool in_callback; + + /* Trigger configuration data */ + unsigned long tdata1; + unsigned long tdata2; + unsigned long tdata3; +}; + +/* Maximum number of hardware breakpoints supported */ +#define RV_MAX_TRIGGERS 32 + +struct perf_event_attr; +struct notifier_block; +struct perf_event; +struct pt_regs; + +int hw_breakpoint_slots(int type); +int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw); +int hw_breakpoint_arch_parse(struct perf_event *bp, + const struct perf_event_attr *attr, + struct arch_hw_breakpoint *hw); +int hw_breakpoint_exceptions_notify(struct notifier_block *unused, + unsigned long val, void *data); +int arch_install_hw_breakpoint(struct perf_event *bp); +void arch_uninstall_hw_breakpoint(struct perf_event *bp); +void hw_breakpoint_pmu_read(struct perf_event *bp); + +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ +#endif /* __RISCV_HW_BREAKPOINT_H */ diff --git a/arch/riscv/include/asm/kdebug.h b/arch/riscv/include/asm/kdebug.h index 85ac00411f6e..53e989781aa1 100644 --- a/arch/riscv/include/asm/kdebug.h +++ b/arch/riscv/include/asm/kdebug.h @@ -6,7 +6,8 @@ enum die_val { DIE_UNUSED, DIE_TRAP, - DIE_OOPS + DIE_OOPS, + DIE_DEBUG };
#endif diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h index be2ca8e8a49e..64fa7a82aa45 100644 --- a/arch/riscv/include/asm/sbi.h +++ b/arch/riscv/include/asm/sbi.h @@ -282,7 +282,9 @@ struct sbi_sta_struct { u8 pad[47]; } __packed;
-#define SBI_SHMEM_DISABLE -1 +#define SBI_SHMEM_DISABLE (-1UL) +#define SBI_SHMEM_LO(pa) ((unsigned long)lower_32_bits(pa)) +#define SBI_SHMEM_HI(pa) ((unsigned long)upper_32_bits(pa))
enum sbi_ext_nacl_fid { SBI_EXT_NACL_PROBE_FEATURE = 0x0, diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index 4f719b09e5ad..3e72505734bd 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -99,6 +99,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += mcount-dyn.o
obj-$(CONFIG_PERF_EVENTS) += perf_callchain.o obj-$(CONFIG_HAVE_PERF_REGS) += perf_regs.o +obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o obj-$(CONFIG_RISCV_SBI) += sbi.o sbi_ecall.o ifeq ($(CONFIG_RISCV_SBI), y) obj-$(CONFIG_SMP) += sbi-ipi.o diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c new file mode 100644 index 000000000000..3f96e744a711 --- /dev/null +++ b/arch/riscv/kernel/hw_breakpoint.c @@ -0,0 +1,614 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 Ventana Micro Systems Inc. + */ + +#include <linux/hw_breakpoint.h> +#include <linux/perf_event.h> +#include <linux/spinlock.h> +#include <linux/percpu.h> +#include <linux/kdebug.h> +#include <linux/bitops.h> +#include <linux/bitfield.h> +#include <linux/cpu.h> +#include <linux/cpuhotplug.h> + +#include <asm/insn.h> +#include <asm/sbi.h> + +#define DBTR_TDATA1_TYPE_SHIFT (__riscv_xlen - 4) +#define DBTR_TDATA1_DMODE BIT_UL(__riscv_xlen - 5) + +#define DBTR_TDATA1_TYPE_MCONTROL (2UL << DBTR_TDATA1_TYPE_SHIFT) +#define DBTR_TDATA1_TYPE_MCONTROL6 (6UL << DBTR_TDATA1_TYPE_SHIFT) + +#define DBTR_TDATA1_MCONTROL6_LOAD BIT(0) +#define DBTR_TDATA1_MCONTROL6_STORE BIT(1) +#define DBTR_TDATA1_MCONTROL6_EXECUTE BIT(2) +#define DBTR_TDATA1_MCONTROL6_U BIT(3) +#define DBTR_TDATA1_MCONTROL6_S BIT(4) +#define DBTR_TDATA1_MCONTROL6_M BIT(6) +#define DBTR_TDATA1_MCONTROL6_SIZE_FIELD GENMASK(18, 16) +#define DBTR_TDATA1_MCONTROL6_SELECT BIT(21) +#define DBTR_TDATA1_MCONTROL6_VU BIT(23) +#define DBTR_TDATA1_MCONTROL6_VS BIT(24) + +#define DBTR_TDATA1_MCONTROL6_SIZE_8BIT 1 +#define DBTR_TDATA1_MCONTROL6_SIZE_16BIT 2 +#define DBTR_TDATA1_MCONTROL6_SIZE_32BIT 3 +#define DBTR_TDATA1_MCONTROL6_SIZE_64BIT 5 + +#define TDATA1_MCTRL6_SZ(sz) \ + FIELD_PREP(DBTR_TDATA1_MCONTROL6_SIZE_FIELD, sz) + +#define DBTR_TDATA1_MCONTROL_LOAD BIT(0) +#define DBTR_TDATA1_MCONTROL_STORE BIT(1) +#define DBTR_TDATA1_MCONTROL_EXECUTE BIT(2) +#define DBTR_TDATA1_MCONTROL_U BIT(3) +#define DBTR_TDATA1_MCONTROL_S BIT(4) +#define DBTR_TDATA1_MCONTROL_M BIT(6) +#define DBTR_TDATA1_MCONTROL_SIZELO_FIELD GENMASK(17, 16) +#define DBTR_TDATA1_MCONTROL_SELECT BIT(19) +#define DBTR_TDATA1_MCONTROL_SIZEHI_FIELD GENMASK(22, 21) + +#define DBTR_TDATA1_MCONTROL_SIZELO_8BIT 1 +#define DBTR_TDATA1_MCONTROL_SIZELO_16BIT 2 +#define DBTR_TDATA1_MCONTROL_SIZELO_32BIT 3 +/* value of 5 split across HI and LO */ +#define DBTR_TDATA1_MCONTROL_SIZELO_64BIT 1 +#define DBTR_TDATA1_MCONTROL_SIZEHI_64BIT 1 + +#define TDATA1_MCTRL_SZ(lo, hi) \ + (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \ + FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi)) + +enum dbtr_mode { + DBTR_MODE_U = 0, + DBTR_MODE_S, + DBTR_MODE_VS, + DBTR_MODE_VU, +}; + +/* Registered per-cpu bp/wp */ +static DEFINE_PER_CPU(struct perf_event *, pcpu_hw_bp_events[RV_MAX_TRIGGERS]); +static DEFINE_PER_CPU(unsigned long, ecall_lock_flags); +static DEFINE_PER_CPU(raw_spinlock_t, ecall_lock); + +/* Per-cpu shared memory between S and M mode */ +static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem); + +/* number of debug triggers on this cpu . */ +static int dbtr_total_num __ro_after_init; +static unsigned long dbtr_type __ro_after_init; +static unsigned long dbtr_init __ro_after_init; + +static int arch_smp_setup_sbi_shmem(unsigned int cpu) +{ + union sbi_dbtr_shmem_entry *dbtr_shmem; + unsigned long shmem_pa; + struct sbiret ret; + int rc; + + dbtr_shmem = per_cpu_ptr(&sbi_dbtr_shmem, cpu); + if (!dbtr_shmem) { + pr_err("Invalid per-cpu shared memory for debug triggers\n"); + return -ENODEV; + } + + shmem_pa = virt_to_phys(dbtr_shmem); + + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM, + SBI_SHMEM_LO(shmem_pa), SBI_SHMEM_HI(shmem_pa), 0, 0, 0, 0); + if (ret.error) { + pr_warn("%s: failed to setup shared memory. error: %ld\n", __func__, ret.error); + return sbi_err_map_linux_errno(ret.error); + } + + pr_debug("CPU %d: HW Breakpoint shared memory registered.\n", cpu); + + return rc; +} + +static int arch_smp_teardown_sbi_shmem(unsigned int cpu) +{ + struct sbiret ret; + + /* Disable shared memory */ + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM, + SBI_SHMEM_DISABLE, SBI_SHMEM_DISABLE, 0, 0, 0, 0); + if (ret.error) { + pr_warn("%s: failed to teardown shared memory. error: %ld\n", __func__, ret.error); + return sbi_err_map_linux_errno(ret.error); + } + + pr_debug("CPU %d: HW Breakpoint shared memory disabled.\n", cpu); + + return 0; +} + +static void init_sbi_dbtr(void) +{ + struct sbiret ret; + + /* + * Called by hw_breakpoint_slots and arch_hw_breakpoint_init. + * Only proceed if this is the first CPU to reach this code. + */ + if (test_and_set_bit(0, &dbtr_init)) + return; + + if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) { + pr_debug("%s: SBI_EXT_DBTR is not supported\n", __func__); + dbtr_total_num = 0; + return; + } + + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, + DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0); + if (ret.error) { + pr_warn("%s: failed to detect mcontrol6 triggers. error: %ld.\n", + __func__, ret.error); + } else if (!ret.value) { + pr_warn("%s: No mcontrol6 triggers available.\n", __func__); + } else { + dbtr_total_num = ret.value; + dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6; + return; + } + + /* Fallback to legacy mcontrol triggers if mcontrol6 is not available */ + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, + DBTR_TDATA1_TYPE_MCONTROL, 0, 0, 0, 0, 0); + if (ret.error) { + pr_warn("%s: failed to detect mcontrol triggers. error: %ld.\n", + __func__, ret.error); + } else if (!ret.value) { + pr_err("%s: No mcontrol triggers available.\n", __func__); + dbtr_total_num = 0; + } else { + dbtr_total_num = ret.value; + dbtr_type = DBTR_TDATA1_TYPE_MCONTROL; + } +} + +int hw_breakpoint_slots(int type) +{ + /* + * We can be called early, so don't rely on + * static variables being initialised. + */ + init_sbi_dbtr(); + + return dbtr_total_num; +} + +int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw) +{ + unsigned int len; + unsigned long va; + + va = hw->address; + len = hw->len; + + return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE); +} + +static int rv_init_mcontrol_trigger(const struct perf_event_attr *attr, + struct arch_hw_breakpoint *hw, enum dbtr_mode mode) +{ + unsigned long tdata1 = DBTR_TDATA1_TYPE_MCONTROL; + + switch (attr->bp_type) { + case HW_BREAKPOINT_X: + tdata1 |= DBTR_TDATA1_MCONTROL_EXECUTE; + break; + case HW_BREAKPOINT_R: + tdata1 |= DBTR_TDATA1_MCONTROL_LOAD; + break; + case HW_BREAKPOINT_W: + tdata1 |= DBTR_TDATA1_MCONTROL_STORE; + break; + case HW_BREAKPOINT_RW: + tdata1 |= DBTR_TDATA1_MCONTROL_STORE | DBTR_TDATA1_MCONTROL_LOAD; + break; + default: + return -EINVAL; + } + + switch (attr->bp_len) { + case HW_BREAKPOINT_LEN_1: + hw->len = 1; + tdata1 |= TDATA1_MCTRL_SZ(DBTR_TDATA1_MCONTROL_SIZELO_8BIT, 0); + break; + case HW_BREAKPOINT_LEN_2: + hw->len = 2; + tdata1 |= TDATA1_MCTRL_SZ(DBTR_TDATA1_MCONTROL_SIZELO_16BIT, 0); + break; + case HW_BREAKPOINT_LEN_4: + hw->len = 4; + tdata1 |= TDATA1_MCTRL_SZ(DBTR_TDATA1_MCONTROL_SIZELO_32BIT, 0); + break; +#if __riscv_xlen >= 64 + case HW_BREAKPOINT_LEN_8: + hw->len = 8; + tdata1 |= TDATA1_MCTRL_SZ(DBTR_TDATA1_MCONTROL_SIZELO_64BIT, + DBTR_TDATA1_MCONTROL_SIZEHI_64BIT); + break; +#endif + default: + return -EINVAL; + } + + switch (mode) { + case DBTR_MODE_U: + tdata1 |= DBTR_TDATA1_MCONTROL_U; + break; + case DBTR_MODE_S: + tdata1 |= DBTR_TDATA1_MCONTROL_S; + break; + default: + return -EINVAL; + } + + hw->tdata1 = tdata1; + + return 0; +} + +static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr, + struct arch_hw_breakpoint *hw, enum dbtr_mode mode) +{ + unsigned long tdata1 = DBTR_TDATA1_TYPE_MCONTROL; + + switch (attr->bp_type) { + case HW_BREAKPOINT_X: + tdata1 |= DBTR_TDATA1_MCONTROL6_EXECUTE; + break; + case HW_BREAKPOINT_R: + tdata1 |= DBTR_TDATA1_MCONTROL6_LOAD; + break; + case HW_BREAKPOINT_W: + tdata1 |= DBTR_TDATA1_MCONTROL6_STORE; + break; + case HW_BREAKPOINT_RW: + tdata1 |= DBTR_TDATA1_MCONTROL6_STORE | DBTR_TDATA1_MCONTROL6_LOAD; + break; + default: + return -EINVAL; + } + + switch (attr->bp_len) { + case HW_BREAKPOINT_LEN_1: + hw->len = 1; + tdata1 |= TDATA1_MCTRL6_SZ(DBTR_TDATA1_MCONTROL6_SIZE_8BIT); + break; + case HW_BREAKPOINT_LEN_2: + hw->len = 2; + tdata1 |= TDATA1_MCTRL6_SZ(DBTR_TDATA1_MCONTROL6_SIZE_16BIT); + break; + case HW_BREAKPOINT_LEN_4: + hw->len = 4; + tdata1 |= TDATA1_MCTRL6_SZ(DBTR_TDATA1_MCONTROL6_SIZE_32BIT); + break; + case HW_BREAKPOINT_LEN_8: + hw->len = 8; + tdata1 |= TDATA1_MCTRL6_SZ(DBTR_TDATA1_MCONTROL6_SIZE_64BIT); + break; + default: + return -EINVAL; + } + + switch (mode) { + case DBTR_MODE_U: + tdata1 |= DBTR_TDATA1_MCONTROL6_U; + break; + case DBTR_MODE_S: + tdata1 |= DBTR_TDATA1_MCONTROL6_S; + break; + case DBTR_MODE_VS: + tdata1 |= DBTR_TDATA1_MCONTROL6_VS; + break; + case DBTR_MODE_VU: + tdata1 |= DBTR_TDATA1_MCONTROL6_VU; + break; + default: + return -EINVAL; + } + + hw->tdata1 = tdata1; + + return 0; +} + +int hw_breakpoint_arch_parse(struct perf_event *bp, + const struct perf_event_attr *attr, + struct arch_hw_breakpoint *hw) +{ + int ret; + + /* Breakpoint address */ + hw->address = attr->bp_addr; + hw->tdata2 = attr->bp_addr; + hw->tdata3 = 0x0; + hw->next_addr = 0x0; + hw->in_callback = false; + + switch (dbtr_type) { + case DBTR_TDATA1_TYPE_MCONTROL: + ret = rv_init_mcontrol_trigger(attr, hw, DBTR_MODE_U); + break; + case DBTR_TDATA1_TYPE_MCONTROL6: + ret = rv_init_mcontrol6_trigger(attr, hw, DBTR_MODE_U); + break; + default: + pr_warn("Unsupported trigger type %lu.\n", dbtr_type >> DBTR_TDATA1_TYPE_SHIFT); + ret = -EOPNOTSUPP; + break; + } + + return ret; +} + +/** + * setup_singlestep - Update breakpoint to next instruction after breakpoint. + * @event: Perf event to change + * @regs: regs at breakpoint + * + * Update breakpoint to next instruction that would have + * executed after the current breakpoint. + * + * This allows for single-stepping the instruction being debugged. + * Then restoring the original breakpoint. + * + * Returns Zero on success, negative on failure + */ +static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) +{ + struct arch_hw_breakpoint *bp = counter_arch_bp(event); + struct perf_event_attr bp_insn; + unsigned long insn, next_addr = 0; + int ret; + + /* Remove breakpoint even if return error as not to loop */ + arch_uninstall_hw_breakpoint(event); + + ret = get_insn_nofault(regs, regs->epc, &insn); + if (ret < 0) + return ret; + + next_addr = get_step_address(regs, insn); + + ret = get_insn_nofault(regs, next_addr, &insn); + if (ret < 0) + return ret; + + bp_insn.bp_type = HW_BREAKPOINT_X; + bp_insn.bp_addr = next_addr; + /* Get the size of the intruction */ + bp_insn.bp_len = GET_INSN_LENGTH(insn); + + ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp); + if (ret) + return ret; + + ret = arch_install_hw_breakpoint(event); + if (ret) + return ret; + + bp->in_callback = true; + bp->next_addr = next_addr; + return 0; +} + +/* + * HW Breakpoint/watchpoint handler + */ +static int hw_breakpoint_handler(struct pt_regs *regs) +{ + int i, ret = 0, bp_ret = NOTIFY_DONE; + bool expecting_callback = false; + struct arch_hw_breakpoint *bp; + struct perf_event *event; + + for (i = 0; i < dbtr_total_num; i++) { + event = this_cpu_read(pcpu_hw_bp_events[i]); + if (!event) + continue; + + bp = counter_arch_bp(event); + switch (event->attr.bp_type) { + /* Breakpoint */ + case HW_BREAKPOINT_X: + if (event->attr.bp_addr == regs->epc) { + perf_bp_event(event, regs); + ret = setup_singlestep(event, regs); + if (ret < 0) { + pr_err("%s: setup_singlestep failed %d.\n", __func__, ret); + goto exit; + } + + bp_ret = NOTIFY_STOP; + goto exit; + } + break; + + /* Watchpoint */ + case HW_BREAKPOINT_W: + case HW_BREAKPOINT_R: + case HW_BREAKPOINT_RW: + /* Watchpoints will trigger on smaller loads than the given type. + * To allow for this, check if the load was within the size of + * the type. Cast badaddr to the type of bp_addr. + */ + if (abs_diff(event->attr.bp_addr, (__u64)regs->badaddr) < bp->len) { + perf_bp_event(event, regs); + ret = setup_singlestep(event, regs); + if (ret < 0) { + pr_err("%s: setup_singlestep failed %d.\n", __func__, ret); + goto exit; + } + + bp_ret = NOTIFY_STOP; + goto exit; + } + break; + + default: + pr_warn("%s: Unknown type: %u\n", __func__, event->attr.bp_type); + goto exit; + } + + if (bp->in_callback) { + expecting_callback = true; + if (regs->epc != bp->next_addr) { + continue; + } + + arch_uninstall_hw_breakpoint(event); + /* Restore original breakpoint */ + if (hw_breakpoint_arch_parse(NULL, &event->attr, bp)) + goto exit; + if (arch_install_hw_breakpoint(event)) + goto exit; + + bp_ret = NOTIFY_STOP; + goto exit; + } + + } + + if (expecting_callback) { + pr_err("%s: in_callback was set, but epc (%lx) was not at next address(%lx).\n", + __func__, regs->epc, bp->next_addr); + } +exit: + return bp_ret; + +} + +int hw_breakpoint_exceptions_notify(struct notifier_block *unused, + unsigned long val, void *data) +{ + struct die_args *args = data; + + if (val != DIE_DEBUG) + return NOTIFY_DONE; + + return hw_breakpoint_handler(args->regs); +} + +/* atomic: counter->ctx->lock is held */ +int arch_install_hw_breakpoint(struct perf_event *event) +{ + struct arch_hw_breakpoint *bp = counter_arch_bp(event); + union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem); + struct sbi_dbtr_data_msg *xmit; + struct sbi_dbtr_id_msg *recv; + struct perf_event **slot; + unsigned long idx; + struct sbiret ret; + int err = 0; + + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), + *this_cpu_ptr(&ecall_lock_flags)); + + xmit = &shmem->data; + recv = &shmem->id; + xmit->tdata1 = cpu_to_le(bp->tdata1); + xmit->tdata2 = cpu_to_le(bp->tdata2); + xmit->tdata3 = cpu_to_le(bp->tdata3); + + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_INSTALL, + 1, 0, 0, 0, 0, 0); + + if (ret.error) { + pr_warn("%s: failed to install trigger. error: %ld\n", __func__, ret.error); + err = sbi_err_map_linux_errno(ret.error); + goto done; + } + + idx = le_to_cpu(recv->idx); + if (idx >= dbtr_total_num) { + pr_warn("%s: invalid trigger index %lu\n", __func__, idx); + err = -EINVAL; + goto done; + } + + slot = this_cpu_ptr(&pcpu_hw_bp_events[idx]); + if (*slot) { + pr_warn("%s: slot %lu is in use\n", __func__, idx); + err = -EBUSY; + goto done; + } + + pr_debug("Trigger 0x%lx installed at index 0x%lx\n", bp->tdata2, idx); + + /* Save the event - to be looked up in handler */ + *slot = event; + +done: + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), + *this_cpu_ptr(&ecall_lock_flags)); + return err; +} + +void arch_uninstall_hw_breakpoint(struct perf_event *event) +{ + struct perf_event **slot; + struct sbiret ret; + int i; + + for (i = 0; i < dbtr_total_num; i++) { + slot = this_cpu_ptr(&pcpu_hw_bp_events[i]); + + if (*slot == event) { + *slot = NULL; + break; + } + } + + if (i == dbtr_total_num) { + pr_warn("%s: Breakpoint not installed.\n", __func__); + return; + } + + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UNINSTALL, + i, 1, 0, 0, 0, 0); + if (ret.error) + pr_warn("%s: Failed to uninstall trigger %d. error: %ld\n", __func__, i, ret.error); +} + +void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { } + +void hw_breakpoint_pmu_read(struct perf_event *bp) { } + +static int __init arch_hw_breakpoint_init(void) +{ + unsigned int cpu; + int rc = 0; + + for_each_possible_cpu(cpu) + raw_spin_lock_init(&per_cpu(ecall_lock, cpu)); + + init_sbi_dbtr(); + + if (dbtr_total_num) { + pr_debug("%s: total number of type %lu triggers: %u\n", + __func__, dbtr_type >> DBTR_TDATA1_TYPE_SHIFT, dbtr_total_num); + } else { + pr_debug("%s: No hardware triggers available\n", __func__); + return rc; + } + + /* Hotplug handler to register/unregister shared memory with SBI */ + rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, + "riscv/hw_breakpoint:prepare", + arch_smp_setup_sbi_shmem, + arch_smp_teardown_sbi_shmem); + + if (rc < 0) + pr_warn("%s: Failed to setup CPU hotplug state\n", __func__); + + return rc; +} +arch_initcall(arch_hw_breakpoint_init); diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 938a8b841f94..2ac471ec79a8 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -289,6 +289,12 @@ void handle_break(struct pt_regs *regs) if (probe_breakpoint_handler(regs)) return;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT + if (notify_die(DIE_DEBUG, "EBREAK", regs, 0, regs->cause, SIGTRAP) + == NOTIFY_STOP) + return; +#endif + current->thread.bad_cause = regs->cause;
if (user_mode(regs))
On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube jesse@rivosinc.com wrote:
From: Himanshu Chauhan hchauhan@ventanamicro.com
RISC-V hardware breakpoint framework is built on top of perf subsystem and uses SBI debug trigger extension to install/uninstall/update/enable/disable hardware triggers as specified in Sdtrig ISA extension.
Signed-off-by: Himanshu Chauhan hchauhan@ventanamicro.com Signed-off-by: Jesse Taube jesse@rivosinc.com
RFC -> V1:
- Add dbtr_mode to rv_init_mcontrol(6)_trigger
- Add select HAVE_MIXED_BREAKPOINTS_REGS
- Add TDATA1_MCTRL_SZ and TDATA1_MCTRL6_SZ
- Capitalize F in Fallback comment
- Fix in_callback code to allow multiple breakpoints
- Move perf_bp_event above setup_singlestep to save the correct state
- Use sbi_err_map_linux_errno for arch_smp_teardown/setup_sbi_shmem
V1 -> V2:
- No change
arch/riscv/Kconfig | 2 + arch/riscv/include/asm/hw_breakpoint.h | 59 +++ arch/riscv/include/asm/kdebug.h | 3 +- arch/riscv/include/asm/sbi.h | 4 +- arch/riscv/kernel/Makefile | 1 + arch/riscv/kernel/hw_breakpoint.c | 614 +++++++++++++++++++++++++ arch/riscv/kernel/traps.c | 6 + 7 files changed, 687 insertions(+), 2 deletions(-) create mode 100644 arch/riscv/include/asm/hw_breakpoint.h create mode 100644 arch/riscv/kernel/hw_breakpoint.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index bbec87b79309..fd8b62cdc6f5 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -163,6 +163,7 @@ config RISCV select HAVE_FUNCTION_ERROR_INJECTION select HAVE_GCC_PLUGINS select HAVE_GENERIC_VDSO if MMU && 64BIT
select HAVE_HW_BREAKPOINT if PERF_EVENTS && RISCV_SBI select HAVE_IRQ_TIME_ACCOUNTING select HAVE_KERNEL_BZIP2 if !XIP_KERNEL && !EFI_ZBOOT select HAVE_KERNEL_GZIP if !XIP_KERNEL && !EFI_ZBOOT@@ -176,6 +177,7 @@ config RISCV select HAVE_KRETPROBES if !XIP_KERNEL # https://github.com/ClangBuiltLinux/linux/issues/1881 select HAVE_LD_DEAD_CODE_DATA_ELIMINATION if !LD_IS_LLD
select HAVE_MIXED_BREAKPOINTS_REGS select HAVE_MOVE_PMD select HAVE_MOVE_PUD select HAVE_PAGE_SIZE_4KBdiff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h new file mode 100644 index 000000000000..cde6688b91d2 --- /dev/null +++ b/arch/riscv/include/asm/hw_breakpoint.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*
- Copyright (C) 2024 Ventana Micro Systems Inc.
- */
+#ifndef __RISCV_HW_BREAKPOINT_H +#define __RISCV_HW_BREAKPOINT_H
+struct task_struct;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+#include <uapi/linux/hw_breakpoint.h>
+#if __riscv_xlen == 64 +#define cpu_to_le cpu_to_le64 +#define le_to_cpu le64_to_cpu +#elif __riscv_xlen == 32 +#define cpu_to_le cpu_to_le32 +#define le_to_cpu le32_to_cpu +#else +#error "Unexpected __riscv_xlen" +#endif
+struct arch_hw_breakpoint {
unsigned long address;unsigned long len;/* Callback info */unsigned long next_addr;bool in_callback;/* Trigger configuration data */unsigned long tdata1;unsigned long tdata2;unsigned long tdata3;+};
+/* Maximum number of hardware breakpoints supported */ +#define RV_MAX_TRIGGERS 32
+struct perf_event_attr; +struct notifier_block; +struct perf_event; +struct pt_regs;
+int hw_breakpoint_slots(int type); +int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw); +int hw_breakpoint_arch_parse(struct perf_event *bp,
const struct perf_event_attr *attr,struct arch_hw_breakpoint *hw);+int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
unsigned long val, void *data);+int arch_install_hw_breakpoint(struct perf_event *bp); +void arch_uninstall_hw_breakpoint(struct perf_event *bp); +void hw_breakpoint_pmu_read(struct perf_event *bp);
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */ +#endif /* __RISCV_HW_BREAKPOINT_H */ diff --git a/arch/riscv/include/asm/kdebug.h b/arch/riscv/include/asm/kdebug.h index 85ac00411f6e..53e989781aa1 100644 --- a/arch/riscv/include/asm/kdebug.h +++ b/arch/riscv/include/asm/kdebug.h @@ -6,7 +6,8 @@ enum die_val { DIE_UNUSED, DIE_TRAP,
DIE_OOPS
DIE_OOPS,DIE_DEBUG};
#endif diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h index be2ca8e8a49e..64fa7a82aa45 100644 --- a/arch/riscv/include/asm/sbi.h +++ b/arch/riscv/include/asm/sbi.h @@ -282,7 +282,9 @@ struct sbi_sta_struct { u8 pad[47]; } __packed;
-#define SBI_SHMEM_DISABLE -1 +#define SBI_SHMEM_DISABLE (-1UL) +#define SBI_SHMEM_LO(pa) ((unsigned long)lower_32_bits(pa)) +#define SBI_SHMEM_HI(pa) ((unsigned long)upper_32_bits(pa))
These definitions of SBI_SHMEM_LO() and SBI_SHMEM_HI() are broken for RV64 platforms where a good amount of RAM is beyond first 4GB.
This should be:
#ifdef CONFIG_32BIT #define SBI_SHMEM_LO(pa) ((unsigned long)lower_32_bits(pa)) #define SBI_SHMEM_HI(pa) ((unsigned long)upper_32_bits(pa)) #else #define SBI_SHMEM_LO(pa) ((unsigned long)pa) #define SBI_SHMEM_HI(pa) 0UL #endif
Regards, Anup
The Sdtrig RISC-V ISA extension does not have a resume flag for returning to and executing the instruction at the breakpoint. To avoid skipping the instruction or looping, it is necessary to remove the hardware breakpoint and single step. Use the icount feature of Sdtrig to accomplish this. Use icount as default with an option to allow software-based single stepping when hardware or SBI does not have icount functionality, as it may cause unwanted side effects when reading the instruction from memory.
Signed-off-by: Jesse Taube jesse@rivosinc.com --- OpenSBI implementation of sbi_debug_read_triggers does not return the updated CSR values. There needs to be a check for working sbi_debug_read_triggers before this works.
https://lists.riscv.org/g/tech-prs/message/1476
RFC -> V1: - Add dbtr_mode to rv_init_icount_trigger - Add icount_triggered to check which breakpoint was triggered - Fix typo: s/affects/effects - Move HW_BREAKPOINT_COMPUTE_STEP to Platform type V1 -> V2: - Remove HW_BREAKPOINT_COMPUTE_STEP kconfig option --- arch/riscv/kernel/hw_breakpoint.c | 173 ++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 18 deletions(-)
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c index 3f96e744a711..f12306247436 100644 --- a/arch/riscv/kernel/hw_breakpoint.c +++ b/arch/riscv/kernel/hw_breakpoint.c @@ -20,6 +20,7 @@ #define DBTR_TDATA1_DMODE BIT_UL(__riscv_xlen - 5)
#define DBTR_TDATA1_TYPE_MCONTROL (2UL << DBTR_TDATA1_TYPE_SHIFT) +#define DBTR_TDATA1_TYPE_ICOUNT (3UL << DBTR_TDATA1_TYPE_SHIFT) #define DBTR_TDATA1_TYPE_MCONTROL6 (6UL << DBTR_TDATA1_TYPE_SHIFT)
#define DBTR_TDATA1_MCONTROL6_LOAD BIT(0) @@ -62,6 +63,14 @@ (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \ FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi))
+#define DBTR_TDATA1_ICOUNT_U BIT(6) +#define DBTR_TDATA1_ICOUNT_S BIT(7) +#define DBTR_TDATA1_ICOUNT_PENDING BIT(8) +#define DBTR_TDATA1_ICOUNT_M BIT(9) +#define DBTR_TDATA1_ICOUNT_COUNT_FIELD GENMASK(23, 10) +#define DBTR_TDATA1_ICOUNT_VU BIT(25) +#define DBTR_TDATA1_ICOUNT_VS BIT(26) + enum dbtr_mode { DBTR_MODE_U = 0, DBTR_MODE_S, @@ -79,6 +88,7 @@ static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem);
/* number of debug triggers on this cpu . */ static int dbtr_total_num __ro_after_init; +static bool have_icount __ro_after_init; static unsigned long dbtr_type __ro_after_init; static unsigned long dbtr_init __ro_after_init;
@@ -129,6 +139,7 @@ static int arch_smp_teardown_sbi_shmem(unsigned int cpu) static void init_sbi_dbtr(void) { struct sbiret ret; + unsigned long dbtr_count = 0;
/* * Called by hw_breakpoint_slots and arch_hw_breakpoint_init. @@ -143,6 +154,19 @@ static void init_sbi_dbtr(void) return; }
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, + DBTR_TDATA1_TYPE_ICOUNT, 0, 0, 0, 0, 0); + if (ret.error) { + pr_warn("%s: failed to detect icount triggers. error: %ld.\n", + __func__, ret.error); + } else if (!ret.value) { + pr_warn("%s: No icount triggers available. " + "Falling-back to computing single step address.\n", __func__); + } else { + dbtr_count = ret.value; + have_icount = true; + } + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0); if (ret.error) { @@ -151,7 +175,7 @@ static void init_sbi_dbtr(void) } else if (!ret.value) { pr_warn("%s: No mcontrol6 triggers available.\n", __func__); } else { - dbtr_total_num = ret.value; + dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6; return; } @@ -166,7 +190,7 @@ static void init_sbi_dbtr(void) pr_err("%s: No mcontrol triggers available.\n", __func__); dbtr_total_num = 0; } else { - dbtr_total_num = ret.value; + dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL; } } @@ -320,6 +344,36 @@ static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr, return 0; }
+static int rv_init_icount_trigger(struct arch_hw_breakpoint *hw, enum dbtr_mode mode) +{ + unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT; + + /* Step one instruction */ + tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1); + + switch (mode) { + case DBTR_MODE_U: + tdata1 |= DBTR_TDATA1_ICOUNT_U; + break; + case DBTR_MODE_S: + tdata1 |= DBTR_TDATA1_ICOUNT_S; + break; + case DBTR_MODE_VS: + tdata1 |= DBTR_TDATA1_ICOUNT_VS; + break; + case DBTR_MODE_VU: + tdata1 |= DBTR_TDATA1_ICOUNT_VU; + break; + default: + return -EINVAL; + } + + hw->tdata1 = tdata1; + hw->tdata2 = 0; + + return 0; +} + int hw_breakpoint_arch_parse(struct perf_event *bp, const struct perf_event_attr *attr, struct arch_hw_breakpoint *hw) @@ -372,24 +426,28 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) /* Remove breakpoint even if return error as not to loop */ arch_uninstall_hw_breakpoint(event);
- ret = get_insn_nofault(regs, regs->epc, &insn); - if (ret < 0) - return ret; + if (have_icount) { + rv_init_icount_trigger(bp, DBTR_MODE_U); + } else { + ret = get_insn_nofault(regs, regs->epc, &insn); + if (ret < 0) + return ret;
- next_addr = get_step_address(regs, insn); + next_addr = get_step_address(regs, insn);
- ret = get_insn_nofault(regs, next_addr, &insn); - if (ret < 0) - return ret; + ret = get_insn_nofault(regs, next_addr, &insn); + if (ret < 0) + return ret;
- bp_insn.bp_type = HW_BREAKPOINT_X; - bp_insn.bp_addr = next_addr; - /* Get the size of the intruction */ - bp_insn.bp_len = GET_INSN_LENGTH(insn); + bp_insn.bp_type = HW_BREAKPOINT_X; + bp_insn.bp_addr = next_addr; + /* Get the size of the intruction */ + bp_insn.bp_len = GET_INSN_LENGTH(insn);
- ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp); - if (ret) - return ret; + ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp); + if (ret) + return ret; + }
ret = arch_install_hw_breakpoint(event); if (ret) @@ -400,6 +458,79 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) return 0; }
+/** + * icount_triggered - Check if event's icount was triggered. + * @event: Perf event to check + * + * Check the given perf event's icount breakpoint was triggered. + * + * Returns: 1 if icount was triggered. + * 0 if icount was not triggered. + * negative on failure. + */ +static int icount_triggered(struct perf_event *event) +{ + union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem); + struct sbiret ret; + struct perf_event **slot; + unsigned long tdata1; + int i; + + for (i = 0; i < dbtr_total_num; i++) { + slot = this_cpu_ptr(&pcpu_hw_bp_events[i]); + + if (*slot == event) + break; + } + + if (i == dbtr_total_num) { + pr_warn("%s: Breakpoint not installed.\n", __func__); + return -ENOENT; + } + + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), + *this_cpu_ptr(&ecall_lock_flags)); + + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ, + i, 1, 0, 0, 0, 0); + tdata1 = shmem->data.tdata1; + + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), + *this_cpu_ptr(&ecall_lock_flags)); + if (ret.error) { + pr_warn("%s: failed to read trigger. error: %ld\n", __func__, ret.error); + return sbi_err_map_linux_errno(ret.error); + } + + /* + * The RISC-V Debug Specification + * Tim Newsome, Paul Donahue (Ventana Micro Systems) + * Version 1.0, Revised 2025-02-21: Ratified + * 5.7.13. Instruction Count (icount, at 0x7a1) + * When count is 1 and the trigger matches, then pending becomes set. + * In addition count will become 0 unless it is hard-wired to 1. + * When pending is set, the trigger fires just before any further + * instructions are executed in a mode where the trigger is enabled. + * As the trigger fires, pending is cleared. In addition, if count is + * hard-wired to 1 then m, s, u, vs, and vu are all cleared. + */ + if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) == 0) + return 1; + + if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) != 1) + return 0; + + if (tdata1 & DBTR_TDATA1_ICOUNT_U) + return 0; + if (tdata1 & DBTR_TDATA1_ICOUNT_S) + return 0; + if (tdata1 & DBTR_TDATA1_ICOUNT_VU) + return 0; + if (tdata1 & DBTR_TDATA1_ICOUNT_VU) + return 0; + return 1; +} + /* * HW Breakpoint/watchpoint handler */ @@ -460,7 +591,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
if (bp->in_callback) { expecting_callback = true; - if (regs->epc != bp->next_addr) { + if (have_icount) { + if (icount_triggered(event) != 1) + continue; + } else if (regs->epc != bp->next_addr) { continue; }
@@ -477,7 +611,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
}
- if (expecting_callback) { + if (expecting_callback && have_icount) { + pr_err("%s: in_callback was set, but icount was not triggered, epc (%lx).\n", + __func__, regs->epc); + } else if (expecting_callback) { pr_err("%s: in_callback was set, but epc (%lx) was not at next address(%lx).\n", __func__, regs->epc, bp->next_addr); }
On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube jesse@rivosinc.com wrote:
The Sdtrig RISC-V ISA extension does not have a resume flag for returning to and executing the instruction at the breakpoint. To avoid skipping the instruction or looping, it is necessary to remove the hardware breakpoint and single step. Use the icount feature of Sdtrig to accomplish this. Use icount as default with an option to allow software-based single stepping when hardware or SBI does not have icount functionality, as it may cause unwanted side effects when reading the instruction from memory.
Can you please elaborate on this? I remember noticing the absence of the resume flag which was causing loops.
Signed-off-by: Jesse Taube jesse@rivosinc.com
OpenSBI implementation of sbi_debug_read_triggers does not return the updated CSR values. There needs to be a check for working sbi_debug_read_triggers before this works.
https://lists.riscv.org/g/tech-prs/message/1476
RFC -> V1:
- Add dbtr_mode to rv_init_icount_trigger
- Add icount_triggered to check which breakpoint was triggered
- Fix typo: s/affects/effects
- Move HW_BREAKPOINT_COMPUTE_STEP to Platform type
V1 -> V2:
- Remove HW_BREAKPOINT_COMPUTE_STEP kconfig option
arch/riscv/kernel/hw_breakpoint.c | 173 ++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 18 deletions(-)
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c index 3f96e744a711..f12306247436 100644 --- a/arch/riscv/kernel/hw_breakpoint.c +++ b/arch/riscv/kernel/hw_breakpoint.c @@ -20,6 +20,7 @@ #define DBTR_TDATA1_DMODE BIT_UL(__riscv_xlen - 5)
#define DBTR_TDATA1_TYPE_MCONTROL (2UL << DBTR_TDATA1_TYPE_SHIFT) +#define DBTR_TDATA1_TYPE_ICOUNT (3UL << DBTR_TDATA1_TYPE_SHIFT) #define DBTR_TDATA1_TYPE_MCONTROL6 (6UL << DBTR_TDATA1_TYPE_SHIFT)
#define DBTR_TDATA1_MCONTROL6_LOAD BIT(0) @@ -62,6 +63,14 @@ (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \ FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi))
+#define DBTR_TDATA1_ICOUNT_U BIT(6) +#define DBTR_TDATA1_ICOUNT_S BIT(7) +#define DBTR_TDATA1_ICOUNT_PENDING BIT(8) +#define DBTR_TDATA1_ICOUNT_M BIT(9) +#define DBTR_TDATA1_ICOUNT_COUNT_FIELD GENMASK(23, 10) +#define DBTR_TDATA1_ICOUNT_VU BIT(25) +#define DBTR_TDATA1_ICOUNT_VS BIT(26)
enum dbtr_mode { DBTR_MODE_U = 0, DBTR_MODE_S, @@ -79,6 +88,7 @@ static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem);
/* number of debug triggers on this cpu . */ static int dbtr_total_num __ro_after_init; +static bool have_icount __ro_after_init; static unsigned long dbtr_type __ro_after_init; static unsigned long dbtr_init __ro_after_init;
@@ -129,6 +139,7 @@ static int arch_smp_teardown_sbi_shmem(unsigned int cpu) static void init_sbi_dbtr(void) { struct sbiret ret;
unsigned long dbtr_count = 0; /* * Called by hw_breakpoint_slots and arch_hw_breakpoint_init.@@ -143,6 +154,19 @@ static void init_sbi_dbtr(void) return; }
ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,DBTR_TDATA1_TYPE_ICOUNT, 0, 0, 0, 0, 0);if (ret.error) {pr_warn("%s: failed to detect icount triggers. error: %ld.\n",__func__, ret.error);} else if (!ret.value) {pr_warn("%s: No icount triggers available. ""Falling-back to computing single step address.\n", __func__);} else {dbtr_count = ret.value;have_icount = true;}ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0); if (ret.error) {@@ -151,7 +175,7 @@ static void init_sbi_dbtr(void) } else if (!ret.value) { pr_warn("%s: No mcontrol6 triggers available.\n", __func__); } else {
dbtr_total_num = ret.value;
dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6; return; }@@ -166,7 +190,7 @@ static void init_sbi_dbtr(void) pr_err("%s: No mcontrol triggers available.\n", __func__); dbtr_total_num = 0; } else {
dbtr_total_num = ret.value;
dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL; }} @@ -320,6 +344,36 @@ static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr, return 0; }
+static int rv_init_icount_trigger(struct arch_hw_breakpoint *hw, enum dbtr_mode mode) +{
unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT;/* Step one instruction */tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1);switch (mode) {case DBTR_MODE_U:tdata1 |= DBTR_TDATA1_ICOUNT_U;break;case DBTR_MODE_S:tdata1 |= DBTR_TDATA1_ICOUNT_S;break;case DBTR_MODE_VS:tdata1 |= DBTR_TDATA1_ICOUNT_VS;break;case DBTR_MODE_VU:tdata1 |= DBTR_TDATA1_ICOUNT_VU;break;default:return -EINVAL;}hw->tdata1 = tdata1;hw->tdata2 = 0;return 0;+}
int hw_breakpoint_arch_parse(struct perf_event *bp, const struct perf_event_attr *attr, struct arch_hw_breakpoint *hw) @@ -372,24 +426,28 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) /* Remove breakpoint even if return error as not to loop */ arch_uninstall_hw_breakpoint(event);
ret = get_insn_nofault(regs, regs->epc, &insn);if (ret < 0)return ret;
if (have_icount) {rv_init_icount_trigger(bp, DBTR_MODE_U);} else {ret = get_insn_nofault(regs, regs->epc, &insn);if (ret < 0)return ret;
next_addr = get_step_address(regs, insn);
next_addr = get_step_address(regs, insn);
ret = get_insn_nofault(regs, next_addr, &insn);if (ret < 0)return ret;
ret = get_insn_nofault(regs, next_addr, &insn);if (ret < 0)return ret;
bp_insn.bp_type = HW_BREAKPOINT_X;bp_insn.bp_addr = next_addr;/* Get the size of the intruction */bp_insn.bp_len = GET_INSN_LENGTH(insn);
bp_insn.bp_type = HW_BREAKPOINT_X;bp_insn.bp_addr = next_addr;/* Get the size of the intruction */bp_insn.bp_len = GET_INSN_LENGTH(insn);
ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);if (ret)return ret;
ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);if (ret)return ret;} ret = arch_install_hw_breakpoint(event); if (ret)@@ -400,6 +458,79 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) return 0; }
+/**
- icount_triggered - Check if event's icount was triggered.
- @event: Perf event to check
- Check the given perf event's icount breakpoint was triggered.
- Returns: 1 if icount was triggered.
0 if icount was not triggered.
negative on failure.- */
+static int icount_triggered(struct perf_event *event) +{
union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem);struct sbiret ret;struct perf_event **slot;unsigned long tdata1;int i;for (i = 0; i < dbtr_total_num; i++) {slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);if (*slot == event)break;}if (i == dbtr_total_num) {pr_warn("%s: Breakpoint not installed.\n", __func__);return -ENOENT;}raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),*this_cpu_ptr(&ecall_lock_flags));ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,i, 1, 0, 0, 0, 0);tdata1 = shmem->data.tdata1;raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),*this_cpu_ptr(&ecall_lock_flags));if (ret.error) {pr_warn("%s: failed to read trigger. error: %ld\n", __func__, ret.error);return sbi_err_map_linux_errno(ret.error);
To avoid a flurry of events or messages, it would probably be good to disable the trigger.
}/** The RISC-V Debug Specification* Tim Newsome, Paul Donahue (Ventana Micro Systems)* Version 1.0, Revised 2025-02-21: Ratified
I think mentioning the version number and section would be enough.
* 5.7.13. Instruction Count (icount, at 0x7a1)* When count is 1 and the trigger matches, then pending becomes set.* In addition count will become 0 unless it is hard-wired to 1.* When pending is set, the trigger fires just before any further* instructions are executed in a mode where the trigger is enabled.* As the trigger fires, pending is cleared. In addition, if count is* hard-wired to 1 then m, s, u, vs, and vu are all cleared.*/if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) == 0)return 1;if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) != 1)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_U)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_S)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_VU)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_VU)return 0;return 1;+}
/*
- HW Breakpoint/watchpoint handler
*/ @@ -460,7 +591,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
if (bp->in_callback) { expecting_callback = true;
if (regs->epc != bp->next_addr) {
if (have_icount) {if (icount_triggered(event) != 1)continue;} else if (regs->epc != bp->next_addr) { continue; }@@ -477,7 +611,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
}
if (expecting_callback) {
if (expecting_callback && have_icount) {pr_err("%s: in_callback was set, but icount was not triggered, epc (%lx).\n",__func__, regs->epc);} else if (expecting_callback) { pr_err("%s: in_callback was set, but epc (%lx) was not at next address(%lx).\n", __func__, regs->epc, bp->next_addr); }
Is this just for debugging or do you want to commit it?
Regards Himanshu
-- 2.43.0
On Tue, Aug 26, 2025 at 10:08:04AM +0530, Himanshu Chauhan wrote:
On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube jesse@rivosinc.com wrote:
The Sdtrig RISC-V ISA extension does not have a resume flag for returning to and executing the instruction at the breakpoint. To avoid skipping the instruction or looping, it is necessary to remove the hardware breakpoint and single step. Use the icount feature of Sdtrig to accomplish this. Use icount as default with an option to allow software-based single stepping when hardware or SBI does not have icount functionality, as it may cause unwanted side effects when reading the instruction from memory.
Can you please elaborate on this? I remember noticing the absence of the resume flag which was causing loops.
Thank you for your feedback. Jesse's internship came to an end last Friday (August 22nd) so I will be picking up these patches, but I have also added her personal email onto this thread.
When a breakpoint is triggered and the kernel gains control, the last instruction to execute was the instruction before the instruction where the breakpoint is installed. If execution was to be resumed at this stage, the same breakpoint would be triggered again. So single stepping requires a careful dance of enabling and disabling breakpoints. However, we can avoid this overhead and code complexity can be reduced by using the icount trigger which allows a direct method for single stepping.
Signed-off-by: Jesse Taube jesse@rivosinc.com
OpenSBI implementation of sbi_debug_read_triggers does not return the updated CSR values. There needs to be a check for working sbi_debug_read_triggers before this works.
https://lists.riscv.org/g/tech-prs/message/1476
RFC -> V1:
- Add dbtr_mode to rv_init_icount_trigger
- Add icount_triggered to check which breakpoint was triggered
- Fix typo: s/affects/effects
- Move HW_BREAKPOINT_COMPUTE_STEP to Platform type
V1 -> V2:
- Remove HW_BREAKPOINT_COMPUTE_STEP kconfig option
arch/riscv/kernel/hw_breakpoint.c | 173 ++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 18 deletions(-)
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c index 3f96e744a711..f12306247436 100644 --- a/arch/riscv/kernel/hw_breakpoint.c +++ b/arch/riscv/kernel/hw_breakpoint.c @@ -20,6 +20,7 @@ #define DBTR_TDATA1_DMODE BIT_UL(__riscv_xlen - 5)
#define DBTR_TDATA1_TYPE_MCONTROL (2UL << DBTR_TDATA1_TYPE_SHIFT) +#define DBTR_TDATA1_TYPE_ICOUNT (3UL << DBTR_TDATA1_TYPE_SHIFT) #define DBTR_TDATA1_TYPE_MCONTROL6 (6UL << DBTR_TDATA1_TYPE_SHIFT)
#define DBTR_TDATA1_MCONTROL6_LOAD BIT(0) @@ -62,6 +63,14 @@ (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \ FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi))
+#define DBTR_TDATA1_ICOUNT_U BIT(6) +#define DBTR_TDATA1_ICOUNT_S BIT(7) +#define DBTR_TDATA1_ICOUNT_PENDING BIT(8) +#define DBTR_TDATA1_ICOUNT_M BIT(9) +#define DBTR_TDATA1_ICOUNT_COUNT_FIELD GENMASK(23, 10) +#define DBTR_TDATA1_ICOUNT_VU BIT(25) +#define DBTR_TDATA1_ICOUNT_VS BIT(26)
enum dbtr_mode { DBTR_MODE_U = 0, DBTR_MODE_S, @@ -79,6 +88,7 @@ static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem);
/* number of debug triggers on this cpu . */ static int dbtr_total_num __ro_after_init; +static bool have_icount __ro_after_init; static unsigned long dbtr_type __ro_after_init; static unsigned long dbtr_init __ro_after_init;
@@ -129,6 +139,7 @@ static int arch_smp_teardown_sbi_shmem(unsigned int cpu) static void init_sbi_dbtr(void) { struct sbiret ret;
unsigned long dbtr_count = 0; /* * Called by hw_breakpoint_slots and arch_hw_breakpoint_init.@@ -143,6 +154,19 @@ static void init_sbi_dbtr(void) return; }
ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,DBTR_TDATA1_TYPE_ICOUNT, 0, 0, 0, 0, 0);if (ret.error) {pr_warn("%s: failed to detect icount triggers. error: %ld.\n",__func__, ret.error);} else if (!ret.value) {pr_warn("%s: No icount triggers available. ""Falling-back to computing single step address.\n", __func__);} else {dbtr_count = ret.value;have_icount = true;}ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0); if (ret.error) {@@ -151,7 +175,7 @@ static void init_sbi_dbtr(void) } else if (!ret.value) { pr_warn("%s: No mcontrol6 triggers available.\n", __func__); } else {
dbtr_total_num = ret.value;
dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6; return; }@@ -166,7 +190,7 @@ static void init_sbi_dbtr(void) pr_err("%s: No mcontrol triggers available.\n", __func__); dbtr_total_num = 0; } else {
dbtr_total_num = ret.value;
dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL; }} @@ -320,6 +344,36 @@ static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr, return 0; }
+static int rv_init_icount_trigger(struct arch_hw_breakpoint *hw, enum dbtr_mode mode) +{
unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT;/* Step one instruction */tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1);switch (mode) {case DBTR_MODE_U:tdata1 |= DBTR_TDATA1_ICOUNT_U;break;case DBTR_MODE_S:tdata1 |= DBTR_TDATA1_ICOUNT_S;break;case DBTR_MODE_VS:tdata1 |= DBTR_TDATA1_ICOUNT_VS;break;case DBTR_MODE_VU:tdata1 |= DBTR_TDATA1_ICOUNT_VU;break;default:return -EINVAL;}hw->tdata1 = tdata1;hw->tdata2 = 0;return 0;+}
int hw_breakpoint_arch_parse(struct perf_event *bp, const struct perf_event_attr *attr, struct arch_hw_breakpoint *hw) @@ -372,24 +426,28 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) /* Remove breakpoint even if return error as not to loop */ arch_uninstall_hw_breakpoint(event);
ret = get_insn_nofault(regs, regs->epc, &insn);if (ret < 0)return ret;
if (have_icount) {rv_init_icount_trigger(bp, DBTR_MODE_U);} else {ret = get_insn_nofault(regs, regs->epc, &insn);if (ret < 0)return ret;
next_addr = get_step_address(regs, insn);
next_addr = get_step_address(regs, insn);
ret = get_insn_nofault(regs, next_addr, &insn);if (ret < 0)return ret;
ret = get_insn_nofault(regs, next_addr, &insn);if (ret < 0)return ret;
bp_insn.bp_type = HW_BREAKPOINT_X;bp_insn.bp_addr = next_addr;/* Get the size of the intruction */bp_insn.bp_len = GET_INSN_LENGTH(insn);
bp_insn.bp_type = HW_BREAKPOINT_X;bp_insn.bp_addr = next_addr;/* Get the size of the intruction */bp_insn.bp_len = GET_INSN_LENGTH(insn);
ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);if (ret)return ret;
ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);if (ret)return ret;} ret = arch_install_hw_breakpoint(event); if (ret)@@ -400,6 +458,79 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) return 0; }
+/**
- icount_triggered - Check if event's icount was triggered.
- @event: Perf event to check
- Check the given perf event's icount breakpoint was triggered.
- Returns: 1 if icount was triggered.
0 if icount was not triggered.
negative on failure.- */
+static int icount_triggered(struct perf_event *event) +{
union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem);struct sbiret ret;struct perf_event **slot;unsigned long tdata1;int i;for (i = 0; i < dbtr_total_num; i++) {slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);if (*slot == event)break;}if (i == dbtr_total_num) {pr_warn("%s: Breakpoint not installed.\n", __func__);return -ENOENT;}raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),*this_cpu_ptr(&ecall_lock_flags));ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,i, 1, 0, 0, 0, 0);tdata1 = shmem->data.tdata1;raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),*this_cpu_ptr(&ecall_lock_flags));if (ret.error) {pr_warn("%s: failed to read trigger. error: %ld\n", __func__, ret.error);return sbi_err_map_linux_errno(ret.error);To avoid a flurry of events or messages, it would probably be good to disable the trigger.
That is a good point.
}/** The RISC-V Debug Specification* Tim Newsome, Paul Donahue (Ventana Micro Systems)* Version 1.0, Revised 2025-02-21: RatifiedI think mentioning the version number and section would be enough.
Agreed.
* 5.7.13. Instruction Count (icount, at 0x7a1)* When count is 1 and the trigger matches, then pending becomes set.* In addition count will become 0 unless it is hard-wired to 1.* When pending is set, the trigger fires just before any further* instructions are executed in a mode where the trigger is enabled.* As the trigger fires, pending is cleared. In addition, if count is* hard-wired to 1 then m, s, u, vs, and vu are all cleared.*/if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) == 0)return 1;if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) != 1)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_U)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_S)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_VU)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_VU)return 0;return 1;+}
/*
- HW Breakpoint/watchpoint handler
*/ @@ -460,7 +591,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
if (bp->in_callback) { expecting_callback = true;
if (regs->epc != bp->next_addr) {
if (have_icount) {if (icount_triggered(event) != 1)continue;} else if (regs->epc != bp->next_addr) { continue; }@@ -477,7 +611,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
}
if (expecting_callback) {
if (expecting_callback && have_icount) {pr_err("%s: in_callback was set, but icount was not triggered, epc (%lx).\n",__func__, regs->epc);} else if (expecting_callback) { pr_err("%s: in_callback was set, but epc (%lx) was not at next address(%lx).\n", __func__, regs->epc, bp->next_addr); }Is this just for debugging or do you want to commit it?
I believe this was intentional, but perhaps it is not a useful print.
- Charlie
Regards Himanshu
-- 2.43.0
On Wed, Aug 27, 2025 at 4:04 AM Charlie Jenkins charlie@rivosinc.com wrote:
On Tue, Aug 26, 2025 at 10:08:04AM +0530, Himanshu Chauhan wrote:
On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube jesse@rivosinc.com wrote:
The Sdtrig RISC-V ISA extension does not have a resume flag for returning to and executing the instruction at the breakpoint. To avoid skipping the instruction or looping, it is necessary to remove the hardware breakpoint and single step. Use the icount feature of Sdtrig to accomplish this. Use icount as default with an option to allow software-based single stepping when hardware or SBI does not have icount functionality, as it may cause unwanted side effects when reading the instruction from memory.
Can you please elaborate on this? I remember noticing the absence of the resume flag which was causing loops.
Yes, signal handler based hardware debugging doesn't work, only ptrace based. ARM64 and ARM also don't support signal handler based hardware debugging as seen in a later patch.
Thank you for your feedback. Jesse's internship came to an end last Friday (August 22nd) so I will be picking up these patches, but I have also added her personal email onto this thread.
I forgot to CC my personal email sorry....
When a breakpoint is triggered and the kernel gains control, the last instruction to execute was the instruction before the instruction where the breakpoint is installed. If execution was to be resumed at this stage, the same breakpoint would be triggered again. So single stepping requires a careful dance of enabling and disabling breakpoints. However, we can avoid this overhead and code complexity can be reduced by using the icount trigger which allows a direct method for single stepping.
Signed-off-by: Jesse Taube jesse@rivosinc.com
OpenSBI implementation of sbi_debug_read_triggers does not return the updated CSR values. There needs to be a check for working sbi_debug_read_triggers before this works.
https://lists.riscv.org/g/tech-prs/message/1476
RFC -> V1:
- Add dbtr_mode to rv_init_icount_trigger
- Add icount_triggered to check which breakpoint was triggered
- Fix typo: s/affects/effects
- Move HW_BREAKPOINT_COMPUTE_STEP to Platform type
V1 -> V2:
- Remove HW_BREAKPOINT_COMPUTE_STEP kconfig option
arch/riscv/kernel/hw_breakpoint.c | 173 ++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 18 deletions(-)
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c index 3f96e744a711..f12306247436 100644 --- a/arch/riscv/kernel/hw_breakpoint.c +++ b/arch/riscv/kernel/hw_breakpoint.c @@ -20,6 +20,7 @@ #define DBTR_TDATA1_DMODE BIT_UL(__riscv_xlen - 5)
#define DBTR_TDATA1_TYPE_MCONTROL (2UL << DBTR_TDATA1_TYPE_SHIFT) +#define DBTR_TDATA1_TYPE_ICOUNT (3UL << DBTR_TDATA1_TYPE_SHIFT) #define DBTR_TDATA1_TYPE_MCONTROL6 (6UL << DBTR_TDATA1_TYPE_SHIFT)
#define DBTR_TDATA1_MCONTROL6_LOAD BIT(0) @@ -62,6 +63,14 @@ (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \ FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi))
+#define DBTR_TDATA1_ICOUNT_U BIT(6) +#define DBTR_TDATA1_ICOUNT_S BIT(7) +#define DBTR_TDATA1_ICOUNT_PENDING BIT(8) +#define DBTR_TDATA1_ICOUNT_M BIT(9) +#define DBTR_TDATA1_ICOUNT_COUNT_FIELD GENMASK(23, 10) +#define DBTR_TDATA1_ICOUNT_VU BIT(25) +#define DBTR_TDATA1_ICOUNT_VS BIT(26)
enum dbtr_mode { DBTR_MODE_U = 0, DBTR_MODE_S, @@ -79,6 +88,7 @@ static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem);
/* number of debug triggers on this cpu . */ static int dbtr_total_num __ro_after_init; +static bool have_icount __ro_after_init; static unsigned long dbtr_type __ro_after_init; static unsigned long dbtr_init __ro_after_init;
@@ -129,6 +139,7 @@ static int arch_smp_teardown_sbi_shmem(unsigned int cpu) static void init_sbi_dbtr(void) { struct sbiret ret;
unsigned long dbtr_count = 0; /* * Called by hw_breakpoint_slots and arch_hw_breakpoint_init.@@ -143,6 +154,19 @@ static void init_sbi_dbtr(void) return; }
ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,DBTR_TDATA1_TYPE_ICOUNT, 0, 0, 0, 0, 0);if (ret.error) {pr_warn("%s: failed to detect icount triggers. error: %ld.\n",__func__, ret.error);} else if (!ret.value) {pr_warn("%s: No icount triggers available. ""Falling-back to computing single step address.\n", __func__);} else {dbtr_count = ret.value;have_icount = true;}ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS, DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0); if (ret.error) {@@ -151,7 +175,7 @@ static void init_sbi_dbtr(void) } else if (!ret.value) { pr_warn("%s: No mcontrol6 triggers available.\n", __func__); } else {
dbtr_total_num = ret.value;
dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6; return; }@@ -166,7 +190,7 @@ static void init_sbi_dbtr(void) pr_err("%s: No mcontrol triggers available.\n", __func__); dbtr_total_num = 0; } else {
dbtr_total_num = ret.value;
dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count); dbtr_type = DBTR_TDATA1_TYPE_MCONTROL; }} @@ -320,6 +344,36 @@ static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr, return 0; }
+static int rv_init_icount_trigger(struct arch_hw_breakpoint *hw, enum dbtr_mode mode) +{
unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT;/* Step one instruction */tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1);switch (mode) {case DBTR_MODE_U:tdata1 |= DBTR_TDATA1_ICOUNT_U;break;case DBTR_MODE_S:tdata1 |= DBTR_TDATA1_ICOUNT_S;break;case DBTR_MODE_VS:tdata1 |= DBTR_TDATA1_ICOUNT_VS;break;case DBTR_MODE_VU:tdata1 |= DBTR_TDATA1_ICOUNT_VU;break;default:return -EINVAL;}hw->tdata1 = tdata1;hw->tdata2 = 0;return 0;+}
int hw_breakpoint_arch_parse(struct perf_event *bp, const struct perf_event_attr *attr, struct arch_hw_breakpoint *hw) @@ -372,24 +426,28 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) /* Remove breakpoint even if return error as not to loop */ arch_uninstall_hw_breakpoint(event);
ret = get_insn_nofault(regs, regs->epc, &insn);if (ret < 0)return ret;
if (have_icount) {rv_init_icount_trigger(bp, DBTR_MODE_U);} else {ret = get_insn_nofault(regs, regs->epc, &insn);if (ret < 0)return ret;
next_addr = get_step_address(regs, insn);
next_addr = get_step_address(regs, insn);
ret = get_insn_nofault(regs, next_addr, &insn);if (ret < 0)return ret;
ret = get_insn_nofault(regs, next_addr, &insn);if (ret < 0)return ret;
bp_insn.bp_type = HW_BREAKPOINT_X;bp_insn.bp_addr = next_addr;/* Get the size of the intruction */bp_insn.bp_len = GET_INSN_LENGTH(insn);
bp_insn.bp_type = HW_BREAKPOINT_X;bp_insn.bp_addr = next_addr;/* Get the size of the intruction */bp_insn.bp_len = GET_INSN_LENGTH(insn);
ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);if (ret)return ret;
ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);if (ret)return ret;} ret = arch_install_hw_breakpoint(event); if (ret)@@ -400,6 +458,79 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs) return 0; }
+/**
- icount_triggered - Check if event's icount was triggered.
- @event: Perf event to check
- Check the given perf event's icount breakpoint was triggered.
- Returns: 1 if icount was triggered.
0 if icount was not triggered.
negative on failure.- */
+static int icount_triggered(struct perf_event *event) +{
union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem);struct sbiret ret;struct perf_event **slot;unsigned long tdata1;int i;for (i = 0; i < dbtr_total_num; i++) {slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);if (*slot == event)break;}if (i == dbtr_total_num) {pr_warn("%s: Breakpoint not installed.\n", __func__);return -ENOENT;}raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),*this_cpu_ptr(&ecall_lock_flags));ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,i, 1, 0, 0, 0, 0);tdata1 = shmem->data.tdata1;raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),*this_cpu_ptr(&ecall_lock_flags));if (ret.error) {pr_warn("%s: failed to read trigger. error: %ld\n", __func__, ret.error);return sbi_err_map_linux_errno(ret.error);To avoid a flurry of events or messages, it would probably be good to disable the trigger.
That is a good point.
Agreed
}/** The RISC-V Debug Specification* Tim Newsome, Paul Donahue (Ventana Micro Systems)* Version 1.0, Revised 2025-02-21: RatifiedI think mentioning the version number and section would be enough.
Agreed.
I wasn't sure the best way to cite this, Version number and section is ok with me.
* 5.7.13. Instruction Count (icount, at 0x7a1)* When count is 1 and the trigger matches, then pending becomes set.* In addition count will become 0 unless it is hard-wired to 1.* When pending is set, the trigger fires just before any further* instructions are executed in a mode where the trigger is enabled.* As the trigger fires, pending is cleared. In addition, if count is* hard-wired to 1 then m, s, u, vs, and vu are all cleared.*/if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) == 0)return 1;if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) != 1)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_U)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_S)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_VU)return 0;if (tdata1 & DBTR_TDATA1_ICOUNT_VU)return 0;return 1;+}
/*
- HW Breakpoint/watchpoint handler
*/ @@ -460,7 +591,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
if (bp->in_callback) { expecting_callback = true;
if (regs->epc != bp->next_addr) {
if (have_icount) {if (icount_triggered(event) != 1)continue;} else if (regs->epc != bp->next_addr) { continue; }@@ -477,7 +611,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs)
}
if (expecting_callback) {
if (expecting_callback && have_icount) {pr_err("%s: in_callback was set, but icount was not triggered, epc (%lx).\n",__func__, regs->epc);} else if (expecting_callback) { pr_err("%s: in_callback was set, but epc (%lx) was not at next address(%lx).\n", __func__, regs->epc, bp->next_addr); }Is this just for debugging or do you want to commit it?
I believe this was intentional, but perhaps it is not a useful print.
It is for someone to find out that a spurious debug trigger event happened as it is expecting either an icount event or a mcontrol event at next_addr if this did not happen it will send a SIG_DEBUG to the user process.
Ignore my previous response as it was HTML.
Thanks, Jesse
- Charlie
Regards Himanshu
-- 2.43.0
Add ability to setup hw breakpoints to ptrace. Call defines a new structure of __riscv_hwdebug_state which will be passed to ptrace.
Signed-off-by: Jesse Taube jesse@rivosinc.com --- RFC -> V1: - Add struct __riscv_hwdebug_state for ptrace_hbp_set/get - Break out ptrace_hbp_set/get so regset can use them - Check for NULL instead of IS_ERR_OR_NULL - Move ptrace_get/sethbpregs above user_regset V1 -> V2: - No change --- arch/riscv/include/asm/processor.h | 4 + arch/riscv/include/uapi/asm/ptrace.h | 9 +++ arch/riscv/kernel/hw_breakpoint.c | 14 +++- arch/riscv/kernel/process.c | 4 + arch/riscv/kernel/ptrace.c | 110 +++++++++++++++++++++++++++ 5 files changed, 140 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index 5f56eb9d114a..488d956a951f 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -12,6 +12,7 @@
#include <vdso/processor.h>
+#include <asm/hw_breakpoint.h> #include <asm/ptrace.h>
#define arch_get_mmap_end(addr, len, flags) \ @@ -108,6 +109,9 @@ struct thread_struct { struct __riscv_v_ext_state vstate; unsigned long align_ctl; struct __riscv_v_ext_state kernel_vstate; +#ifdef CONFIG_HAVE_HW_BREAKPOINT + struct perf_event *ptrace_bps[RV_MAX_TRIGGERS]; +#endif #ifdef CONFIG_SMP /* Flush the icache on migration */ bool force_icache_flush; diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h index a38268b19c3d..20d1aa595cbd 100644 --- a/arch/riscv/include/uapi/asm/ptrace.h +++ b/arch/riscv/include/uapi/asm/ptrace.h @@ -14,6 +14,8 @@
#define PTRACE_GETFDPIC_EXEC 0 #define PTRACE_GETFDPIC_INTERP 1 +#define PTRACE_GETHBPREGS 2 +#define PTRACE_SETHBPREGS 3
/* * User-mode register state for core dumps, ptrace, sigcontext @@ -120,6 +122,13 @@ struct __riscv_v_regset_state { char vreg[]; };
+struct __riscv_hwdebug_state { + unsigned long addr; + unsigned long type; + unsigned long len; + unsigned long ctrl; +} __packed; + /* * According to spec: The number of bits in a single vector register, * VLEN >= ELEN, which must be a power of 2, and must be no greater than diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c index f12306247436..f8841941f2ab 100644 --- a/arch/riscv/kernel/hw_breakpoint.c +++ b/arch/riscv/kernel/hw_breakpoint.c @@ -715,7 +715,19 @@ void arch_uninstall_hw_breakpoint(struct perf_event *event) pr_warn("%s: Failed to uninstall trigger %d. error: %ld\n", __func__, i, ret.error); }
-void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { } +/* + * Release the user breakpoints used by ptrace + */ +void flush_ptrace_hw_breakpoint(struct task_struct *tsk) +{ + int i; + struct thread_struct *t = &tsk->thread; + + for (i = 0; i < dbtr_total_num; i++) { + unregister_hw_breakpoint(t->ptrace_bps[i]); + t->ptrace_bps[i] = NULL; + } +}
void hw_breakpoint_pmu_read(struct perf_event *bp) { }
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 15d8f75902f8..9cf07ecfb523 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -9,6 +9,7 @@
#include <linux/bitfield.h> #include <linux/cpu.h> +#include <linux/hw_breakpoint.h> #include <linux/kernel.h> #include <linux/sched.h> #include <linux/sched/debug.h> @@ -164,6 +165,7 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
void flush_thread(void) { + flush_ptrace_hw_breakpoint(current); #ifdef CONFIG_FPU /* * Reset FPU state and context @@ -218,6 +220,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) set_bit(MM_CONTEXT_LOCK_PMLEN, &p->mm->context.flags);
memset(&p->thread.s, 0, sizeof(p->thread.s)); + if (IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT)) + memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
/* p->thread holds context to be restored by __switch_to() */ if (unlikely(args->fn)) { diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index ea67e9fb7a58..e097e6a61910 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -9,11 +9,13 @@
#include <asm/vector.h> #include <asm/ptrace.h> +#include <asm/hw_breakpoint.h> #include <asm/syscall.h> #include <asm/thread_info.h> #include <asm/switch_to.h> #include <linux/audit.h> #include <linux/compat.h> +#include <linux/hw_breakpoint.h> #include <linux/ptrace.h> #include <linux/elf.h> #include <linux/regset.h> @@ -184,6 +186,104 @@ static int tagged_addr_ctrl_set(struct task_struct *target, } #endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT +static void ptrace_hbptriggered(struct perf_event *bp, + struct perf_sample_data *data, + struct pt_regs *regs) +{ + struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp); + int num = 0; + + force_sig_ptrace_errno_trap(num, (void __user *)bkpt->address); +} + +static int ptrace_hbp_get(struct task_struct *child, unsigned long idx, + struct __riscv_hwdebug_state *state) +{ + struct perf_event *bp; + + if (idx >= RV_MAX_TRIGGERS) + return -EINVAL; + + bp = child->thread.ptrace_bps[idx]; + + if (!bp) + return -ENOENT; + + state->addr = bp->attr.bp_addr; + state->len = bp->attr.bp_len; + state->type = bp->attr.bp_type; + state->ctrl = bp->attr.disabled == 1; + + return 0; +} + +static int ptrace_hbp_set(struct task_struct *child, unsigned long idx, + struct __riscv_hwdebug_state *state) +{ + struct perf_event *bp; + struct perf_event_attr attr; + + if (idx >= RV_MAX_TRIGGERS) + return -EINVAL; + + bp = child->thread.ptrace_bps[idx]; + if (bp) + attr = bp->attr; + else + ptrace_breakpoint_init(&attr); + + attr.bp_addr = state->addr; + attr.bp_len = state->len; + attr.bp_type = state->type; + attr.disabled = state->ctrl == 1; + + if (!bp) { + bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL, + child); + if (IS_ERR(bp)) + return PTR_ERR(bp); + + child->thread.ptrace_bps[idx] = bp; + return 0; + } + + return modify_user_hw_breakpoint(bp, &attr); +} + +/* + * idx selects the breakpoint index. + * Both PTRACE_GETHBPREGS and PTRACE_SETHBPREGS transfer __riscv_hwdebug_state + */ + +static long ptrace_gethbpregs(struct task_struct *child, unsigned long idx, + unsigned long __user *datap) +{ + struct __riscv_hwdebug_state state; + long ret; + + ret = ptrace_hbp_get(child, idx, &state); + if (ret) + return ret; + if (copy_to_user(datap, &state, sizeof(state))) + return -EFAULT; + + return 0; +} + +static long ptrace_sethbpregs(struct task_struct *child, unsigned long idx, + unsigned long __user *datap) +{ + struct __riscv_hwdebug_state state; + + if (copy_from_user(&state, datap, sizeof(state))) + return -EFAULT; + + return ptrace_hbp_set(child, idx, &state); + +} +#endif + static const struct user_regset riscv_user_regset[] = { [REGSET_X] = { .core_note_type = NT_PRSTATUS, @@ -340,8 +440,18 @@ long arch_ptrace(struct task_struct *child, long request, unsigned long addr, unsigned long data) { long ret = -EIO; + unsigned long __user *datap = (unsigned long __user *) data;
switch (request) { +#ifdef CONFIG_HAVE_HW_BREAKPOINT + case PTRACE_GETHBPREGS: + ret = ptrace_gethbpregs(child, addr, datap); + break; + + case PTRACE_SETHBPREGS: + ret = ptrace_sethbpregs(child, addr, datap); + break; +#endif default: ret = ptrace_request(child, request, addr, data); break;
Add ability to setup hw breakpoints using REGSET use the __riscv_hwdebug_state structure to configure breakpoints.
Signed-off-by: Jesse Taube jesse@rivosinc.com --- RFC -> V1: - New commit V1 -> V2: - No change --- arch/riscv/kernel/ptrace.c | 59 ++++++++++++++++++++++++++++++++++ include/uapi/linux/elf.h | 2 ++ tools/include/uapi/linux/elf.h | 1 + 3 files changed, 62 insertions(+)
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index e097e6a61910..fbd0097ec168 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -33,6 +33,9 @@ enum riscv_regset { #ifdef CONFIG_RISCV_ISA_SUPM REGSET_TAGGED_ADDR_CTRL, #endif +#ifdef CONFIG_HAVE_HW_BREAKPOINT + REGSET_HW_BREAK +#endif };
static int riscv_gpr_get(struct task_struct *target, @@ -280,7 +283,53 @@ static long ptrace_sethbpregs(struct task_struct *child, unsigned long idx, return -EFAULT;
return ptrace_hbp_set(child, idx, &state); +}
+static int hw_break_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + struct __riscv_hwdebug_state state; + int ret, idx, offset, limit; + + idx = offset = 0; + limit = regset->n * regset->size; + while (count && offset < limit) { + if (count < sizeof(state)) + return -EINVAL; + + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &state, + offset, offset + sizeof(state)); + if (ret) + return ret; + ret = ptrace_hbp_set(target, idx, &state); + if (ret) + return ret; + offset += sizeof(state); + idx++; + } + + return 0; +} + +static int hw_break_get(struct task_struct *target, + const struct user_regset *regset, + struct membuf to) +{ + int ret, idx = 0; + struct __riscv_hwdebug_state state; + + while (to.left) { + ret = ptrace_hbp_get(target, idx, &state); + if (ret) + return ret; + + membuf_write(&to, &state, sizeof(state)); + idx++; + } + + return 0; } #endif
@@ -324,6 +373,16 @@ static const struct user_regset riscv_user_regset[] = { .set = tagged_addr_ctrl_set, }, #endif +#ifdef CONFIG_HAVE_HW_BREAKPOINT + [REGSET_HW_BREAK] = { + .core_note_type = NT_RISCV_HW_BREAK, + .n = sizeof(struct __riscv_hwdebug_state) / sizeof(unsigned long), + .size = sizeof(unsigned long), + .align = sizeof(unsigned long), + .regset_get = hw_break_get, + .set = hw_break_set, + }, +#endif };
static const struct user_regset_view riscv_user_native_view = { diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index 819ded2d39de..7a32073e0d68 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -545,6 +545,8 @@ typedef struct elf64_shdr { #define NT_RISCV_VECTOR 0x901 /* RISC-V vector registers */ #define NN_RISCV_TAGGED_ADDR_CTRL "LINUX" #define NT_RISCV_TAGGED_ADDR_CTRL 0x902 /* RISC-V tagged address control (prctl()) */ +#define NN_RISCV_HW_BREAK "LINUX" +#define NT_RISCV_HW_BREAK 0x903 /* RISC-V hardware breakpoint registers */ #define NN_LOONGARCH_CPUCFG "LINUX" #define NT_LOONGARCH_CPUCFG 0xa00 /* LoongArch CPU config registers */ #define NN_LOONGARCH_CSR "LINUX" diff --git a/tools/include/uapi/linux/elf.h b/tools/include/uapi/linux/elf.h index 5834b83d7f9a..b5f35df1de7a 100644 --- a/tools/include/uapi/linux/elf.h +++ b/tools/include/uapi/linux/elf.h @@ -460,6 +460,7 @@ typedef struct elf64_shdr { #define NT_RISCV_CSR 0x900 /* RISC-V Control and Status Registers */ #define NT_RISCV_VECTOR 0x901 /* RISC-V vector registers */ #define NT_RISCV_TAGGED_ADDR_CTRL 0x902 /* RISC-V tagged address control (prctl()) */ +#define NT_RISCV_HW_BREAK 0x903 /* RISC-V hardware breakpoint registers */ #define NT_LOONGARCH_CPUCFG 0xa00 /* LoongArch CPU config registers */ #define NT_LOONGARCH_CSR 0xa01 /* LoongArch control and status registers */ #define NT_LOONGARCH_LSX 0xa02 /* LoongArch Loongson SIMD Extension registers */
Add riscv specific selftest for hardware breakpoints. These tests are based on: tools/testing/selftests/breakpoints/breakpoint_test_arm64.c
Signed-off-by: Jesse Taube jesse@rivosinc.com --- The selftest fails as register_user_hw_breakpoint seemingly does not call arch_install_hw_breakpoint. The test also seems to fail on arm64 in the same way when I tested it.
RFC -> V1: - New commit V1 -> V2: - Add KHDR_INCLUDES to Makefile - Change <elf.h> to <linux/elf.h> --- tools/perf/tests/tests.h | 3 +- tools/testing/selftests/riscv/Makefile | 2 +- .../selftests/riscv/breakpoints/.gitignore | 1 + .../selftests/riscv/breakpoints/Makefile | 13 + .../riscv/breakpoints/breakpoint_test.c | 246 ++++++++++++++++++ 5 files changed, 263 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/riscv/breakpoints/.gitignore create mode 100644 tools/testing/selftests/riscv/breakpoints/Makefile create mode 100644 tools/testing/selftests/riscv/breakpoints/breakpoint_test.c
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h index 8aea344536b8..5ff35304c11a 100644 --- a/tools/perf/tests/tests.h +++ b/tools/perf/tests/tests.h @@ -183,7 +183,8 @@ DECLARE_SUITE(util); * Just disable the test for these architectures until these issues are * resolved. */ -#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__) || defined(__aarch64__) +#if defined(__powerpc__) || defined(__s390x__) || defined(__arm__) || defined(__aarch64__) || \ + defined(__riscv) #define BP_SIGNAL_IS_SUPPORTED 0 #else #define BP_SIGNAL_IS_SUPPORTED 1 diff --git a/tools/testing/selftests/riscv/Makefile b/tools/testing/selftests/riscv/Makefile index 099b8c1f46f8..96aba246cb3e 100644 --- a/tools/testing/selftests/riscv/Makefile +++ b/tools/testing/selftests/riscv/Makefile @@ -5,7 +5,7 @@ ARCH ?= $(shell uname -m 2>/dev/null || echo not)
ifneq (,$(filter $(ARCH),riscv)) -RISCV_SUBTARGETS ?= abi hwprobe mm sigreturn vector +RISCV_SUBTARGETS ?= abi hwprobe mm sigreturn vector breakpoints else RISCV_SUBTARGETS := endif diff --git a/tools/testing/selftests/riscv/breakpoints/.gitignore b/tools/testing/selftests/riscv/breakpoints/.gitignore new file mode 100644 index 000000000000..9b3193d06608 --- /dev/null +++ b/tools/testing/selftests/riscv/breakpoints/.gitignore @@ -0,0 +1 @@ +breakpoint_test diff --git a/tools/testing/selftests/riscv/breakpoints/Makefile b/tools/testing/selftests/riscv/breakpoints/Makefile new file mode 100644 index 000000000000..2315d7d874fc --- /dev/null +++ b/tools/testing/selftests/riscv/breakpoints/Makefile @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2021 ARM Limited +# Originally tools/testing/arm64/abi/Makefile + +CFLAGS += -I$(top_srcdir)/tools/include +CFLAGS += $(KHDR_INCLUDES) + +TEST_GEN_PROGS := breakpoint_test + +include ../../lib.mk + +$(OUTPUT)/breakpoint_test: breakpoint_test.c + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ diff --git a/tools/testing/selftests/riscv/breakpoints/breakpoint_test.c b/tools/testing/selftests/riscv/breakpoints/breakpoint_test.c new file mode 100644 index 000000000000..2675055a924e --- /dev/null +++ b/tools/testing/selftests/riscv/breakpoints/breakpoint_test.c @@ -0,0 +1,246 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2016 Google, Inc. + * + * Original Code by Pavel Labath labath@google.com + * + * Code modified by Pratyush Anand panand@redhat.com + * for testing different byte select for each access size. + * Originally tools/testing/selftests/breakpoints/breakpoint_test_arm64.c + */ + +#define _GNU_SOURCE + +#include <asm/ptrace.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/ptrace.h> +#include <sys/param.h> +#include <sys/uio.h> +#include <stdint.h> +#include <stdbool.h> +#include <stddef.h> +#include <string.h> +#include <stdio.h> +#include <unistd.h> +#include <linux/elf.h> +#include <errno.h> +#include <signal.h> + +#include "../../kselftest.h" + +#define MAX_BP_SIZE 8 + +static volatile uint8_t var[3*MAX_BP_SIZE] __attribute__((__aligned__(MAX_BP_SIZE))); + +static void child(int size, int wr) +{ + volatile uint8_t *addr = &var[MAX_BP_SIZE + wr]; + + if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) != 0) { + ksft_print_msg( + "ptrace(PTRACE_TRACEME) failed: %s\n", + strerror(errno)); + _exit(1); + } + + if (raise(SIGSTOP) != 0) { + ksft_print_msg( + "raise(SIGSTOP) failed: %s\n", strerror(errno)); + _exit(1); + } + + if ((uintptr_t) addr % size) { + ksft_print_msg( + "Wrong address write for the given size: %s\n", + strerror(errno)); + _exit(1); + } + + switch (size) { + case 1: + *addr = 47; + break; + case 2: + *(uint16_t *)addr = 47; + break; + case 4: + *(uint32_t *)addr = 47; + break; + case 8: + *(uint64_t *)addr = 47; + break; + } + + _exit(0); +} + +static bool set_watchpoint(pid_t pid, int size, int wp) +{ + const volatile uint8_t *addr = &var[MAX_BP_SIZE + wp]; + const int offset = (uintptr_t)addr % 8; + const unsigned int type = 2; /* Write */ + const unsigned int enable = 1; + struct __riscv_hwdebug_state debug_state; + struct iovec iov; + + memset(&debug_state, 0, sizeof(debug_state)); + debug_state.addr = (uintptr_t)(addr - offset); + debug_state.len = size; + debug_state.ctrl = enable; + debug_state.type = type; + iov.iov_base = &debug_state; + iov.iov_len = sizeof(debug_state); + if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_HW_BREAK, &iov) == 0) + return true; + + if (errno == EIO) + ksft_print_msg( + "ptrace(PTRACE_SETREGSET, NT_RISCV_HW_BREAK) not supported on this hardware: %s\n", + strerror(errno)); + + ksft_print_msg( + "ptrace(PTRACE_SETREGSET, NT_RISCV_HW_BREAK) failed: %s\n", + strerror(errno)); + return false; +} + +static bool run_test(int wr_size, int wp_size, int wr, int wp) +{ + int status; + siginfo_t siginfo; + pid_t pid = fork(); + pid_t wpid; + + if (pid < 0) { + ksft_test_result_fail( + "fork() failed: %s\n", strerror(errno)); + return false; + } + if (pid == 0) + child(wr_size, wr); + + wpid = waitpid(pid, &status, __WALL); + if (wpid != pid) { + ksft_print_msg( + "waitpid() failed: %s\n", strerror(errno)); + return false; + } + if (!WIFSTOPPED(status)) { + ksft_print_msg( + "child did not stop: %s\n", strerror(errno)); + return false; + } + if (WSTOPSIG(status) != SIGSTOP) { + ksft_print_msg("child did not stop with SIGSTOP\n"); + return false; + } + + if (!set_watchpoint(pid, wp_size, wp)) + return false; + + if (ptrace(PTRACE_CONT, pid, NULL, NULL) < 0) { + ksft_print_msg( + "ptrace(PTRACE_CONT) failed: %s\n", + strerror(errno)); + return false; + } + + alarm(3); + wpid = waitpid(pid, &status, __WALL); + if (wpid != pid) { + ksft_print_msg( + "waitpid() failed: %s\n", strerror(errno)); + return false; + } + alarm(0); + if (WIFEXITED(status)) { + ksft_print_msg("child exited prematurely\n"); + return false; + } + if (!WIFSTOPPED(status)) { + ksft_print_msg("child did not stop\n"); + return false; + } + if (WSTOPSIG(status) != SIGTRAP) { + ksft_print_msg("child did not stop with SIGTRAP\n"); + return false; + } + if (ptrace(PTRACE_GETSIGINFO, pid, NULL, &siginfo) != 0) { + ksft_print_msg( + "ptrace(PTRACE_GETSIGINFO): %s\n", + strerror(errno)); + return false; + } + if (siginfo.si_code != TRAP_HWBKPT) { + ksft_print_msg( + "Unexpected si_code %d\n", siginfo.si_code); + return false; + } + + kill(pid, SIGKILL); + wpid = waitpid(pid, &status, 0); + if (wpid != pid) { + ksft_print_msg( + "waitpid() failed: %s\n", strerror(errno)); + return false; + } + return true; +} + +static void sigalrm(int sig) +{ +} + +int main(int argc, char **argv) +{ + int opt; + bool succeeded = true; + struct sigaction act; + int wr, wp, size; + bool result; + + ksft_print_header(); + ksft_set_plan(213); + + act.sa_handler = sigalrm; + sigemptyset(&act.sa_mask); + act.sa_flags = 0; + sigaction(SIGALRM, &act, NULL); + for (size = 1; size <= MAX_BP_SIZE; size = size*2) { + for (wr = 0; wr <= MAX_BP_SIZE; wr = wr + size) { + for (wp = wr - size; wp <= wr + size; wp = wp + size) { + result = run_test(size, MIN(size, 8), wr, wp); + if ((result && wr == wp) || + (!result && wr != wp)) + ksft_test_result_pass( + "Test size = %d write offset = %d watchpoint offset = %d\n", + size, wr, wp); + else { + ksft_test_result_fail( + "Test size = %d write offset = %d watchpoint offset = %d\n", + size, wr, wp); + succeeded = false; + } + } + } + } + + for (size = 1; size <= MAX_BP_SIZE; size = size*2) { + if (run_test(size, 8, -size, -8)) + ksft_test_result_pass( + "Test size = %d write offset = %d watchpoint offset = -8\n", + size, -size); + else { + ksft_test_result_fail( + "Test size = %d write offset = %d watchpoint offset = -8\n", + size, -size); + succeeded = false; + } + } + + if (succeeded) + ksft_exit_pass(); + else + ksft_exit_fail(); +}
On Fri, Aug 22, 2025 at 10:47 AM Jesse Taube jesse@rivosinc.com wrote:
This patchset adds initial support for hardware breakpoints and watchpoints to the RISC-V architecture. The framework is built on top of perf subsystem and SBI debug trigger extension.
Currently following features are not supported and are in works:
- icount for single stepping
- Virtualization of debug triggers
- kernel space debug triggers
The SBI debug trigger extension can be found at: https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-debug-tri...
The Sdtrig ISA is part of RISC-V debug specification which can be found at: https://github.com/riscv/riscv-debug-spec
based off the original RFC by Himanshu Chauhan here: https://lore.kernel.org/lkml/20240222125059.13331-1-hchauhan@ventanamicro.co...
Second RFC by Jesse Taube here: https://lore.kernel.org/lkml/20250722173829.984082-1-jesse@rivosinc.com/
Himanshu Chauhan (2): riscv: Add SBI debug trigger extension and function ids riscv: Introduce support for hardware break/watchpoints
Jesse Taube (6): riscv: Add insn.c, consolidate instruction decoding riscv: insn: Add get_insn_nofault riscv: hw_breakpoint: Use icount for single stepping riscv: ptrace: Add hw breakpoint support riscv: ptrace: Add hw breakpoint regset selftests: riscv: Add test for hardware breakpoints
arch/riscv/Kconfig | 2 + arch/riscv/include/asm/bug.h | 12 - arch/riscv/include/asm/hw_breakpoint.h | 59 ++ arch/riscv/include/asm/insn.h | 132 ++- arch/riscv/include/asm/kdebug.h | 3 +- arch/riscv/include/asm/processor.h | 4 + arch/riscv/include/asm/sbi.h | 33 +- arch/riscv/include/uapi/asm/ptrace.h | 9 + arch/riscv/kernel/Makefile | 2 + arch/riscv/kernel/hw_breakpoint.c | 763 ++++++++++++++++++ arch/riscv/kernel/insn.c | 165 ++++ arch/riscv/kernel/kgdb.c | 102 +-- arch/riscv/kernel/probes/kprobes.c | 1 + arch/riscv/kernel/process.c | 4 + arch/riscv/kernel/ptrace.c | 169 ++++ arch/riscv/kernel/traps.c | 11 +- arch/riscv/kernel/traps_misaligned.c | 93 +-- include/uapi/linux/elf.h | 2 + tools/include/uapi/linux/elf.h | 1 + tools/perf/tests/tests.h | 3 +- tools/testing/selftests/riscv/Makefile | 2 +- .../selftests/riscv/breakpoints/.gitignore | 1 + .../selftests/riscv/breakpoints/Makefile | 13 + .../riscv/breakpoints/breakpoint_test.c | 246 ++++++ 24 files changed, 1641 insertions(+), 191 deletions(-) create mode 100644 arch/riscv/include/asm/hw_breakpoint.h create mode 100644 arch/riscv/kernel/hw_breakpoint.c create mode 100644 arch/riscv/kernel/insn.c create mode 100644 tools/testing/selftests/riscv/breakpoints/.gitignore create mode 100644 tools/testing/selftests/riscv/breakpoints/Makefile create mode 100644 tools/testing/selftests/riscv/breakpoints/breakpoint_test.c
-- 2.43.0
Oops, this meant to be V2.
Thanks, Jesse Taube
linux-kselftest-mirror@lists.linaro.org
-
Anup Patel -
Charlie Jenkins -
Himanshu Chauhan -
Jesse T -
Jesse Taube