Re: [PATCH v3 1/4] openrisc: Add utilities and clean up simulation of instructions

[Stafford Horne <shorne@gmail.com>]

Hi Sahil,

Thanks for your patches, sorry its taken me time to get around to them, my year
since summer has been quite hectic.

I will do the review in chunks so give me some time.

On Tue, Oct 27, 2026 at 01:42:37AM +0530, Sahil Siddiq wrote:
> Introduce new instruction-related utilities and macros for OpenRISC.
> This is in preparation for patches that add tracing support such as
> KProbes and Kretprobes.
> 
> Currently, PC is first updated and then saved in the link register
> (r9) when simulating l.jal and l.jalr. This can result in a corrupted
> page table (bad page map in process). Fix this by saving PC in r9
> before updating it.
> 
> Move branch simulation to its own function, simulate_branch(), so it
> can be reused. It's already being used by do_illegal_instruction().
> It'll also be used by KProbes in the following commit.
> 
> Introduce simulate_pc() to simulate l.adrp. KProbes will require this
> instruction to be simulated as well. simulate_branch() and simulate_pc()
> need to be handled differently since branch instructions change the
> value of PC while l.adrp depends on PC but doesn't change its value.
> 
> Use a static key to check if delay slots are disabled. Delay slots are
> a part of the CPU's configuration and won't change at runtime. Since
> they are often enabled in an OpenRISC CPU, using a static key will
> reduce the overhead of checking this flag in the default case while
> processing KProbes inserted at branch instructions and simulating them.
> 
> Link: https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.4-rev0.pdf
> Signed-off-by: Sahil Siddiq <sahilcdq0@gmail.com>
> ---
> Changes v2 -> v3:
> - arch/openrisc/kernel/insn.c:
>   - (simulate_pc):
>     Handle case when rd=0
>     Add function to KProbe blacklist
>   - (simulate_branch): Likewise
> - arch/openrisc/kernel/setup.c:
>   - (setup_arch): Set static key before calling paging_init()
> - arch/openrisc/kernel/traps.c:
>   - (do_trap): Move handling of Kprobes in commit #2
> 
> Changes v1 -> v2:
> - arch/openrisc/include/asm/cpuinfo.h: Introduce "delay_slot" static key
> - arch/openrisc/include/asm/insn-def.h:
>    - Use macros instead of union
>    - Use better names for macros
>    - Use genmask() for masks
> - arch/openrisc/kernel/insn.c:
>    - Use macros instead of magic values
>    - (simulate_pc): Update PC
>    - (simulate_branch): Handle rb=r9 in l.jalr simulation
> - arch/openrisc/kernel/setup.c: Use "delay slot" static key
> - arch/openrisc/kernel/traps.c: Use macros instead of magic values
> 
>  arch/openrisc/include/asm/cpuinfo.h  |  3 +
>  arch/openrisc/include/asm/insn-def.h | 58 ++++++++++++++++--
>  arch/openrisc/include/asm/spr_defs.h |  1 +
>  arch/openrisc/kernel/Makefile        |  2 +-
>  arch/openrisc/kernel/insn.c          | 88 ++++++++++++++++++++++++++++
>  arch/openrisc/kernel/setup.c         |  4 ++
>  arch/openrisc/kernel/traps.c         | 53 +++--------------
>  7 files changed, 159 insertions(+), 50 deletions(-)
>  create mode 100644 arch/openrisc/kernel/insn.c
> 
> diff --git a/arch/openrisc/include/asm/cpuinfo.h b/arch/openrisc/include/asm/cpuinfo.h
> index 3cfc4cf0b019..d6bb8744eba0 100644
> --- a/arch/openrisc/include/asm/cpuinfo.h
> +++ b/arch/openrisc/include/asm/cpuinfo.h
> @@ -15,6 +15,7 @@
>  #ifndef __ASM_OPENRISC_CPUINFO_H
>  #define __ASM_OPENRISC_CPUINFO_H
>  
> +#include <linux/jump_label.h>
>  #include <asm/spr.h>
>  #include <asm/spr_defs.h>
>  
> @@ -34,6 +35,8 @@ struct cpuinfo_or1k {
>  	u16 coreid;
>  };
>  
> +DECLARE_STATIC_KEY_TRUE(delay_slot);
> +
>  extern struct cpuinfo_or1k cpuinfo_or1k[NR_CPUS];
>  extern void setup_cpuinfo(void);
>  
> diff --git a/arch/openrisc/include/asm/insn-def.h b/arch/openrisc/include/asm/insn-def.h
> index 1e0c028a5b95..ccbda049171f 100644
> --- a/arch/openrisc/include/asm/insn-def.h
> +++ b/arch/openrisc/include/asm/insn-def.h
> @@ -3,13 +3,63 @@
>   * Copyright (C) 2025 Chen Miao
>   */
>  
> +#include <asm/spr.h>
> +#include <asm/spr_defs.h>
> +#include <linux/bits.h>
> +
>  #ifndef __ASM_OPENRISC_INSN_DEF_H
>  #define __ASM_OPENRISC_INSN_DEF_H
>  
> -/* or1k instructions are always 32 bits. */
> -#define	OPENRISC_INSN_SIZE		4
> -
>  /* or1k nop instruction code */
> -#define OPENRISC_INSN_NOP     0x15000000U
> +#define OPENRISC_INSN_CSYNC	0x23000000U
> +#define OPENRISC_INSN_MACRC	0x18010000U
> +#define OPENRISC_MASK_MACRC	0xfc01ffffU
> +#define OPENRISC_INSN_MSYNC	0x22000000U
> +#define OPENRISC_INSN_NOP	0x15000000U
> +#define OPENRISC_INSN_PSYNC	0x22800000U
> +#define OPENRISC_INSN_SYS	0x20000000U
> +#define OPENRISC_INSN_TRAP	0x21000000U
> +
> +#define OPENRISC_OPCODE_ADRP	0x02
> +#define OPENRISC_OPCODE_BF		0x04
> +#define OPENRISC_OPCODE_BNF		0x03
> +#define OPENRISC_OPCODE_J		0x00
> +#define OPENRISC_OPCODE_JAL		0x01
> +#define OPENRISC_OPCODE_JALR	0x12
> +#define OPENRISC_OPCODE_JR		0x11
> +#define OPENRISC_OPCODE_LWA		0x1b
> +#define OPENRISC_OPCODE_MFSPR	0x2d
> +#define OPENRISC_OPCODE_MTSPR	0x30
> +#define OPENRISC_OPCODE_RFE		0x09
> +#define OPENRISC_OPCODE_SWA		0x33
> +
> +#define OPENRISC_ADRP_IMM_MASK		GENMASK(18, 0)
> +#define OPENRISC_ADRP_OFFSET_MASK	GENMASK(31, 13)
> +#define OPENRISC_ADRP_IMM_SHIFT		13
> +#define OPENRISC_ADRP_REG_MASK		GENMASK(25, 0)
> +#define OPENRISC_ADRP_REG_SHIFT		21
> +#define OPENRISC_JUMP_IMM_MASK		GENMASK(25, 0)
> +#define OPENRISC_JUMP_IMM_SHIFT		2
> +#define OPENRISC_JUMP_REG_MASK		GENMASK(15, 11)
> +#define OPENRISC_JUMP_REG_SHIFT		11
> +
> +#define OPENRISC_MASK_SYS	GENMASK(31, 16)
> +#define OPENRISC_MASK_TRAP	GENMASK(31, 16)
> +#define OPENRISC_MASK_BCODE	GENMASK(15, 0)
> +
> +#define OPENRISC_OPCODE_MASK	0x3f
> +#define OPENRISC_OPCODE_SHIFT	0x1a
> +
> +struct pt_regs;
> +
> +#define OPENRISC_INSN_SIZE	4
> +
> +static __always_inline unsigned char get_6_bit_opcode(unsigned long insn)
> +{
> +	return (insn >> OPENRISC_OPCODE_SHIFT) & OPENRISC_OPCODE_MASK;
> +}
> +
> +void simulate_pc(struct pt_regs *regs, unsigned long jmp);
> +void simulate_branch(struct pt_regs *regs, unsigned long jmp);
>  
>  #endif /* __ASM_OPENRISC_INSN_DEF_H */
> diff --git a/arch/openrisc/include/asm/spr_defs.h b/arch/openrisc/include/asm/spr_defs.h
> index f0b6b492e9f4..5d13a9b96263 100644
> --- a/arch/openrisc/include/asm/spr_defs.h
> +++ b/arch/openrisc/include/asm/spr_defs.h
> @@ -179,6 +179,7 @@
>  #define SPR_CPUCFGR_OF32S  0x00000080  /* ORFPX32 supported */
>  #define SPR_CPUCFGR_OF64S  0x00000100  /* ORFPX64 supported */
>  #define SPR_CPUCFGR_OV64S  0x00000200  /* ORVDX64 supported */
> +#define SPR_CPUCFGR_ND     0x00000400  /* No delay slot */
>  #define SPR_CPUCFGR_RES	   0xfffffc00  /* Reserved */
>  
>  /*
> diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
> index 19e0eb94f2eb..150779fbf010 100644
> --- a/arch/openrisc/kernel/Makefile
> +++ b/arch/openrisc/kernel/Makefile
> @@ -5,7 +5,7 @@
>  
>  always-$(KBUILD_BUILTIN)	:= vmlinux.lds
>  
> -obj-y	:= head.o setup.o or32_ksyms.o process.o dma.o \
> +obj-y	:= head.o insn.o setup.o or32_ksyms.o process.o dma.o \
>  	   traps.o time.o irq.o entry.o ptrace.o signal.o \
>  	   sys_call_table.o unwinder.o cacheinfo.o
>  
> diff --git a/arch/openrisc/kernel/insn.c b/arch/openrisc/kernel/insn.c
> new file mode 100644
> index 000000000000..619d8080d822
> --- /dev/null
> +++ b/arch/openrisc/kernel/insn.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * OpenRISC instruction utils
> + *
> + * Linux architectural port borrowing liberally from similar works of
> + * others.  All original copyrights apply as per the original source
> + * declaration.
> + *
> + * OpenRISC implementation:
> + * Copyright (C) 2026 Sahil Siddiq <sahilcdq0@gmail.com>
> + */
> +
> +#include <linux/kprobes.h>
> +#include <linux/ptrace.h>
> +#include <asm/insn-def.h>
> +#include <asm/cpuinfo.h>
> +
> +DEFINE_STATIC_KEY_TRUE(delay_slot);
> +
> +void simulate_pc(struct pt_regs *regs, unsigned long pc_insn)

Since this simulates l.adrp, maybe the name makes more sense to be called
simulate_adrp?

> +{
> +	int page_offset;
> +	unsigned int rd, op;
> +
> +	page_offset = sign_extend32(
> +		(pc_insn & OPENRISC_ADRP_IMM_MASK) << OPENRISC_ADRP_IMM_SHIFT, 31);
> +	rd = (pc_insn & OPENRISC_ADRP_REG_MASK) >> OPENRISC_ADRP_REG_SHIFT;
> +	op = get_6_bit_opcode(pc_insn);
> +
> +	switch (op) {
> +	case OPENRISC_OPCODE_ADRP:
> +		if (rd != 0)
> +			regs->gpr[rd] = page_offset + (regs->pc & OPENRISC_ADRP_OFFSET_MASK);
> +		regs->pc += OPENRISC_INSN_SIZE;
> +		return;
> +	default:
> +		break;
> +	}
> +}
> +NOKPROBE_SYMBOL(simulate_pc)
> +
> +void simulate_branch(struct pt_regs *regs, unsigned long jmp_insn)
> +{
> +	int displacement;
> +	unsigned int rb, op, link_offset, temp;
> +
> +	displacement = sign_extend32(
> +		((jmp_insn) & OPENRISC_JUMP_IMM_MASK) << OPENRISC_JUMP_IMM_SHIFT, 27);
> +	rb = (jmp_insn & OPENRISC_JUMP_REG_MASK) >> OPENRISC_JUMP_REG_SHIFT;
> +	op = get_6_bit_opcode(jmp_insn);
> +	link_offset = OPENRISC_INSN_SIZE;
> +
> +	if (static_branch_likely(&delay_slot))
> +		link_offset <<= 1;

Why do shift left here instead of += OPENRISC_INSN_SIZE? Since logically we are
adding one more insn for the delay slot, not doubling.

However, It may be more efficient to avoid using the alu and write as:

       if (static_branch_likely(&delay_slot))
               link_offset = 2 * OPENRISC_INSN_SIZE;
       else
               link_offset = OPENRISC_INSN_SIZE;

what do you think?

> +	switch (op) {
> +	case OPENRISC_OPCODE_J: /* l.j */

I think these comments (/* l.j */) are no longer needed sinc you have
the named macros.

> +		regs->pc += displacement;
> +		return;
> +	case OPENRISC_OPCODE_JAL: /* l.jal */
> +		regs->gpr[9] = regs->pc + link_offset;
> +		regs->pc += displacement;
> +		return;
> +	case OPENRISC_OPCODE_BNF: /* l.bnf */
> +		if (regs->sr & SPR_SR_F)
> +			regs->pc += link_offset;
> +		else
> +			regs->pc += displacement;
> +		return;
> +	case OPENRISC_OPCODE_BF: /* l.bf */
> +		if (regs->sr & SPR_SR_F)
> +			regs->pc += displacement;
> +		else
> +			regs->pc += link_offset;
> +		return;
> +	case OPENRISC_OPCODE_JR: /* l.jr */
> +		regs->pc = rb == 0 ? 0 : regs->gpr[rb];
> +		return;
> +	case OPENRISC_OPCODE_JALR: /* l.jalr */
> +		temp = regs->pc + link_offset;
> +		regs->pc = rb == 0 ? 0 : regs->gpr[rb];
> +		regs->gpr[9] = temp;
> +		return;
> +	default:
> +		break;
> +	}
> +}
> +NOKPROBE_SYMBOL(simulate_branch)
> diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
> index 000a9cc10e6f..9c231b320d45 100644
> --- a/arch/openrisc/kernel/setup.c
> +++ b/arch/openrisc/kernel/setup.c
> @@ -252,6 +252,10 @@ void __init setup_arch(char **cmdline_p)
>  	/* perform jump_table sorting before paging_init locks down read only memory */
>  	jump_label_init();
>  
> +	/* determine if the processor supports delay slots */
> +	if (mfspr(SPR_CPUCFGR) & SPR_CPUCFGR_ND)
> +		static_branch_disable(&delay_slot);
> +
>  	/* paging_init() sets up the MMU and marks all pages as reserved */
>  	paging_init();
>  
> diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c
> index c195be9cc9fc..a59abbca3555 100644
> --- a/arch/openrisc/kernel/traps.c
> +++ b/arch/openrisc/kernel/traps.c
> @@ -32,6 +32,7 @@
>  
>  #include <asm/bug.h>
>  #include <asm/fpu.h>
> +#include <asm/insn-def.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
>  #include <asm/unwinder.h>
> @@ -252,12 +253,12 @@ static inline int in_delay_slot(struct pt_regs *regs)
>  	insn = *((unsigned int *)regs->pc);
>  	op = insn >> 26;
>  	switch (op) {
> -	case 0x00: /* l.j */
> -	case 0x01: /* l.jal */
> -	case 0x03: /* l.bnf */
> -	case 0x04: /* l.bf */
> -	case 0x11: /* l.jr */
> -	case 0x12: /* l.jalr */
> +	case OPENRISC_OPCODE_J: /* l.j */
> +	case OPENRISC_OPCODE_JAL: /* l.jal */
> +	case OPENRISC_OPCODE_BNF: /* l.bnf */
> +	case OPENRISC_OPCODE_BF: /* l.bf */
> +	case OPENRISC_OPCODE_JR: /* l.jr */
> +	case OPENRISC_OPCODE_JALR: /* l.jalr */

Nice cleanup, again I think comments can be removed now.

>  		return 1;
>  	default:
>  		return 0;
> @@ -269,47 +270,9 @@ static inline int in_delay_slot(struct pt_regs *regs)
>  
>  static inline void adjust_pc(struct pt_regs *regs, unsigned long address)
>  {
> -	int displacement;
> -	unsigned int rb, op, jmp;
> -
>  	if (unlikely(in_delay_slot(regs))) {
>  		/* In delay slot, instruction at pc is a branch, simulate it */
> -		jmp = *((unsigned int *)regs->pc);
> -
> -		displacement = sign_extend32(((jmp) & 0x3ffffff) << 2, 27);
> -		rb = (jmp & 0x0000ffff) >> 11;
> -		op = jmp >> 26;
> -
> -		switch (op) {
> -		case 0x00: /* l.j */
> -			regs->pc += displacement;
> -			return;
> -		case 0x01: /* l.jal */
> -			regs->pc += displacement;
> -			regs->gpr[9] = regs->pc + 8;
> -			return;
> -		case 0x03: /* l.bnf */
> -			if (regs->sr & SPR_SR_F)
> -				regs->pc += 8;
> -			else
> -				regs->pc += displacement;
> -			return;
> -		case 0x04: /* l.bf */
> -			if (regs->sr & SPR_SR_F)
> -				regs->pc += displacement;
> -			else
> -				regs->pc += 8;
> -			return;
> -		case 0x11: /* l.jr */
> -			regs->pc = regs->gpr[rb];
> -			return;
> -		case 0x12: /* l.jalr */
> -			regs->pc = regs->gpr[rb];
> -			regs->gpr[9] = regs->pc + 8;
> -			return;
> -		default:
> -			break;
> -		}
> +		simulate_branch(regs, *((unsigned int *)regs->pc));
>  	} else {
>  		regs->pc += 4;
>  	}
> -- 
> 2.55.0
>