Re: [PATCH] parisc: Fix code/instruction patching on PA1.x machines

[Sven Schnelle <svens@stackframe.org>]

Helge Deller <deller@gmx.de> writes:

> Hi Dave,
>
> On 11/4/21 18:57, John David Anglin wrote:
>> On 2021-11-03 5:08 p.m., Helge Deller wrote:
>>> On 11/3/21 21:12, John David Anglin wrote:
>>>> I think the real problem is that neither flush_kernel_vmap_range() or
>>>> invalidate_kernel_vmap_range() flush the icache.  They only operate
>>>> on the data cache. flush_icache_range will flush both caches.
>>> Yes.
>>> But we write the new instructions to a congruently memory are (same
>>> physical memory like the kernel code), then flush/invalidate the
>>> D-Cache, and finally flush the I-cache of kernel code memory.
>>> See last function call of __patch_text_multiple().
>>>
>>> So, logically I think it should work (and it does on PA2.x).
>>
>> I still believe it is incorrect to use invalidate_kernel_vmap_range() to flush the writes in
>> __patch_text_multiple() to memory.  Both the PA 1.1 and 2.0 architecture documents state that
>> it is implementation dependent whether pdc writes back dirty lines to memory at priority 0.
>> If the writes are not flushed to memory, the patch won't install.
>>
>>> Or do you mean to flush the I-Cache of both mappings?
>>
>> I reviewed the flush operations in __patch_text_multiple().  I believe the current code is more or
>> less correct, but not optimal.  I believe the final call to flush_icache_range() is unnecessary.  We
>> can also eliminate one range flush in the calls to make sure we don't have any aliases in the cache.
>> See change attached below.
>>
>> The big problem with __patch_text_multiple() is can only patch code that the patch code doesn't depend
>> on.  This line in __patch_text_multiple() will cause a TLB fault on the first execution after a new
>> patch_map is setup. We do alternative patching to the TLB handler when ALT_COND_NO_SMP is true.
>
> I believe the alternative code patching isn't critical.
> It just patches in NOPs, so even if the newly patched NOPs aren't visible yet (when the TLB handler is
> executed), it uses the old instructions which shouldn't harm anything. They were executed before the
> whole time.
>
> I do agree, that patching other code paths (with non-NOPs) could be critial.
>
> By the way, to narrow down the problem, I do boot those tests here with the "no-alternatives"
> kernel parameter, which effectively disables the alternatives-patching.
>

I think there are two paths in code patching, that are mixed up in the
text above:

a) __patch_text_multiple(), which is only used by ftrace (and kprobes on
   ftrace). This patches the functions as follows:

   - it patches the trampoline located before the function start
   - after doing so, it patches the branch which sits right at the
     beginning of the function. When removing, it first removes the branch
     and the trampoline code afterwards. This is done via two calls to
     __patch_text_multiple(), which isn't ideal. See
     ftrace_make_nop()/ftrace_make_call()

b) The alternative patching, which replaces code with memcpy(), which
   might suffer from the tlb problem mentioned above.

Sven