[PATCH 0/3] Add generic data patching functions

[PATCH 0/3] Add generic data patching functions

[Benjamin Gray]

There are issues on ppc64 with using patch_instruction() for arbitrary data.
Add dedicated functions for patching data. More details in the first patch.

Just to explain a couple of quirks:

* ppc32 patch_instruction() is noinline to prevent a mis-optimisation where
  patch_memory() is inlined into patch_branch(), preventing it from being
  inlined into patch_instruction().
* The val32 variable is a big endian workaround. The alternative would be to
  pass the data indirectly through a void*, which would probably not optimise
  as well.
* IS_ENABLED(CONFIG_PPC64) is required to help the ppc32 code optimise out
  the is_dword param.

Benjamin Gray (3):
  powerpc/code-patching: Add generic memory patching
  powerpc/64: Convert patch_instruction() to patch_u32()
  powerpc/32: Convert patch_instruction() to patch_uint()

 arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
 arch/powerpc/kernel/module_64.c          |  5 +-
 arch/powerpc/kernel/static_call.c        |  2 +-
 arch/powerpc/lib/code-patching.c         | 69 +++++++++++++++++-------
 arch/powerpc/platforms/powermac/smp.c    |  2 +-
 5 files changed, 88 insertions(+), 23 deletions(-)


base-commit: 0bfb97203f5f300777624a2ad6f8f84aea3e8658
--
2.39.1

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

[Benjamin Gray]

patch_instruction() is designed for patching instructions in otherwise
readonly memory. Other consumers also sometimes need to patch readonly
memory, so have abused patch_instruction() for arbitrary data patches.

This is a problem on ppc64 as patch_instruction() decides on the patch
width using the 'instruction' opcode to see if it's a prefixed
instruction. Data that triggers this can lead to larger writes, possibly
crossing a page boundary and failing the write altogether.

Introduce patch_uint(), and patch_ulong(), with aliases patch_u32(), and
patch_u64() (on ppc64) designed for aligned data patches. The patch
size is now determined by the called function, and is passed as an
additional parameter to generic internals.

While the instruction flushing is not required for data patches, the
use cases for data patching (mainly module loading and static calls)
are less performance sensitive than for instruction patching
(ftrace activation). So the instruction flushing remains unconditional
in this patch.

ppc32 does not support prefixed instructions, so is unaffected by the
original issue. Care is taken in not exposing the size parameter in the
public (non-static) interface, so the compiler can const-propagate it
away.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

GCC 12.2.1 compiled kernels show the following structure:

- ppc64le_defconfig (ppc64, strict RWX): patch_{uint,ulong,instruction}()
  are small wrappers that tail call into patch_memory()

- ppc_defconfig (ppc32, strict RWX): patch_uint() is the only full RWX
  aware implementation. All use of patch size is eliminated.

  Note: patch_instruction() is marked noinline to prevent making
  patch_instruction() a wrapper of patch_memory(); GCC likes to tail
  call patch_branch() -> patch_memory(), skipping patch_instruction()
  and preventing patch_memory() from inlining inside patch_instruction().

- pmac32_defconfig (ppc32, no strict RWX): patch_uint() and
  raw_patch_instruction() are both implemented but have the same
  ~40 byte function body (as is the case already mind).
---
 arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
 arch/powerpc/lib/code-patching.c         | 69 +++++++++++++++++-------
 2 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 3f881548fb61..89fc4c3f6ecf 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -75,6 +75,39 @@ int patch_branch(u32 *addr, unsigned long target, int flags);
 int patch_instruction(u32 *addr, ppc_inst_t instr);
 int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
 
+/*
+ * patch_uint() and patch_ulong() should only be called on addresses where the
+ * patch does not cross a cacheline, otherwise it may not be flushed properly
+ * and mixes of new and stale data may be observed. It cannot cross a page
+ * boundary, as only the target page is mapped as writable.
+ *
+ * patch_instruction() and other instruction patchers automatically satisfy this
+ * requirement due to instruction alignment requirements.
+ */
+
+#ifdef CONFIG_PPC64
+
+int patch_uint(void *addr, unsigned int val);
+#define patch_u32 patch_uint
+
+int patch_ulong(void *addr, unsigned long val);
+#define patch_u64 patch_ulong
+
+#else
+
+static inline int patch_uint(u32 *addr, unsigned int val)
+{
+	return patch_instruction(addr, ppc_inst(val));
+}
+#define patch_u32 patch_uint
+
+static inline int patch_ulong(void *addr, unsigned long val)
+{
+	return patch_instruction(addr, ppc_inst(val));
+}
+
+#endif
+
 static inline unsigned long patch_site_addr(s32 *site)
 {
 	return (unsigned long)site + *site;
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;
+		__put_kernel_nofault(patch_addr, &val32, u32, failed);
 	}
 
 	asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
@@ -43,7 +41,10 @@ static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr
 
 int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
 {
-	return __patch_instruction(addr, instr, addr);
+	if (ppc_inst_prefixed(instr))
+		return __patch_memory(addr, ppc_inst_as_ulong(instr), addr, true);
+	else
+		return __patch_memory(addr, ppc_inst_val(instr), addr, false);
 }
 
 struct patch_context {
@@ -278,7 +279,7 @@ static void unmap_patch_area(unsigned long addr)
 	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
 }
 
-static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
+static int __do_patch_memory_mm(void *addr, unsigned long val, bool is_dword)
 {
 	int err;
 	u32 *patch_addr;
@@ -307,7 +308,7 @@ 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);
+	err = __patch_memory(addr, val, patch_addr, is_dword);
 
 	/* hwsync performed by __patch_instruction (sync) if successful */
 	if (err)
@@ -328,7 +329,7 @@ static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
 	return err;
 }
 
-static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
+static int __do_patch_memory(void *addr, unsigned long val, bool is_dword)
 {
 	int err;
 	u32 *patch_addr;
@@ -345,7 +346,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
 	if (radix_enabled())
 		asm volatile("ptesync": : :"memory");
 
-	err = __patch_instruction(addr, instr, patch_addr);
+	err = __patch_memory(addr, val, patch_addr, is_dword);
 
 	pte_clear(&init_mm, text_poke_addr, pte);
 	flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
@@ -353,7 +354,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
 	return err;
 }
 
-int patch_instruction(u32 *addr, ppc_inst_t instr)
+static int patch_memory(void *addr, unsigned long val, bool is_dword)
 {
 	int err;
 	unsigned long flags;
@@ -365,18 +366,50 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
 	 */
 	if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) ||
 	    !static_branch_likely(&poking_init_done))
-		return raw_patch_instruction(addr, instr);
+		return __patch_memory(addr, val, addr, is_dword);
 
 	local_irq_save(flags);
 	if (mm_patch_enabled())
-		err = __do_patch_instruction_mm(addr, instr);
+		err = __do_patch_memory_mm(addr, val, is_dword);
 	else
-		err = __do_patch_instruction(addr, instr);
+		err = __do_patch_memory(addr, val, is_dword);
 	local_irq_restore(flags);
 
 	return err;
 }
-NOKPROBE_SYMBOL(patch_instruction);
+
+#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);
+}
+NOKPROBE_SYMBOL(patch_instruction)
+
+#endif
 
 int patch_branch(u32 *addr, unsigned long target, int flags)
 {
-- 
2.39.1

[PATCH 2/3] powerpc/64: Convert patch_instruction() to patch_u32()

[Benjamin Gray]

This use of patch_instruction() is working on 32 bit data, and can fail
if the data looks like a prefixed instruction and the extra write
crosses a page boundary. Use patch_u32() to fix the write size.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

patch_u64() should be more efficient, but judging from the bug report[1]
it doesn't seem like the data is doubleword aligned.

[1]: https://lore.kernel.org/all/20230203004649.1f59dbd4@yea/
---
 arch/powerpc/kernel/module_64.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 2ac78d207f77..a1693d38eb27 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -458,12 +458,11 @@ static inline int create_stub(const Elf64_Shdr *sechdrs,
 	// func_desc_t is 8 bytes if ABIv2, else 16 bytes
 	desc = func_desc(addr);
 	for (i = 0; i < sizeof(func_desc_t) / sizeof(u32); i++) {
-		if (patch_instruction(((u32 *)&entry->funcdata) + i,
-				      ppc_inst(((u32 *)(&desc))[i])))
+		if (patch_u32(((u32 *)&entry->funcdata) + i, ((u32 *)&desc)[i]))
 			return 0;
 	}
 
-	if (patch_instruction(&entry->magic, ppc_inst(STUB_MAGIC)))
+	if (patch_u32(&entry->magic, STUB_MAGIC))
 		return 0;
 
 	return 1;
-- 
2.39.1

[PATCH 3/3] powerpc/32: Convert patch_instruction() to patch_uint()

[Benjamin Gray]

These changes are for patch_instruction() uses on data. Unlike ppc64
these should not be incorrect as-is, but using the patch_uint() alias
better reflects what kind of data being patched and allows for
benchmarking the effect of different patch_* implementations (e.g.,
skipping instruction flushing when patching data).

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/kernel/static_call.c     | 2 +-
 arch/powerpc/platforms/powermac/smp.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/static_call.c b/arch/powerpc/kernel/static_call.c
index 863a7aa24650..1502b7e439ca 100644
--- a/arch/powerpc/kernel/static_call.c
+++ b/arch/powerpc/kernel/static_call.c
@@ -17,7 +17,7 @@ void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
 	mutex_lock(&text_mutex);
 
 	if (func && !is_short) {
-		err = patch_instruction(tramp + PPC_SCT_DATA, ppc_inst(target));
+		err = patch_ulong(tramp + PPC_SCT_DATA, target);
 		if (err)
 			goto out;
 	}
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 5b26a9012d2e..8a8a4688c5f4 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -825,7 +825,7 @@ static int smp_core99_kick_cpu(int nr)
 	mdelay(1);
 
 	/* Restore our exception vector */
-	patch_instruction(vector, ppc_inst(save_vector));
+	patch_uint(vector, save_vector);
 
 	local_irq_restore(flags);
 	if (ppc_md.progress) ppc_md.progress("smp_core99_kick_cpu done", 0x347);
-- 
2.39.1

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

[Christophe Leroy]

Le 07/02/2023 à 02:56, Benjamin Gray a écrit :
> patch_instruction() is designed for patching instructions in otherwise
> readonly memory. Other consumers also sometimes need to patch readonly
> memory, so have abused patch_instruction() for arbitrary data patches.
> 
> This is a problem on ppc64 as patch_instruction() decides on the patch
> width using the 'instruction' opcode to see if it's a prefixed
> instruction. Data that triggers this can lead to larger writes, possibly
> crossing a page boundary and failing the write altogether.
> 
> Introduce patch_uint(), and patch_ulong(), with aliases patch_u32(), and
> patch_u64() (on ppc64) designed for aligned data patches. The patch
> size is now determined by the called function, and is passed as an
> additional parameter to generic internals.
> 
> While the instruction flushing is not required for data patches, the
> use cases for data patching (mainly module loading and static calls)
> are less performance sensitive than for instruction patching
> (ftrace activation). So the instruction flushing remains unconditional
> in this patch.
> 
> ppc32 does not support prefixed instructions, so is unaffected by the
> original issue. Care is taken in not exposing the size parameter in the
> public (non-static) interface, so the compiler can const-propagate it
> away.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> 
> ---
> 
> GCC 12.2.1 compiled kernels show the following structure:
> 
> - ppc64le_defconfig (ppc64, strict RWX): patch_{uint,ulong,instruction}()
>    are small wrappers that tail call into patch_memory()
> 
> - ppc_defconfig (ppc32, strict RWX): patch_uint() is the only full RWX
>    aware implementation. All use of patch size is eliminated.
> 
>    Note: patch_instruction() is marked noinline to prevent making
>    patch_instruction() a wrapper of patch_memory(); GCC likes to tail
>    call patch_branch() -> patch_memory(), skipping patch_instruction()
>    and preventing patch_memory() from inlining inside patch_instruction().
> 
> - pmac32_defconfig (ppc32, no strict RWX): patch_uint() and
>    raw_patch_instruction() are both implemented but have the same
>    ~40 byte function body (as is the case already mind).
> ---
>   arch/powerpc/include/asm/code-patching.h | 33 ++++++++++++
>   arch/powerpc/lib/code-patching.c         | 69 +++++++++++++++++-------
>   2 files changed, 84 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> index 3f881548fb61..89fc4c3f6ecf 100644
> --- a/arch/powerpc/include/asm/code-patching.h
> +++ b/arch/powerpc/include/asm/code-patching.h
> @@ -75,6 +75,39 @@ int patch_branch(u32 *addr, unsigned long target, int flags);
>   int patch_instruction(u32 *addr, ppc_inst_t instr);
>   int raw_patch_instruction(u32 *addr, ppc_inst_t instr);
>   
> +/*
> + * patch_uint() and patch_ulong() should only be called on addresses where the
> + * patch does not cross a cacheline, otherwise it may not be flushed properly
> + * and mixes of new and stale data may be observed. It cannot cross a page
> + * boundary, as only the target page is mapped as writable.
> + *
> + * patch_instruction() and other instruction patchers automatically satisfy this
> + * requirement due to instruction alignment requirements.
> + */
> +
> +#ifdef CONFIG_PPC64
> +
> +int patch_uint(void *addr, unsigned int val);
> +#define patch_u32 patch_uint
> +
> +int patch_ulong(void *addr, unsigned long val);
> +#define patch_u64 patch_ulong
> +
> +#else
> +
> +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 ?

> +}
> +#define patch_u32 patch_uint

You can put this outside the ifdef instead of duplicating it.

> +
> +static inline int patch_ulong(void *addr, unsigned long val)
> +{
> +	return patch_instruction(addr, ppc_inst(val));
> +}
> +
> +#endif
> +
>   static inline unsigned long patch_site_addr(s32 *site)
>   {
>   	return (unsigned long)site + *site;
> 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 ?

> +		__put_kernel_nofault(patch_addr, &val32, u32, failed);
>   	}
>   
>   	asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> @@ -43,7 +41,10 @@ static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr
>   
>   int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
>   {
> -	return __patch_instruction(addr, instr, addr);
> +	if (ppc_inst_prefixed(instr))
> +		return __patch_memory(addr, ppc_inst_as_ulong(instr), addr, true);
> +	else
> +		return __patch_memory(addr, ppc_inst_val(instr), addr, false);
>   }
>   
>   struct patch_context {
> @@ -278,7 +279,7 @@ static void unmap_patch_area(unsigned long addr)
>   	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>   }
>   
> -static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
> +static int __do_patch_memory_mm(void *addr, unsigned long val, bool is_dword)
>   {
>   	int err;
>   	u32 *patch_addr;
> @@ -307,7 +308,7 @@ 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);
> +	err = __patch_memory(addr, val, patch_addr, is_dword);
>   
>   	/* hwsync performed by __patch_instruction (sync) if successful */
>   	if (err)
> @@ -328,7 +329,7 @@ static int __do_patch_instruction_mm(u32 *addr, ppc_inst_t instr)
>   	return err;
>   }
>   
> -static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
> +static int __do_patch_memory(void *addr, unsigned long val, bool is_dword)
>   {
>   	int err;
>   	u32 *patch_addr;
> @@ -345,7 +346,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
>   	if (radix_enabled())
>   		asm volatile("ptesync": : :"memory");
>   
> -	err = __patch_instruction(addr, instr, patch_addr);
> +	err = __patch_memory(addr, val, patch_addr, is_dword);
>   
>   	pte_clear(&init_mm, text_poke_addr, pte);
>   	flush_tlb_kernel_range(text_poke_addr, text_poke_addr + PAGE_SIZE);
> @@ -353,7 +354,7 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr)
>   	return err;
>   }
>   
> -int patch_instruction(u32 *addr, ppc_inst_t instr)
> +static int patch_memory(void *addr, unsigned long val, bool is_dword)
>   {
>   	int err;
>   	unsigned long flags;
> @@ -365,18 +366,50 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
>   	 */
>   	if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) ||
>   	    !static_branch_likely(&poking_init_done))
> -		return raw_patch_instruction(addr, instr);
> +		return __patch_memory(addr, val, addr, is_dword);
>   
>   	local_irq_save(flags);
>   	if (mm_patch_enabled())
> -		err = __do_patch_instruction_mm(addr, instr);
> +		err = __do_patch_memory_mm(addr, val, is_dword);
>   	else
> -		err = __do_patch_instruction(addr, instr);
> +		err = __do_patch_memory(addr, val, is_dword);
>   	local_irq_restore(flags);
>   
>   	return err;
>   }
> -NOKPROBE_SYMBOL(patch_instruction);
> +
> +#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.

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.

> +NOKPROBE_SYMBOL(patch_instruction)
> +
> +#endif
>   
>   int patch_branch(u32 *addr, unsigned long target, int flags)
>   {

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

[Benjamin Gray]

On Thu, 2023-02-09 at 07:15 +0000, Christophe Leroy wrote:
> > +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.

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

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