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

Conor Dooley
2022-11-29T23:15:02Z

On Tue, Nov 29, 2022 at 11:09:36PM +0000, Conor Dooley wrote:
> 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>

I should've waited for allmodconfig on this particular commit to finish:
/stuff/linux/arch/riscv/kernel/probes/decode-insn.c:43:2: error: call to undeclared function 'riscv_insn_is_auipc'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
RISCV_INSN_SET_SIMULATE(auipc, insn);
^
/stuff/linux/arch/riscv/kernel/probes/simulate-insn.h:20:7: note: expanded from macro 'RISCV_INSN_SET_SIMULATE'
if (riscv_insn_is_##name(code)) { \
^
<scratch space>:62:1: note: expanded from here
riscv_insn_is_auipc
^
1 error generated.