Re: [PATCH 1/3] powerpc/code-patching: Add generic memory patching

On Thu, 2023-02-09 at 07:15 +0000, Christophe Leroy wrote:

quoted

+static inline int patch_uint(u32 *addr, unsigned int val)
+{
+       return patch_instruction(addr, ppc_inst(val));

Would it make more sense that patch_instruction() calls patch_uint() 
instead of the reverse ?

That's what I had originally, but I figured it would be nicer to see
'patch_instruction' in the disassembly given it's still the main usage.
It's equivalent otherwise though.

quoted

diff --git a/arch/powerpc/lib/code-patching.c

b/arch/powerpc/lib/code-patching.c
index b00112d7ad46..0f7e9949faf0 100644

--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c

@@ -20,16 +20,14 @@

  #include <asm/code-patching.h>
  #include <asm/inst.h>
  
-static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr,
u32 *patch_addr)
+static int __patch_memory(void *exec_addr, unsigned long val, void
*patch_addr,
+                         bool is_dword)
  {
-       if (!ppc_inst_prefixed(instr)) {
-               u32 val = ppc_inst_val(instr);
-
-               __put_kernel_nofault(patch_addr, &val, u32,
failed);
-       } else {
-               u64 val = ppc_inst_as_ulong(instr);
-
+       if (IS_ENABLED(CONFIG_PPC64) && unlikely(is_dword)) {
                __put_kernel_nofault(patch_addr, &val, u64,
failed);
+       } else {
+               unsigned int val32 = val;

Why unsigned int and not u32 as before ?

No particular reason, I just tend to use int/long over 32/64 in code
compiled on 32 bit as well and there was a long period of time between
removing the original vars and fixing the big endian issue.

quoted

+#ifdef CONFIG_PPC64
+
+int patch_instruction(u32 *addr, ppc_inst_t instr)
+{
+       if (ppc_inst_prefixed(instr))
+               return patch_memory(addr, ppc_inst_as_ulong(instr),
true);
+       else
+               return patch_memory(addr, ppc_inst_val(instr),
false);
+}
+NOKPROBE_SYMBOL(patch_instruction)
+
+int patch_uint(void *addr, unsigned int val)
+{
+       return patch_memory(addr, val, false);
+}
+NOKPROBE_SYMBOL(patch_uint)
+
+int patch_ulong(void *addr, unsigned long val)
+{
+       return patch_memory(addr, val, true);
+}
+NOKPROBE_SYMBOL(patch_ulong)
+
+#else
+
+noinline int patch_instruction(u32 *addr, ppc_inst_t instr)
+{
+       return patch_memory(addr, ppc_inst_val(instr), false);
+}

A comment explaining the reason for the noinline would be welcome
here.

Yeah makes sense

By the way, would the noinline change anything on PPC64 ? If not we 
could have a common function as ppc_inst_prefixed() constant folds to
false in PPC32.

On ppc64 that prevents patch_branch() from calling patch_memory()
directly with is_dword=false.