On Thu, 2023-02-09 at 07:15 +0000, Christophe Leroy wrote: > > +static inline int patch_uint(u32 *addr, unsigned int val) > > +{ > > + return patch_instruction(addr, ppc_inst(val)); > > Would it make more sense that patch_instruction() calls patch_uint() > instead of the reverse ? >
That's what I had originally, but I figured it would be nicer to see 'patch_instruction' in the disassembly given it's still the main usage. It's equivalent otherwise though. > > diff --git a/arch/powerpc/lib/code-patching.c > > b/arch/powerpc/lib/code-patching.c > > index b00112d7ad46..0f7e9949faf0 100644 > > --- a/arch/powerpc/lib/code-patching.c > > +++ b/arch/powerpc/lib/code-patching.c > > @@ -20,16 +20,14 @@ > > #include <asm/code-patching.h> > > #include <asm/inst.h> > > > > -static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, > > u32 *patch_addr) > > +static int __patch_memory(void *exec_addr, unsigned long val, void > > *patch_addr, > > + bool is_dword) > > { > > - if (!ppc_inst_prefixed(instr)) { > > - u32 val = ppc_inst_val(instr); > > - > > - __put_kernel_nofault(patch_addr, &val, u32, > > failed); > > - } else { > > - u64 val = ppc_inst_as_ulong(instr); > > - > > + if (IS_ENABLED(CONFIG_PPC64) && unlikely(is_dword)) { > > __put_kernel_nofault(patch_addr, &val, u64, > > failed); > > + } else { > > + unsigned int val32 = val; > > Why unsigned int and not u32 as before ? > No particular reason, I just tend to use int/long over 32/64 in code compiled on 32 bit as well and there was a long period of time between removing the original vars and fixing the big endian issue. > > +#ifdef CONFIG_PPC64 > > + > > +int patch_instruction(u32 *addr, ppc_inst_t instr) > > +{ > > + if (ppc_inst_prefixed(instr)) > > + return patch_memory(addr, ppc_inst_as_ulong(instr), > > true); > > + else > > + return patch_memory(addr, ppc_inst_val(instr), > > false); > > +} > > +NOKPROBE_SYMBOL(patch_instruction) > > + > > +int patch_uint(void *addr, unsigned int val) > > +{ > > + return patch_memory(addr, val, false); > > +} > > +NOKPROBE_SYMBOL(patch_uint) > > + > > +int patch_ulong(void *addr, unsigned long val) > > +{ > > + return patch_memory(addr, val, true); > > +} > > +NOKPROBE_SYMBOL(patch_ulong) > > + > > +#else > > + > > +noinline int patch_instruction(u32 *addr, ppc_inst_t instr) > > +{ > > + return patch_memory(addr, ppc_inst_val(instr), false); > > +} > > A comment explaining the reason for the noinline would be welcome > here. Yeah makes sense > By the way, would the noinline change anything on PPC64 ? If not we > could have a common function as ppc_inst_prefixed() constant folds to > false in PPC32. On ppc64 that prevents patch_branch() from calling patch_memory() directly with is_dword=false.