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

[Christophe Leroy]

Le 28/09/2022 à 03:30, Benjamin Gray a écrit :
> On Tue, 2022-09-27 at 06:40 +0000, Christophe Leroy wrote:
>>> +       /* 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.


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

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