[PATCH v5 1/5] powerpc/code-patching: introduce patch_instructions()

[Hari Bathini]

Thanks for the review, Song.

On 29/09/23 2:38 am, Song Liu wrote:
> On Thu, Sep 28, 2023 at 12:48 PM Hari Bathini <hbathini at linux.ibm.com> wrote:
>>
>> patch_instruction() entails setting up pte, patching the instruction,
>> clearing the pte and flushing the tlb. If multiple instructions need
>> to be patched, every instruction would have to go through the above
>> drill unnecessarily. Instead, introduce function patch_instructions()
>> that sets up the pte, clears the pte and flushes the tlb only once per
>> page range of instructions to be patched. This adds a slight overhead
>> to patch_instruction() call while improving the patching time for
>> scenarios where more than one instruction needs to be patched.
>>
>> Signed-off-by: Hari Bathini <hbathini at linux.ibm.com>
> 
> Acked-by: Song Liu <song at kernel.org>
> 
> With a nit below.
> 
> [...]
>> +/*
>> + * A page is mapped and instructions that fit the page are patched.
>> + * Assumes 'len' to be (PAGE_SIZE - offset_in_page(addr)) or below.
>> + */
>> +static int __do_patch_instructions_mm(u32 *addr, void *code, size_t len, bool repeat_instr)
>>   {
>>          int err;
>>          u32 *patch_addr;
>> @@ -307,11 +336,15 @@ static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
>>
>>          orig_mm = start_using_temp_mm(patching_mm);
>>
>> -       err = __patch_instruction(addr, instr, patch_addr);
>> +       /* Single instruction case. */
>> +       if (len == 0) {
>> +               err = __patch_instruction(addr, *(ppc_inst_t *)code, patch_addr);
> 
> len == 0 for single instruction is a little weird to me. How about we just use
> len == 4 or 8 depending on the instruction to patch?

Yeah. Looks a bit weird but it avoids the need to call ppc_inst_read()
& ppc_inst_len(). A comment explaining why this weird check could
have been better though..

Thanks
Hari