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. > > + > > +/* 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) <[email protected]>
