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

[Stafford Horne <shorne@gmail.com>]

On Fri, Nov 13, 2026 at 10:41:56PM +0530, Sahil wrote:
> Hi Stafford,
> 
> On 11/7/26 12:15 PM, Stafford Horne wrote:
> > Hi Sahil,
> > 
> > Thanks for your patches, sorry its taken me time to get around to them, my year
> > since summer has been quite hectic.
> 
> No worries. I completely understand :)
> 
> > I will do the review in chunks so give me some time.
> 
> Thank you for the review.
> 
> > 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/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?
> 
> Sure, I'll rename it.
> 
> > > +{
> > > +	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.
> 
> Since link_offset is initialized to OPENRISC_INSN_SIZE, shifting it to the left by 1 would
> update link_offset to 2 * OPENRISC_INSN_SIZE. This is a little more obscure than explicitly
> setting it to 2 * OPENRISC_INSN_SIZE. Initially, I thought the left shift operator might be
> less expensive than multiplying.
> 
> I was going through mor1kx's design and noticed there's also a pipelined implementation for
> the multiplier [1]. But it looks like the implementation for shifting is simpler and can be done
> in one cycle. Please let me know if my understanding is incorrect here.

That's correct, but my suggestion was to use += OPENRISC_INSN_SIZE instead of
2 *.

> > 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?
> 
> I am not sure if I have understood the alu part. WIll it be more efficient because 2 * OPENRISC_INSN_SIZE
> will be optimized by the compiler to simply be a single number? So no alu operation will be required,
> just one store operation?

Yup, since 2 * OPENRISC_INSN_SIZE is a constant expression the compiler
will replace it with 8.  Then the above code would be just one store operation,
something like:

    if (static_branch_likely(&delay_slot))
	    link_offset = 8;
    else
	    link_offset = 4;


> 
> > > +	switch (op) {
> > > +	case OPENRISC_OPCODE_J: /* l.j */
> > 
> > I think these comments (/* l.j */) are no longer needed sinc you have
> > the named macros.
> 
> Got it, I'll remove the comments.

[snip]

Thanks,
 -Stafford