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

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 <[email protected]> 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/[email protected]/T/#mb4a939437351b705c30ec7f9e9195d519f9b586f