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

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

quoted

+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.

quoted

+       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.

quoted

+               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.

quoted

+       }

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.

quoted

+
+       asm ("dcbst 0, %0; sync" :: "r" (dest));

Maybe write it in C:

        dcbst(dest);
        mb(); /* sync */

quoted

+
+       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.

quoted

+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.

quoted

+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.