[PATCH v1 1/3] powerpc/code-patching: Test patch_instructions() during boot

patch_instructions() introduces new behaviour with a couple of
variations. Test each case of

  * a repeated 32-bit instruction,
  * a repeated 64-bit instruction (ppc64), and
  * a copied sequence of instructions

for both on a single page and when it crosses a page boundary.

Signed-off-by: Benjamin Gray <redacted>
---
 arch/powerpc/lib/test-code-patching.c | 92 +++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/arch/powerpc/lib/test-code-patching.c b/arch/powerpc/lib/test-code-patching.c
index c44823292f73..35a3756272df 100644
--- a/arch/powerpc/lib/test-code-patching.c
+++ b/arch/powerpc/lib/test-code-patching.c

@@ -347,6 +347,97 @@ static void __init test_prefixed_patching(void)
 	check(!memcmp(iptr, expected, sizeof(expected)));
 }
 
+static void __init test_multi_instruction_patching(void)
+{
+	u32 code[256];
+	void *buf;
+	u32 *addr32;
+	u64 *addr64;
+	ppc_inst_t inst64 = ppc_inst_prefix(OP_PREFIX << 26 | 3UL << 24, PPC_RAW_TRAP());
+	u32 inst32 = PPC_RAW_NOP();
+
+	buf = vzalloc(PAGE_SIZE * 8);
+	check(buf);
+	if (!buf)
+		return;
+
+	/* Test single page 32-bit repeated instruction */
+	addr32 = buf + PAGE_SIZE;
+	check(!patch_instructions(addr32 + 1, &inst32, 12, true));
+
+	check(addr32[0] == 0);
+	check(addr32[1] == inst32);
+	check(addr32[2] == inst32);
+	check(addr32[3] == inst32);
+	check(addr32[4] == 0);
+
+	/* Test single page 64-bit repeated instruction */
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		check(ppc_inst_prefixed(inst64));
+
+		addr64 = buf + PAGE_SIZE * 2;
+		ppc_inst_write(code, inst64);
+		check(!patch_instructions((u32 *)(addr64 + 1), code, 24, true));
+
+		check(addr64[0] == 0);
+		check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[1]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[2]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[3]), inst64));
+		check(addr64[4] == 0);
+	}
+
+	/* Test single page memcpy */
+	addr32 = buf + PAGE_SIZE * 3;
+
+	for (int i = 0; i < ARRAY_SIZE(code); i++)
+		code[i] = i + 1;
+
+	check(!patch_instructions(addr32 + 1, code, sizeof(code), false));
+
+	check(addr32[0] == 0);
+	check(!memcmp(&addr32[1], code, sizeof(code)));
+	check(addr32[ARRAY_SIZE(code) + 1] == 0);
+
+	/* Test multipage 32-bit repeated instruction */
+	addr32 = buf + PAGE_SIZE * 4 - 8;
+	check(!patch_instructions(addr32 + 1, &inst32, 12, true));
+
+	check(addr32[0] == 0);
+	check(addr32[1] == inst32);
+	check(addr32[2] == inst32);
+	check(addr32[3] == inst32);
+	check(addr32[4] == 0);
+
+	/* Test multipage 64-bit repeated instruction */
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		check(ppc_inst_prefixed(inst64));
+
+		addr64 = buf + PAGE_SIZE * 5 - 8;
+		ppc_inst_write(code, inst64);
+		check(!patch_instructions((u32 *)(addr64 + 1), code, 24, true));
+
+		check(addr64[0] == 0);
+		check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[1]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[2]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32 *)&addr64[3]), inst64));
+		check(addr64[4] == 0);
+	}
+
+	/* Test multipage memcpy */
+	addr32 = buf + PAGE_SIZE * 6 - 12;
+
+	for (int i = 0; i < ARRAY_SIZE(code); i++)
+		code[i] = i + 1;
+
+	check(!patch_instructions(addr32 + 1, code, sizeof(code), false));
+
+	check(addr32[0] == 0);
+	check(!memcmp(&addr32[1], code, sizeof(code)));
+	check(addr32[ARRAY_SIZE(code) + 1] == 0);
+
+	vfree(buf);
+}
+
 static int __init test_code_patching(void)
 {
 	pr_info("Running code patching self-tests ...\n");

@@ -356,6 +447,7 @@ static int __init test_code_patching(void)
 	test_create_function_call();
 	test_translate_branch();
 	test_prefixed_patching();
+	test_multi_instruction_patching();
 
 	return 0;
 }

-- 
2.44.0

The patching page set up as a writable alias may be in quadrant 1
(userspace) if the temporary mm path is used. This causes sanitiser
failures if so. Sanitiser failures also occur on the non-mm path
because the plain memset family is instrumented, and KASAN treats the
patching window as poisoned.

Introduce locally defined patch_* variants of memset that perform an
uninstrumented lower level set, as well as detecting write errors like
the original single patch variant does.

copy_to_user() is not correct here, as the PTE makes it a proper kernel
page (the EEA is privileged access only, RW). It just happens to be in
quadrant 1 because that's the hardware's mechanism for using the current
PID vs PID 0 in translations. Importantly, it's incorrect to allow user
page accesses.

Now that the patching memsets are used, we also propagate a failure up
to the caller as the single patch variant does.

Signed-off-by: Benjamin Gray <redacted>

---

The patch_memcpy() can be optimised to 4 bytes at a time assuming the
same requirements as regular instruction patching are being followed
for the 'copy sequence of instructions' mode (i.e., they actually are
instructions following instruction alignment rules).
---
 arch/powerpc/lib/code-patching.c | 42 +++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c6ab46156cda..c6633759b509 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c

@@ -372,9 +372,43 @@ int patch_instruction(u32 *addr, ppc_inst_t instr)
 }
 NOKPROBE_SYMBOL(patch_instruction);
 
+static int patch_memset64(u64 *addr, u64 val, size_t count)
+{
+	for (u64 *end = addr + count; addr < end; addr++)
+		__put_kernel_nofault(addr, &val, u64, failed);
+
+	return 0;
+
+failed:
+	return -EPERM;
+}
+
+static int patch_memset32(u32 *addr, u32 val, size_t count)
+{
+	for (u32 *end = addr + count; addr < end; addr++)
+		__put_kernel_nofault(addr, &val, u32, failed);
+
+	return 0;
+
+failed:
+	return -EPERM;
+}
+
+static int patch_memcpy(void *dst, void *src, size_t len)
+{
+	for (void *end = src + len; src < end; dst++, src++)
+		__put_kernel_nofault(dst, src, u8, failed);
+
+	return 0;
+
+failed:
+	return -EPERM;
+}
+
 static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool repeat_instr)
 {
 	unsigned long start = (unsigned long)patch_addr;
+	int err;
 
 	/* Repeat instruction */
 	if (repeat_instr) {

@@ -383,19 +417,19 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
 		if (ppc_inst_prefixed(instr)) {
 			u64 val = ppc_inst_as_ulong(instr);
 
-			memset64((u64 *)patch_addr, val, len / 8);
+			err = patch_memset64((u64 *)patch_addr, val, len / 8);
 		} else {
 			u32 val = ppc_inst_val(instr);
 
-			memset32(patch_addr, val, len / 4);
+			err = patch_memset32(patch_addr, val, len / 4);
 		}
 	} else {
-		memcpy(patch_addr, code, len);
+		err = patch_memcpy(patch_addr, code, len);
 	}
 
 	smp_wmb();	/* smp write barrier */
 	flush_icache_range(start, start + len);
-	return 0;
+	return err;
 }
 
 /*

-- 
2.44.0

As we are patching instructions, we can assume the length is a multiple
of 4 and the destination address is aligned.

Atomicity of patching a prefixed instruction is not a concern, as the
original implementation doesn't provide it anyway.

Signed-off-by: Benjamin Gray <redacted>
---
 arch/powerpc/lib/code-patching.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c6633759b509..ed450a32918c 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c

@@ -394,10 +394,10 @@ static int patch_memset32(u32 *addr, u32 val, size_t count)
 	return -EPERM;
 }
 
-static int patch_memcpy(void *dst, void *src, size_t len)
+static int patch_memcpy32(u32 *dst, u32 *src, size_t count)
 {
-	for (void *end = src + len; src < end; dst++, src++)
-		__put_kernel_nofault(dst, src, u8, failed);
+	for (u32 *end = src + count; src < end; dst++, src++)
+		__put_kernel_nofault(dst, src, u32, failed);
 
 	return 0;
 

@@ -424,7 +424,7 @@ static int __patch_instructions(u32 *patch_addr, u32 *code, size_t len, bool rep
 			err = patch_memset32(patch_addr, val, len / 4);
 		}
 	} else {
-		err = patch_memcpy(patch_addr, code, len);
+		err = patch_memcpy32(patch_addr, code, len / 4);
 	}
 
 	smp_wmb();	/* smp write barrier */

-- 
2.44.0

Also supersedes
https://lore.kernel.org/all/20240213043638.168048-1-bgray@linux.ibm.com/

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

The patching page set up as a writable alias may be in quadrant 1
(userspace) if the temporary mm path is used. This causes sanitiser
failures if so. Sanitiser failures also occur on the non-mm path
because the plain memset family is instrumented, and KASAN treats the
patching window as poisoned.

Introduce locally defined patch_* variants of memset that perform an
uninstrumented lower level set, as well as detecting write errors like
the original single patch variant does.

copy_to_user() is not correct here, as the PTE makes it a proper kernel
page (the EEA is privileged access only, RW). It just happens to be in
quadrant 1 because that's the hardware's mechanism for using the current
PID vs PID 0 in translations. Importantly, it's incorrect to allow user
page accesses.

Now that the patching memsets are used, we also propagate a failure up
to the caller as the single patch variant does.

Signed-off-by: Benjamin Gray <redacted>

---

The patch_memcpy() can be optimised to 4 bytes at a time assuming the
same requirements as regular instruction patching are being followed
for the 'copy sequence of instructions' mode (i.e., they actually are
instructions following instruction alignment rules).

Why not use copy_to_kernel_nofault() ?

Is it correct ? Shouldn't it be -EFAULT ?

Use copy_to_kernel_nofault() instead of open coding a new less optimised 
version of it.

  	}
  
  	smp_wmb();	/* smp write barrier */
  	flush_icache_range(start, start + len);
-	return 0;
+	return err;
  }
  
  /*

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

As we are patching instructions, we can assume the length is a multiple
of 4 and the destination address is aligned.

Atomicity of patching a prefixed instruction is not a concern, as the
original implementation doesn't provide it anyway.

This patch looks unnecessary.

copy_to_kernel_nofault() is what you want to use instead.

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

Build failure:

   CC      arch/powerpc/lib/test-code-patching.o
arch/powerpc/lib/test-code-patching.c: In function 
'test_multi_instruction_patching':
arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size of 
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
   439 | }
       | ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243: 
arch/powerpc/lib/test-code-patching.o] Error 1


I have to avoid big arrays on the stack.

On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

quoted

patch_instructions() introduces new behaviour with a couple of
variations. Test each case of

   * a repeated 32-bit instruction,
   * a repeated 64-bit instruction (ppc64), and
   * a copied sequence of instructions

for both on a single page and when it crosses a page boundary.

Signed-off-by: Benjamin Gray <redacted>
---
  arch/powerpc/lib/test-code-patching.c | 92
+++++++++++++++++++++++++++
  1 file changed, 92 insertions(+)

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

b/arch/powerpc/lib/test-code-patching.c
index c44823292f73..35a3756272df 100644

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

@@ -347,6 +347,97 @@ static void __init

test_prefixed_patching(void)
  	check(!memcmp(iptr, expected, sizeof(expected)));
  }
  
+static void __init test_multi_instruction_patching(void)
+{
+	u32 code[256];

Build failure:

   CC      arch/powerpc/lib/test-code-patching.o
arch/powerpc/lib/test-code-patching.c: In function 
'test_multi_instruction_patching':
arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size of
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
   439 | }
       | ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243: 
arch/powerpc/lib/test-code-patching.o] Error 1


I have to avoid big arrays on the stack.

All good, I can do that.

I do run my patches through a couple of 32-bit configs, but I didn't
see this error. Is this a standard config I should be testing with?

quoted

+	void *buf;
+	u32 *addr32;
+	u64 *addr64;
+	ppc_inst_t inst64 = ppc_inst_prefix(OP_PREFIX << 26 | 3UL
<< 24, PPC_RAW_TRAP());
+	u32 inst32 = PPC_RAW_NOP();
+
+	buf = vzalloc(PAGE_SIZE * 8);
+	check(buf);
+	if (!buf)
+		return;
+
+	/* Test single page 32-bit repeated instruction */
+	addr32 = buf + PAGE_SIZE;
+	check(!patch_instructions(addr32 + 1, &inst32, 12, true));
+
+	check(addr32[0] == 0);
+	check(addr32[1] == inst32);
+	check(addr32[2] == inst32);
+	check(addr32[3] == inst32);
+	check(addr32[4] == 0);
+
+	/* Test single page 64-bit repeated instruction */
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		check(ppc_inst_prefixed(inst64));
+
+		addr64 = buf + PAGE_SIZE * 2;
+		ppc_inst_write(code, inst64);
+		check(!patch_instructions((u32 *)(addr64 + 1),
code, 24, true));
+
+		check(addr64[0] == 0);
+		check(ppc_inst_equal(ppc_inst_read((u32
*)&addr64[1]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32
*)&addr64[2]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32
*)&addr64[3]), inst64));
+		check(addr64[4] == 0);
+	}
+
+	/* Test single page memcpy */
+	addr32 = buf + PAGE_SIZE * 3;
+
+	for (int i = 0; i < ARRAY_SIZE(code); i++)
+		code[i] = i + 1;
+
+	check(!patch_instructions(addr32 + 1, code, sizeof(code),
false));
+
+	check(addr32[0] == 0);
+	check(!memcmp(&addr32[1], code, sizeof(code)));
+	check(addr32[ARRAY_SIZE(code) + 1] == 0);
+
+	/* Test multipage 32-bit repeated instruction */
+	addr32 = buf + PAGE_SIZE * 4 - 8;
+	check(!patch_instructions(addr32 + 1, &inst32, 12, true));
+
+	check(addr32[0] == 0);
+	check(addr32[1] == inst32);
+	check(addr32[2] == inst32);
+	check(addr32[3] == inst32);
+	check(addr32[4] == 0);
+
+	/* Test multipage 64-bit repeated instruction */
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		check(ppc_inst_prefixed(inst64));
+
+		addr64 = buf + PAGE_SIZE * 5 - 8;
+		ppc_inst_write(code, inst64);
+		check(!patch_instructions((u32 *)(addr64 + 1),
code, 24, true));
+
+		check(addr64[0] == 0);
+		check(ppc_inst_equal(ppc_inst_read((u32
*)&addr64[1]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32
*)&addr64[2]), inst64));
+		check(ppc_inst_equal(ppc_inst_read((u32
*)&addr64[3]), inst64));
+		check(addr64[4] == 0);
+	}
+
+	/* Test multipage memcpy */
+	addr32 = buf + PAGE_SIZE * 6 - 12;
+
+	for (int i = 0; i < ARRAY_SIZE(code); i++)
+		code[i] = i + 1;
+
+	check(!patch_instructions(addr32 + 1, code, sizeof(code),
false));
+
+	check(addr32[0] == 0);
+	check(!memcmp(&addr32[1], code, sizeof(code)));
+	check(addr32[ARRAY_SIZE(code) + 1] == 0);
+
+	vfree(buf);
+}
+
  static int __init test_code_patching(void)
  {
  	pr_info("Running code patching self-tests ...\n");

@@ -356,6 +447,7 @@ static int __init test_code_patching(void)

  	test_create_function_call();
  	test_translate_branch();
  	test_prefixed_patching();
+	test_multi_instruction_patching();
  
  	return 0;
  }

On Fri, 2024-03-15 at 06:36 +0000, Christophe Leroy wrote:

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

quoted

The patching page set up as a writable alias may be in quadrant 1
(userspace) if the temporary mm path is used. This causes sanitiser
failures if so. Sanitiser failures also occur on the non-mm path
because the plain memset family is instrumented, and KASAN treats
the
patching window as poisoned.

Introduce locally defined patch_* variants of memset that perform
an
uninstrumented lower level set, as well as detecting write errors
like
the original single patch variant does.

copy_to_user() is not correct here, as the PTE makes it a proper
kernel
page (the EEA is privileged access only, RW). It just happens to be
in
quadrant 1 because that's the hardware's mechanism for using the
current
PID vs PID 0 in translations. Importantly, it's incorrect to allow
user
page accesses.

Now that the patching memsets are used, we also propagate a failure
up
to the caller as the single patch variant does.

Signed-off-by: Benjamin Gray <redacted>

---

The patch_memcpy() can be optimised to 4 bytes at a time assuming
the
same requirements as regular instruction patching are being
followed
for the 'copy sequence of instructions' mode (i.e., they actually
are
instructions following instruction alignment rules).

Why not use copy_to_kernel_nofault() ?

I had not come across copy_to_kernel_nofault(). It looks like the
optimised memcpy() I wanted, so thanks.

quoted

---
  arch/powerpc/lib/code-patching.c | 42
+++++++++++++++++++++++++++++---
  1 file changed, 38 insertions(+), 4 deletions(-)

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

b/arch/powerpc/lib/code-patching.c
index c6ab46156cda..c6633759b509 100644

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

@@ -372,9 +372,43 @@ int patch_instruction(u32 *addr, ppc_inst_t

instr)
  }
  NOKPROBE_SYMBOL(patch_instruction);
  
+static int patch_memset64(u64 *addr, u64 val, size_t count)
+{
+	for (u64 *end = addr + count; addr < end; addr++)
+		__put_kernel_nofault(addr, &val, u64, failed);
+
+	return 0;
+
+failed:
+	return -EPERM;

Is it correct ? Shouldn't it be -EFAULT ?

The single instruction patch returns EPERM, which was set this way to
align with ftrace's expectations. I think it's best to keep the
single/multi patching variants consistent with each other where
possible.

quoted

+}
+
+static int patch_memset32(u32 *addr, u32 val, size_t count)
+{
+	for (u32 *end = addr + count; addr < end; addr++)
+		__put_kernel_nofault(addr, &val, u32, failed);
+
+	return 0;
+
+failed:
+	return -EPERM;
+}
+
+static int patch_memcpy(void *dst, void *src, size_t len)
+{
+	for (void *end = src + len; src < end; dst++, src++)
+		__put_kernel_nofault(dst, src, u8, failed);
+
+	return 0;
+
+failed:
+	return -EPERM;
+}
+
  static int __patch_instructions(u32 *patch_addr, u32 *code,
size_t len, bool repeat_instr)
  {
  	unsigned long start = (unsigned long)patch_addr;
+	int err;
  
  	/* Repeat instruction */
  	if (repeat_instr) {

@@ -383,19 +417,19 @@ static int __patch_instructions(u32

*patch_addr, u32 *code, size_t len, bool rep
  		if (ppc_inst_prefixed(instr)) {
  			u64 val = ppc_inst_as_ulong(instr);
  
-			memset64((u64 *)patch_addr, val, len / 8);
+			err = patch_memset64((u64 *)patch_addr,
val, len / 8);
  		} else {
  			u32 val = ppc_inst_val(instr);
  
-			memset32(patch_addr, val, len / 4);
+			err = patch_memset32(patch_addr, val, len
/ 4);
  		}
  	} else {
-		memcpy(patch_addr, code, len);
+		err = patch_memcpy(patch_addr, code, len);

Use copy_to_kernel_nofault() instead of open coding a new less
optimised 
version of it.

quoted

  	}
  
  	smp_wmb();	/* smp write barrier */
  	flush_icache_range(start, start + len);
-	return 0;
+	return err;
  }
  
  /*

On Fri, 2024-03-15 at 06:39 +0000, Christophe Leroy wrote:

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

quoted

As we are patching instructions, we can assume the length is a
multiple
of 4 and the destination address is aligned.

Atomicity of patching a prefixed instruction is not a concern, as
the
original implementation doesn't provide it anyway.

This patch looks unnecessary.

copy_to_kernel_nofault() is what you want to use instead.

Yeah, I would drop this patch when using copy_to_kernel_nofault()

quoted

Signed-off-by: Benjamin Gray <redacted>
---
  arch/powerpc/lib/code-patching.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

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

b/arch/powerpc/lib/code-patching.c
index c6633759b509..ed450a32918c 100644

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

@@ -394,10 +394,10 @@ static int patch_memset32(u32 *addr, u32 val,

size_t count)
  	return -EPERM;
  }
  
-static int patch_memcpy(void *dst, void *src, size_t len)
+static int patch_memcpy32(u32 *dst, u32 *src, size_t count)
  {
-	for (void *end = src + len; src < end; dst++, src++)
-		__put_kernel_nofault(dst, src, u8, failed);
+	for (u32 *end = src + count; src < end; dst++, src++)
+		__put_kernel_nofault(dst, src, u32, failed);
  
  	return 0;
  

@@ -424,7 +424,7 @@ static int __patch_instructions(u32

*patch_addr, u32 *code, size_t len, bool rep
  			err = patch_memset32(patch_addr, val, len
/ 4);
  		}
  	} else {
-		err = patch_memcpy(patch_addr, code, len);
+		err = patch_memcpy32(patch_addr, code, len / 4);
  	}
  
  	smp_wmb();	/* smp write barrier */

On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:

On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:

quoted

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

quoted

patch_instructions() introduces new behaviour with a couple of
variations. Test each case of

   * a repeated 32-bit instruction,
   * a repeated 64-bit instruction (ppc64), and
   * a copied sequence of instructions

for both on a single page and when it crosses a page boundary.

Signed-off-by: Benjamin Gray <redacted>
---
  arch/powerpc/lib/test-code-patching.c | 92
+++++++++++++++++++++++++++
  1 file changed, 92 insertions(+)

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

b/arch/powerpc/lib/test-code-patching.c
index c44823292f73..35a3756272df 100644

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

@@ -347,6 +347,97 @@ static void __init

test_prefixed_patching(void)
  	check(!memcmp(iptr, expected, sizeof(expected)));
  }
  
+static void __init test_multi_instruction_patching(void)
+{
+	u32 code[256];

Build failure:

   CC      arch/powerpc/lib/test-code-patching.o
arch/powerpc/lib/test-code-patching.c: In function 
'test_multi_instruction_patching':
arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
of
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
   439 | }
       | ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243: 
arch/powerpc/lib/test-code-patching.o] Error 1


I have to avoid big arrays on the stack.

All good, I can do that.

I do run my patches through a couple of 32-bit configs, but I didn't
see this error. Is this a standard config I should be testing with?

Specifically pmac32_defconfig and ppc44x_defconfig

 mjW5LaBPOdGiiDE1w95Ri9HRK27S2dRZpyib9L4mkfYWPAF41mTudjKmVpgtBLO//rO+zmF04OMB/4sWJhLfvhq1CXULDqw5dcuIAIYwf2ughOtyAPFK1ViDcMO5X1bVpNAFO5m4VBpZvFDQ0j0JfqfVBdL68uH05W1/8dMj76RaWj5m0rLM5slY1FQUPddSU+ic9vaZhlDepjU3ZyI8fmioofNGHaxJq6uNTytKdj87kwDV6PQ4hmuGtY56C7JCgjp053sRJ6sXqgKBWfe4ZOJH17mQm+fws93byLoZvvz4Z3im0Rb0MlFo/WirNyhu+TmTNLpnzFUZfenoKrqAkZLY8u1iCFquhgqA321P+sfYew66DtwQmaoi2GKmF89y2enXXzjLNKfLDKkuVoKxFSPeizYqrLi22R9iO8EGBYKACAWIQQ9K5v9I+L06Hi4yOJ5xrdpFsvehAUCYzuwkQIbAgCBCRB5xrdpFsvehHYgBBkRCAAdFiEESFUlaLYscsf4Dt5gaavCcpI6D/8FAmM7sJEACgkQaavCcpI6D/95UgEAqfSj0QhCrYfazQiLDKJstrz3oIKFjhB6+FYMZqt+K1MA/2ioFtHbypeeWbsqYYRhRyTjAKcvE1NZGtH/YWLgkViUidoBAN6gFX/P+VWB77/w8S/BnPmnJx45wmphlkCL8ckOyopFAQCj9eWamHCl2DSaASMSuoZed6C6Gm0OFtuZh/r8K485BQ==
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Date: Mon, 18 Mar 2024 08:55:02 +1100
MIME-Version: 1.0
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
X-Trend-IP-HD: ip=[9.192.253.14]helo={ozlabs.au.ibm.com}sender=(bgray@linux.ibm.com)recipient=<christophe.leroy@csgroup.eu;mpe@ellerman.id.au;linuxppc-dev@lists.ozlabs.org>

On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:

On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:

quoted

=20
=20
Le 15/03/2024 =C3=A0 03:57, Benjamin Gray a =C3=A9crit=C2=A0:

quoted

patch_instructions() introduces new behaviour with a couple of
variations. Test each case of
=20
=C2=A0=C2=A0 * a repeated 32-bit instruction,
=C2=A0=C2=A0 * a repeated 64-bit instruction (ppc64), and
=C2=A0=C2=A0 * a copied sequence of instructions
=20
for both on a single page and when it crosses a page boundary.
=20
Signed-off-by: Benjamin Gray <redacted>
---
=C2=A0 arch/powerpc/lib/test-code-patching.c | 92
+++++++++++++++++++++++++++
=C2=A0 1 file changed, 92 insertions(+)
=20

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

b/arch/powerpc/lib/test-code-patching.c
index c44823292f73..35a3756272df 100644

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

@@ -347,6 +347,97 @@ static void __init

test_prefixed_patching(void)
=C2=A0=C2=A0	check(!memcmp(iptr, expected, sizeof(expected)));
=C2=A0 }
=C2=A0=20
+static void __init test_multi_instruction_patching(void)
+{
+	u32 code[256];

=20
Build failure:
=20
=C2=A0=C2=A0 CC=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 arch/powerpc/lib/test-cod=

e-patching.o

quoted

arch/powerpc/lib/test-code-patching.c: In function=20
'test_multi_instruction_patching':
arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
of
1040 bytes is larger than 1024 bytes [-Werror=3Dframe-larger-than=3D]
=C2=A0=C2=A0 439 | }
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243:=20
arch/powerpc/lib/test-code-patching.o] Error 1
=20
=20
I have to avoid big arrays on the stack.

=20
All good, I can do that.
=20
I do run my patches through a couple of 32-bit configs, but I didn't
see this error. Is this a standard config I should be testing with?
=20

Specifically I build pmac32_defconfig and ppc44x_defconfig

Benjamin Gray [off-list ref] writes:

On Mon, 2024-03-18 at 08:38 +1100, Benjamin Gray wrote:

quoted

On Fri, 2024-03-15 at 07:14 +0000, Christophe Leroy wrote:

quoted

Le 15/03/2024 à 03:57, Benjamin Gray a écrit :

quoted

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

b/arch/powerpc/lib/test-code-patching.c
index c44823292f73..35a3756272df 100644

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

@@ -347,6 +347,97 @@ static void __init

test_prefixed_patching(void)
  	check(!memcmp(iptr, expected, sizeof(expected)));
  }
  
+static void __init test_multi_instruction_patching(void)
+{
+	u32 code[256];

Build failure:

   CC      arch/powerpc/lib/test-code-patching.o
arch/powerpc/lib/test-code-patching.c: In function 
'test_multi_instruction_patching':
arch/powerpc/lib/test-code-patching.c:439:1: error: the frame size
of
1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
   439 | }
       | ^
cc1: all warnings being treated as errors
make[4]: *** [scripts/Makefile.build:243: 
arch/powerpc/lib/test-code-patching.o] Error 1


I have to avoid big arrays on the stack.

All good, I can do that.

I do run my patches through a couple of 32-bit configs, but I didn't
see this error. Is this a standard config I should be testing with?

Specifically pmac32_defconfig and ppc44x_defconfig

Both of those have CONFIG_FRAME_WARN=1024, so should have caught this.

But neither have CONFIG_CODE_PATCHING_SELFTEST=y, so I suspect that's
why you didn't see it.

I recommend ppc32_allmodconfig.

cheers