Re: [PATCH 1/6] powerpc/code-patching: Implement generic text patching function

[Benjamin Gray <bgray@linux.ibm.com>]

On Mon, 2022-09-19 at 06:04 +0000, Christophe Leroy wrote:
> With CONFIG_STRICT_KERNEL_RWX, this patches causes a 15% time
> increase 
> for activation/deactivation of ftrace.

It's possible that new alignment check is the cause. I'll see


> Without CONFIG_STRICT_KERNEL_RWX, it doesn't build.

Yup, fixed for v2

> > +static int __patch_text(void *dest, const void *src, size_t size,
> > bool is_exec, void *exec_addr)
> 
> Is 'text' a good name ? For me text mean executable code. Should it
> be 
> __patch_memory() ?

Well patching regular memory is just a normal store. Text to me implies
its non-writeable. Though __patch_memory would be fine.

> Why pass src as a void * ? This forces data to go via the stack.
> Can't 
> you pass it as a 'long' ?

Probably, I wasn't aware that it would make a difference. I prefer
pointers in general for their semantic meaning, but will change if it
affects param passing.

> > +       if (virt_to_pfn(dest) != virt_to_pfn(dest + size - 1))
> > +               return -EFAULT;
> 
> Why do you need that new check ?

If the patch crosses a page boundary then letting it happen is
unpredictable. Though perhaps this requirement can just be put as a
comment, or require that patches be aligned to the patch size.

> > +               case 8:
> > +                       __put_kernel_nofault(dest, src, u64,
> > failed);
> > +                       break;
> 
> Is case 8 needed for PPC32 ?

I don't have a particular need for it, but the underlying
__put_kernel_nofault is capable of it so I included it.

> > +       }
> 
> Do you catch it when size if none of 1,2,4,8 ?
> 

Not yet. Perhaps I should wrap patch_text_data in a macro that checks
the size with BUILD_BUG_ON? I'd rather not check at runtime.

> > +
> > +       asm ("dcbst 0, %0; sync" :: "r" (dest));
> 
> Maybe write it in C:
> 
>         dcbst(dest);
>         mb(); /* sync */
> 
> > +
> > +       if (is_exec)
> > +               asm ("icbi 0,%0; sync; isync" :: "r" (exec_addr));
> 
> Same, can be:
> 
>         if (is_exec) {
>                 icbi(exec_addr);
>                 mb(); /* sync */
>                 isync();
>         }
> 
> Or keep it flat:
> 
>         if (!is_exec)
>                 return 0;
> 
>         icbi(exec_addr);
>         mb(); /* sync */
>         isync();
> 
>         return 0;

Will try this.

> > +static int do_patch_text(void *dest, const void *src, size_t size,
> > bool is_exec)
> > +{
> > +       int err;
> > +       pte_t *pte;
> > +       u32 *patch_addr;
> > +
> > +       pte = start_text_patch(dest, &patch_addr);
> > +       err = __patch_text(patch_addr, src, size, is_exec, dest);
> > +       finish_text_patch(pte);
> 
> Why do you need to split this function in three parts ? I can't see
> the 
> added value, all it does is reduce readability.

It made it more readable to me, so the __patch_text didn't get buried.
It also made it easier to do the refactoring, and potentially add code
patching variants that use the poke area but not __patch_text. I'll
remove it for v2 though given this is the only use right now.

> Did you check the impact of calling __this_cpu_read() twice ?

I wasn't concerned about performance, but given I'll merge it back
again it will only be read once in v2 again.

> > +void *patch_memory(void *dest, const void *src, size_t size)
> 
> What is this function used for ?
> 

Build failure apparently :)

It's removed in v2.
>