[PATCH 1/3] powerpc/code-patching: Add generic memory patching

[Benjamin Gray]

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.