Re: [PATCH v2 04/13] RISC-V: Move riscv_insn_is_* macros into a common header

Conor Dooley
2022-11-29T23:09:50Z

On Mon, Nov 28, 2022 at 11:26:23AM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
>
> Right now the riscv kernel has (at least) two independent sets
> of functions to check if an encoded instruction is of a specific
> type. One in kgdb and one kprobes simulate-insn code.
>
> More parts of the kernel will probably need this in the future,
> so instead of allowing this duplication to go on further,
> move macros that do the function declaration in a common header,
> similar to at least aarch64.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
> arch/riscv/include/asm/parse_asm.h | 41 ++++++++++++++++----
> arch/riscv/kernel/kgdb.c | 49 ++++++++----------------
> arch/riscv/kernel/probes/simulate-insn.h | 26 +++----------
> 3 files changed, 54 insertions(+), 62 deletions(-)

> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> index cb6ff7dccb92..29fb16cd335c 100644
> --- a/arch/riscv/kernel/probes/simulate-insn.h
> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> @@ -3,14 +3,7 @@
> #ifndef _RISCV_KERNEL_PROBES_SIMULATE_INSN_H
> #define _RISCV_KERNEL_PROBES_SIMULATE_INSN_H
>
> -#define __RISCV_INSN_FUNCS(name, mask, val) \
> -static __always_inline bool riscv_insn_is_##name(probe_opcode_t code) \
> -{ \
> - BUILD_BUG_ON(~(mask) & (val)); \
> - return (code & (mask)) == (val); \
> -} \
> -bool simulate_##name(u32 opcode, unsigned long addr, \
> - struct pt_regs *regs)
> +#include <asm/parse_asm.h>
>
> #define RISCV_INSN_REJECTED(name, code) \
> do { \
> @@ -30,18 +23,9 @@ __RISCV_INSN_FUNCS(fence, 0x7f, 0x0f);
> } \
> } while (0)
>
> -__RISCV_INSN_FUNCS(c_j, 0xe003, 0xa001);
> -__RISCV_INSN_FUNCS(c_jr, 0xf007, 0x8002);
> -__RISCV_INSN_FUNCS(c_jal, 0xe003, 0x2001);
> -__RISCV_INSN_FUNCS(c_jalr, 0xf007, 0x9002);
> -__RISCV_INSN_FUNCS(c_beqz, 0xe003, 0xc001);
> -__RISCV_INSN_FUNCS(c_bnez, 0xe003, 0xe001);
> -__RISCV_INSN_FUNCS(c_ebreak, 0xffff, 0x9002);
> -
> -__RISCV_INSN_FUNCS(auipc, 0x7f, 0x17);
> -__RISCV_INSN_FUNCS(branch, 0x7f, 0x63);
> -
> -__RISCV_INSN_FUNCS(jal, 0x7f, 0x6f);
> -__RISCV_INSN_FUNCS(jalr, 0x707f, 0x67);
> +bool simulate_auipc(u32 opcode, unsigned long addr, struct pt_regs *regs);
> +bool simulate_branch(u32 opcode, unsigned long addr, struct pt_regs *regs);
> +bool simulate_jal(u32 opcode, unsigned long addr, struct pt_regs *regs);
> +bool simulate_jalr(u32 opcode, unsigned long addr, struct pt_regs *regs);

I assume the other ones didn't actually have a user and so didn't need a
function created? Code movement here looks fine. Dropping the double
definitions is always nice :)

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.