Messages in this thread | | | From | Björn Töpel <> | Subject | Re: [PATCH v6 04/13] riscv/kprobe: Add common RVI and RVC instruction decoder code | Date | Wed, 01 Feb 2023 14:29:44 +0100 |
| |
Chen Guokai <chenguokai17@mails.ucas.ac.cn> writes:
> From: Liao Chang <liaochang1@huawei.com> > > These RVI and RVC instruction decoder are used in the free register > searching algorithm, each instruction of instrumented function needs to > decode and test if it contains a free register to form AUIPC/JALR. > > For RVI instruction format, the position and length of rs1/rs2/rd/opcode > parts are uniform [1], but RVC instruction formats are complicated, so > it addresses a series of functions to decode rs1/rs2/rd for RVC [1]. > > [1] https://github.com/riscv/riscv-isa-manual/releases > > Signed-off-by: Liao Chang <liaochang1@huawei.com> > Co-developed-by: Chen Guokai <chenguokai17@mails.ucas.ac.cn> > Signed-off-by: Chen Guokai <chenguokai17@mails.ucas.ac.cn> > --- > arch/riscv/include/asm/bug.h | 5 +- > arch/riscv/kernel/probes/decode-insn.h | 148 +++++++++++++++++++++++ > arch/riscv/kernel/probes/simulate-insn.h | 42 +++++++ > 3 files changed, 194 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h > index 1aaea81fb141..9c33d3b58225 100644 > --- a/arch/riscv/include/asm/bug.h > +++ b/arch/riscv/include/asm/bug.h > @@ -19,11 +19,14 @@ > #define __BUG_INSN_32 _UL(0x00100073) /* ebreak */ > #define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */ > > +#define RVI_INSN_LEN 4UL > +#define RVC_INSN_LEN 2UL > + > #define GET_INSN_LENGTH(insn) \ > ({ \ > unsigned long __len; \ > __len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? \ > - 4UL : 2UL; \ > + RVI_INSN_LEN : RVC_INSN_LEN; \ > __len; \ > }) > > diff --git a/arch/riscv/kernel/probes/decode-insn.h b/arch/riscv/kernel/probes/decode-insn.h > index 42269a7d676d..785b023a62ea 100644 > --- a/arch/riscv/kernel/probes/decode-insn.h > +++ b/arch/riscv/kernel/probes/decode-insn.h > @@ -3,6 +3,7 @@ > #ifndef _RISCV_KERNEL_KPROBES_DECODE_INSN_H > #define _RISCV_KERNEL_KPROBES_DECODE_INSN_H > > +#include <linux/bitops.h> > #include <asm/sections.h> > #include <asm/kprobes.h> > > @@ -15,4 +16,151 @@ enum probe_insn { > enum probe_insn __kprobes > riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *asi); > > +#ifdef CONFIG_KPROBES
No reason to hide the static inlines behind an ifdef; Leave it out, so it's less likely that code breaks slip through.
I wonder if these functions below would make more sense in the asm/insn.h, where riscv_insn_is_##name live (which you're using in later patches). Heiko (Cc'd) recently did a big clean up there, which probably apply to the code below.
> + > +static inline u16 rvi_rs1(kprobe_opcode_t opcode) > +{ > + return (u16)((opcode >> 15) & 0x1f); > +} > + > +static inline u16 rvi_rs2(kprobe_opcode_t opcode) > +{ > + return (u16)((opcode >> 20) & 0x1f); > +} > + > +static inline u16 rvi_rd(kprobe_opcode_t opcode) > +{ > + return (u16)((opcode >> 7) & 0x1f); > +} > + > +static inline s32 rvi_branch_imme(kprobe_opcode_t opcode) > +{ > + u32 imme = 0; > + > + imme |= (((opcode >> 8) & 0xf) << 1) | > + (((opcode >> 25) & 0x3f) << 5) | > + (((opcode >> 7) & 0x1) << 11) | > + (((opcode >> 31) & 0x1) << 12); > + > + return sign_extend32(imme, 13); > +} > + > +static inline s32 rvi_jal_imme(kprobe_opcode_t opcode) > +{ > + u32 imme = 0; > + > + imme |= (((opcode >> 21) & 0x3ff) << 1) | > + (((opcode >> 20) & 0x1) << 11) | > + (((opcode >> 12) & 0xff) << 12) | > + (((opcode >> 31) & 0x1) << 20); > + > + return sign_extend32(imme, 21); > +} > + > +#ifdef CONFIG_RISCV_ISA_C
Dito. Just get rid of the ifdef clutter.
Björn
| |