Re: [RFC 1/2] openrisc: Add utilities and clean up simulation of instructions

[Sahil]

Hi Masami,

Thank you for your review.

On 4/15/26 12:09 PM, Masami Hiramatsu (Google) wrote:
> On Tue, 14 Apr 2026 18:11:37 +0100
> Stafford Horne <shorne@xxxxxxxxx> wrote:
>
> > Hello Sahil,
> >
> > Thanks for your patches.  I have a few questions. so this is not really a
> > technical review at the moment just some main items.
> >
> > On Wed, Apr 08, 2026 at 12:26:49AM +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.
> > >
> > > Simulate l.adrp. Fix bugs in simulation of l.jal and l.jalr. Earlier,
> >
> > Why emulate l.adrp?  We don't really use this yet in any OpenRISC code.  This
> > instruction is meant to be used to help -fpic code, see:
> >
> >    https://openrisc.io/proposals/ladrp
>
> Maybe because l.adrp depends on InsnAddr (instruction address),
> if kprobe runs copied instruction on its trampoline buffer, it
> generates a wrong result. If it may not be used in the kernel,
> arch_prepare_kprobe() can detect it and return an error.
>
> [...]
> > > +
> > > +#define OPENRISC_INSN_SIZE  (sizeof(union openrisc_instruction))
>
> This should be defined within an architecture header file.

Sorry, I am a little confused here. OPENRISC_INSN_SIZE is in OpenRISC's
instruction definition file (arch/openrisc/include/asm/insn-def.h).

The instruction size for Arm has also been set in it's own insn-def.h
file [1].

> > > +
> > > +/* Helpers for working with l.trap */
> > > +static inline unsigned long __emit_trap(unsigned int code)
> > > +{
> > > +	return (code & 0xffff) | OPCODE_TRAP;
> > > +}
> > > +
> > > +static inline bool has_delay_slot(void)
> > > +{
> > > +	unsigned int cpucfgr = mfspr(SPR_CPUCFGR);
> > > +
> > > +	return !(cpucfgr & SPR_CPUCFGR_ND);
> > > +}
> >
> > This is for handling CPU's that do not have delay slots.  We didn't do this
> > before, why are you doing it now?  Should we mention this in the git commit
> > message?
> >
> > Also, since this is a static configuration for the CPU should we use static keys
> > for this?
> >
> > > +
> > > +void simulate_pc(struct pt_regs *regs, unsigned int jmp);
> > > +void simulate_branch(struct pt_regs *regs, unsigned int jmp, bool has_delay_slot);
>
> Also, do not use the same name for a function name and a local variable.
> Please rename has_delay_slot() to machine_has_delay_slot() or rename
> the has_delay_slot parameter to 'delay_slot'.
>
> [...]

Oh, good catch. I missed this. I'll change this and retest.

> > > +#include <linux/ptrace.h>
> > > +#include <asm/insn-def.h>
> > > +
> > > +void simulate_pc(struct pt_regs *regs, unsigned int jmp)
> > > +{
> > > +	int displacement;
> > > +	unsigned int rd, op;
> > > +
> > > +	displacement = sign_extend32(((jmp) & 0x7ffff) << 13, 31);
> > > +	rd = (jmp & 0x3ffffff) >> 21;
>
> Please use GENMASK() macro instead of hex value.
> (Moreover, define meaningful named macro instead of the magic number.)

Understood, I'll fix this.

> > > +	op = jmp >> 26;
> > > +
> > > +	switch (op) {
> > > +	case l_adrp:
>
> l_adrp seems have 2 modes. Is this OK only supporting 32-bit?
>
> 32-bit Implementation:
> rD[31:0] ← exts(Immediate[18:0] << 13) + (InstAddr & -8192)
> 64-bit Implementation:
> rD[63:0] ← exts(Immediate[20:0] << 13) + (InstAddr & -8192)

I think supporting the 32-bit version should be good enough for the time
being, since OpenRISC's implemention in the kernel is 32-bit. Please see
the following comment from the patch series to support jump labels (published
last year) [2]:

> for us it's more correct to use u32.  I always thing unsigned long is
> OK in openrisc as we have no 64-bit implementations.  But might as well have it
> correct in case we ever add the 64-bit implementation.

I haven't found any OpenRISC patches in the past year that add the 64-bit
implementation.

> > > +		regs->gpr[rd] = displacement + (regs->pc & (-8192));
>
> Please use nicely named macro instead of -8192.

Got it. I'll make this change too.

> Thanks,

Thanks,
Sahil

[1] https://github.com/torvalds/linux/blob/master/arch/arm64/include/asm/insn-def.h#L9
[2] https://lore.kernel.org/openrisc/20250805084114.4125333-2-chenmiao.ku@xxxxxxxxx/T/#mb4a939437351b705c30ec7f9e9195d519f9b586f