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

Le 28/09/2022 à 03:30, Benjamin Gray a écrit :

On Tue, 2022-09-27 at 06:40 +0000, Christophe Leroy wrote:

quoted

quoted

+       /* Flush on the EA that may be executed in case of a non-
coherent icache */
+       icbi(prog_addr);

prog_addr is a misleading name ? Is that the address at which you
program it ? Is that the address the programs runs at ?

exec_addr was a lot more explicit as it clearly defines the address
at
which the code is executed.

I'm not sure what it could be confused for other than "the address the
program uses" (be it uses for executing, or uses as data). I just
called it that because it's not necessarily executed, so 'exec_addr' is
misleading (to the extent it matters in the first place...).

For me, at first look it means 'programming address', that is the 
address that is used to perform the programming ie the patching. Hence 
the confusion.

Whereas exec_addr is explicitely for me the address at which the code is 
run.

quoted

quoted

+       if (IS_ENABLED(CONFIG_PPC64) && L1_CACHE_BYTES < 64)
+               icbi(prog_addr + size - 1);

This doesn't exist today.

I'd rather have:

         BUILD_BUG_ON(IS_ENABLED(CONFIG_PPC64) && L1_CACHE_BYTES <
64);

Sure, I can adjust the style.

That's not just style ...

quoted

quoted

+static int __always_inline __do_patch_memory(void *dest, unsigned
long src, size_t size)
   {
         int err;
         u32 *patch_addr;
-       unsigned long text_poke_addr;
         pte_t *pte;
-       unsigned long pfn = get_patch_pfn(addr);
-
-       text_poke_addr = (unsigned
long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;
-       patch_addr = (u32 *)(text_poke_addr +
offset_in_page(addr));
+       unsigned long text_poke_addr = (unsigned
long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK;
+       unsigned long pfn = get_patch_pfn(dest);

+       patch_addr = (u32 *)(text_poke_addr +
offset_in_page(dest));

Can we avoid this churn ?
Ok, you want to change 'addr' to 'dest', can we leave everything else
as
is ?

'addr' was only renamed because the v1 used a pointer to the data, so
'addr' was ambiguous. I'll restore it to 'addr' for v3.

I'll also restore the formatting.

Then maybe the 'src' should be renamed 'val' or something else.

Christophe