Re: [PATCH v6 04/13] riscv/kprobe: Add common RVI and RVC instruction decoder code

[Björn Töpel]

Chen Guokai <chenguokai17@xxxxxxxxxxxxxxxx> writes:

> From: Liao Chang <liaochang1@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Co-developed-by: Chen Guokai <chenguokai17@xxxxxxxxxxxxxxxx>
> Signed-off-by: Chen Guokai <chenguokai17@xxxxxxxxxxxxxxxx>
> ---
>  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