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

[Google]

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.

> > +
> > +/* 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'.

[...]
> > +#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.)

> > +	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)

> > +		regs->gpr[rd] = displacement + (regs->pc & (-8192));

Please use nicely named macro instead of -8192.

Thanks,



-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>