[PATCH v2 00/13] Implement livepatch on PPC32 and more

[PATCH v2 00/13] Implement livepatch on PPC32 and more

[Christophe Leroy]

This series implements livepatch on PPC32 and implements
CONFIG_DYNAMIC_FTRACE_WITH_ARGS to simplify ftracing.

v2:
- Fix problem with strict modules RWX
- Convert powerpc to CONFIG_DYNAMIC_FTRACE_WITH_ARGS
- Convert function graph tracing to C
- Refactor PPC32 versus PPC64

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Christophe Leroy (13):
  livepatch: Fix build failure on 32 bits processors
  tracing: Fix selftest config check for function graph start up test
  powerpc/module_32: Fix livepatching for RO modules
  powerpc/ftrace: Add support for livepatch to PPC32
  powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32
  powerpc/ftrace: Simplify PPC32's return_to_handler()
  powerpc/ftrace: Prepare PPC32's ftrace_caller() for
    CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  powerpc/ftrace: Prepare PPC64's ftrace_caller() for
    CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller
  powerpc/ftrace: directly call of function graph tracer by ftrace
    caller
  powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
  powerpc/ftrace: Remove ftrace_32.S

 arch/powerpc/Kconfig                          |   7 +-
 arch/powerpc/include/asm/ftrace.h             |  62 +++---
 arch/powerpc/include/asm/livepatch.h          |  12 +-
 arch/powerpc/include/asm/thread_info.h        |   2 +-
 arch/powerpc/kernel/asm-offsets.c             |   2 +-
 arch/powerpc/kernel/module_32.c               |  44 +++--
 arch/powerpc/kernel/trace/Makefile            |   7 +-
 arch/powerpc/kernel/trace/ftrace.c            |  32 +--
 arch/powerpc/kernel/trace/ftrace_32.S         | 187 ------------------
 .../trace/{ftrace_64.S => ftrace_low.S}       |  14 ++
 ...ftrace_64_mprofile.S => ftrace_mprofile.S} | 158 +++++++--------
 kernel/livepatch/core.c                       |   4 +-
 kernel/trace/trace_selftest.c                 |   6 +-
 13 files changed, 178 insertions(+), 359 deletions(-)
 delete mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
 rename arch/powerpc/kernel/trace/{ftrace_64.S => ftrace_low.S} (85%)
 rename arch/powerpc/kernel/trace/{ftrace_64_mprofile.S => ftrace_mprofile.S} (75%)

-- 
2.33.1

[PATCH v2 00/13] Implement livepatch on PPC32 and more

[Christophe Leroy]

This series implements livepatch on PPC32 and implements
CONFIG_DYNAMIC_FTRACE_WITH_ARGS to simplify ftracing.

v2:
- Fix problem with strict modules RWX
- Convert powerpc to CONFIG_DYNAMIC_FTRACE_WITH_ARGS
- Convert function graph tracing to C
- Refactor PPC32 versus PPC64

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Christophe Leroy (13):
  livepatch: Fix build failure on 32 bits processors
  tracing: Fix selftest config check for function graph start up test
  powerpc/module_32: Fix livepatching for RO modules
  powerpc/ftrace: Add support for livepatch to PPC32
  powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32
  powerpc/ftrace: Simplify PPC32's return_to_handler()
  powerpc/ftrace: Prepare PPC32's ftrace_caller() for
    CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  powerpc/ftrace: Prepare PPC64's ftrace_caller() for
    CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
  powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller
  powerpc/ftrace: directly call of function graph tracer by ftrace
    caller
  powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
  powerpc/ftrace: Remove ftrace_32.S

 arch/powerpc/Kconfig                          |   7 +-
 arch/powerpc/include/asm/ftrace.h             |  62 +++---
 arch/powerpc/include/asm/livepatch.h          |  12 +-
 arch/powerpc/include/asm/thread_info.h        |   2 +-
 arch/powerpc/kernel/asm-offsets.c             |   2 +-
 arch/powerpc/kernel/module_32.c               |  44 +++--
 arch/powerpc/kernel/trace/Makefile            |   7 +-
 arch/powerpc/kernel/trace/ftrace.c            |  32 +--
 arch/powerpc/kernel/trace/ftrace_32.S         | 187 ------------------
 .../trace/{ftrace_64.S => ftrace_low.S}       |  14 ++
 ...ftrace_64_mprofile.S => ftrace_mprofile.S} | 158 +++++++--------
 kernel/livepatch/core.c                       |   4 +-
 kernel/trace/trace_selftest.c                 |   6 +-
 13 files changed, 178 insertions(+), 359 deletions(-)
 delete mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
 rename arch/powerpc/kernel/trace/{ftrace_64.S => ftrace_low.S} (85%)
 rename arch/powerpc/kernel/trace/{ftrace_64_mprofile.S => ftrace_mprofile.S} (75%)

-- 
2.33.1

[PATCH v2 01/13] livepatch: Fix build failure on 32 bits processors

[Christophe Leroy]

Trying to build livepatch on powerpc/32 results in:

	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
	      |                       ^
	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
	      |                     ^
	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
	      |                                   ^~~~~~~
	      |                                   |
	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
	      |                                ~~~~~~~~~~~~^~~~~~~

Fix it by using the right types instead of forcing 64 bits types.

Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..c0789383807b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	return -EINVAL;
 }
 
-static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
+static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
 			       unsigned int symndx, Elf_Shdr *relasec,
 			       const char *sec_objname)
 {
@@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
 	relas = (Elf_Rela *) relasec->sh_addr;
 	/* For each rela in this klp relocation section */
 	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
-		sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
+		sym = (Elf_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
 		if (sym->st_shndx != SHN_LIVEPATCH) {
 			pr_err("symbol %s is not marked as a livepatch symbol\n",
 			       strtab + sym->st_name);
-- 
2.33.1

[PATCH v2 01/13] livepatch: Fix build failure on 32 bits processors

[Christophe Leroy]

Trying to build livepatch on powerpc/32 results in:

	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
	      |                       ^
	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
	      |                     ^
	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
	      |                                   ^~~~~~~
	      |                                   |
	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
	      |                                ~~~~~~~~~~~~^~~~~~~

Fix it by using the right types instead of forcing 64 bits types.

Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..c0789383807b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	return -EINVAL;
 }
 
-static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
+static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
 			       unsigned int symndx, Elf_Shdr *relasec,
 			       const char *sec_objname)
 {
@@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
 	relas = (Elf_Rela *) relasec->sh_addr;
 	/* For each rela in this klp relocation section */
 	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
-		sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
+		sym = (Elf_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
 		if (sym->st_shndx != SHN_LIVEPATCH) {
 			pr_err("symbol %s is not marked as a livepatch symbol\n",
 			       strtab + sym->st_name);
-- 
2.33.1

Re: [PATCH v2 01/13] livepatch: Fix build failure on 32 bits processors

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> Trying to build livepatch on powerpc/32 results in:
> 
> 	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> 	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                       ^
> 	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                     ^
> 	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> 	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> 	      |                                   ^~~~~~~
> 	      |                                   |
> 	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
> 	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> 	      |                                ~~~~~~~~~~~~^~~~~~~
> 
> Fix it by using the right types instead of forcing 64 bits types.
> 
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Acked-by: Petr Mladek <pmladek@suse.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH v2 01/13] livepatch: Fix build failure on 32 bits processors

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> Trying to build livepatch on powerpc/32 results in:
> 
> 	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> 	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                       ^
> 	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                     ^
> 	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> 	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> 	      |                                   ^~~~~~~
> 	      |                                   |
> 	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
> 	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> 	      |                                ~~~~~~~~~~~~^~~~~~~
> 
> Fix it by using the right types instead of forcing 64 bits types.
> 
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Acked-by: Petr Mladek <pmladek@suse.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH v2 01/13] livepatch: Fix build failure on 32 bits processors

[Joe Lawrence]

On Mon, Dec 20, 2021 at 04:38:02PM +0000, Christophe Leroy wrote:
> Trying to build livepatch on powerpc/32 results in:
> 
> 	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> 	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                       ^
> 	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                     ^
> 	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> 	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> 	      |                                   ^~~~~~~
> 	      |                                   |
> 	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
> 	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> 	      |                                ~~~~~~~~~~~~^~~~~~~
> 
> Fix it by using the right types instead of forcing 64 bits types.
> 
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Acked-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..c0789383807b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> +static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
>  			       unsigned int symndx, Elf_Shdr *relasec,
>  			       const char *sec_objname)
>  {
> @@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
>  	relas = (Elf_Rela *) relasec->sh_addr;
>  	/* For each rela in this klp relocation section */
>  	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> -		sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> +		sym = (Elf_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
>  		if (sym->st_shndx != SHN_LIVEPATCH) {
>  			pr_err("symbol %s is not marked as a livepatch symbol\n",
>  			       strtab + sym->st_name);
> -- 
> 2.33.1
> 

Thanks for finding and fixing, lgtm.

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH v2 01/13] livepatch: Fix build failure on 32 bits processors

[Joe Lawrence]

On Mon, Dec 20, 2021 at 04:38:02PM +0000, Christophe Leroy wrote:
> Trying to build livepatch on powerpc/32 results in:
> 
> 	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> 	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                       ^
> 	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                     ^
> 	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> 	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> 	      |                                   ^~~~~~~
> 	      |                                   |
> 	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
> 	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> 	      |                                ~~~~~~~~~~~~^~~~~~~
> 
> Fix it by using the right types instead of forcing 64 bits types.
> 
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Acked-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..c0789383807b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	return -EINVAL;
>  }
>  
> -static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> +static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
>  			       unsigned int symndx, Elf_Shdr *relasec,
>  			       const char *sec_objname)
>  {
> @@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
>  	relas = (Elf_Rela *) relasec->sh_addr;
>  	/* For each rela in this klp relocation section */
>  	for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
> -		sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> +		sym = (Elf_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
>  		if (sym->st_shndx != SHN_LIVEPATCH) {
>  			pr_err("symbol %s is not marked as a livepatch symbol\n",
>  			       strtab + sym->st_name);
> -- 
> 2.33.1
> 

Thanks for finding and fixing, lgtm.

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

[PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Christophe Leroy]

CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
direct tramp.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/trace/trace_selftest.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index afd937a46496..abcadbe933bb 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -784,9 +784,7 @@ static struct fgraph_ops fgraph_ops __initdata  = {
 	.retfunc		= &trace_graph_return,
 };
 
-#if defined(CONFIG_DYNAMIC_FTRACE) && \
-    defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
-#define TEST_DIRECT_TRAMP
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 noinline __noclone static void trace_direct_tramp(void) { }
 #endif
 
@@ -849,7 +847,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
 		goto out;
 	}
 
-#ifdef TEST_DIRECT_TRAMP
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	tracing_reset_online_cpus(&tr->array_buffer);
 	set_graph_array(tr);
 
-- 
2.33.1

[PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Christophe Leroy]

CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
direct tramp.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/trace/trace_selftest.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index afd937a46496..abcadbe933bb 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -784,9 +784,7 @@ static struct fgraph_ops fgraph_ops __initdata  = {
 	.retfunc		= &trace_graph_return,
 };
 
-#if defined(CONFIG_DYNAMIC_FTRACE) && \
-    defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
-#define TEST_DIRECT_TRAMP
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 noinline __noclone static void trace_direct_tramp(void) { }
 #endif
 
@@ -849,7 +847,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
 		goto out;
 	}
 
-#ifdef TEST_DIRECT_TRAMP
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	tracing_reset_online_cpus(&tr->array_buffer);
 	set_graph_array(tr);
 
-- 
2.33.1

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Christophe Leroy]

Hi Michael,

Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
> direct tramp.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

You didn't apply this patch when you merged the series. Without it I get 
the following :

[    6.191287] Testing ftrace recursion: PASSED
[    6.473308] Testing ftrace recursion safe: PASSED
[    6.755759] Testing ftrace regs: PASSED
[    7.037994] Testing tracer nop: PASSED
[    7.042256] Testing tracer function_graph: FAILED!
[   12.216112] ------------[ cut here ]------------
[   12.220436] WARNING: CPU: 0 PID: 1 at kernel/trace/trace.c:1953 
run_tracer_selftest+0x138/0x1b4
[   12.229045] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.17.0-rc2-s3k-dev-02096-g28b040bd2357 #1030
[   12.237735] NIP:  c00d01b4 LR: c00d01b4 CTR: c03d37fc
[   12.242724] REGS: c902bd90 TRAP: 0700   Not tainted 
(5.17.0-rc2-s3k-dev-02096-g28b040bd2357)
[   12.251157] MSR:  00029032 <EE,ME,IR,DR,RI>  CR: 28000242  XER: 00000000
[   12.257870]
[   12.257870] GPR00: c00d01b4 c902be50 c2140000 00000007 c108d224 
00000001 c11ed2e8 c108d340
[   12.257870] GPR08: 3fffbfff 00000000 c129beac 3fffc000 22000244 
00000000 c0004b78 00000000
[   12.257870] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000000 00000000 c1039020
[   12.257870] GPR24: c12d0000 c1000144 c1223c48 c12b53c4 c12b55dc 
c1293118 fffffdf4 c1223c38
[   12.293843] NIP [c00d01b4] run_tracer_selftest+0x138/0x1b4
[   12.299265] LR [c00d01b4] run_tracer_selftest+0x138/0x1b4
[   12.304603] Call Trace:
[   12.307012] [c902be50] [c00d01b4] run_tracer_selftest+0x138/0x1b4 
(unreliable)
[   12.314155] [c902be70] [c100cf44] register_tracer+0x14c/0x218
[   12.319835] [c902be90] [c10011a0] do_one_initcall+0x8c/0x17c
[   12.325430] [c902bef0] [c10014c0] kernel_init_freeable+0x1a8/0x254
[   12.331540] [c902bf20] [c0004ba8] kernel_init+0x30/0x150
[   12.336789] [c902bf30] [c001222c] ret_from_kernel_thread+0x5c/0x64
[   12.342902] Instruction dump:
[   12.345828] 4bf9a135 813d0030 7fc4f378 7d2903a6 7fa3eb78 4e800421 
7c7e1b79 939f0f60
[   12.353657] 41820014 3c60c08a 3863644c 4bf9a109 <0fe00000> 387f00b0 
4bff76bd 893d0052
[   12.361659] ---[ end trace 0000000000000000 ]---


With the patch I get:

[    6.191286] Testing ftrace recursion: PASSED
[    6.473307] Testing ftrace recursion safe: PASSED
[    6.755758] Testing ftrace regs: PASSED
[    7.037993] Testing tracer nop: PASSED
[    7.042255] Testing tracer function_graph: PASSED

Is this patch going to be merged via another tree ?

Thanks
Christophe


> ---
>   kernel/trace/trace_selftest.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index afd937a46496..abcadbe933bb 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -784,9 +784,7 @@ static struct fgraph_ops fgraph_ops __initdata  = {
>   	.retfunc		= &trace_graph_return,
>   };
>   
> -#if defined(CONFIG_DYNAMIC_FTRACE) && \
> -    defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
> -#define TEST_DIRECT_TRAMP
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>   noinline __noclone static void trace_direct_tramp(void) { }
>   #endif
>   
> @@ -849,7 +847,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
>   		goto out;
>   	}
>   
> -#ifdef TEST_DIRECT_TRAMP
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>   	tracing_reset_online_cpus(&tr->array_buffer);
>   	set_graph_array(tr);
>   

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Christophe Leroy]

Hi Michael,

Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
> direct tramp.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

You didn't apply this patch when you merged the series. Without it I get 
the following :

[    6.191287] Testing ftrace recursion: PASSED
[    6.473308] Testing ftrace recursion safe: PASSED
[    6.755759] Testing ftrace regs: PASSED
[    7.037994] Testing tracer nop: PASSED
[    7.042256] Testing tracer function_graph: FAILED!
[   12.216112] ------------[ cut here ]------------
[   12.220436] WARNING: CPU: 0 PID: 1 at kernel/trace/trace.c:1953 
run_tracer_selftest+0x138/0x1b4
[   12.229045] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.17.0-rc2-s3k-dev-02096-g28b040bd2357 #1030
[   12.237735] NIP:  c00d01b4 LR: c00d01b4 CTR: c03d37fc
[   12.242724] REGS: c902bd90 TRAP: 0700   Not tainted 
(5.17.0-rc2-s3k-dev-02096-g28b040bd2357)
[   12.251157] MSR:  00029032 <EE,ME,IR,DR,RI>  CR: 28000242  XER: 00000000
[   12.257870]
[   12.257870] GPR00: c00d01b4 c902be50 c2140000 00000007 c108d224 
00000001 c11ed2e8 c108d340
[   12.257870] GPR08: 3fffbfff 00000000 c129beac 3fffc000 22000244 
00000000 c0004b78 00000000
[   12.257870] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000000 00000000 c1039020
[   12.257870] GPR24: c12d0000 c1000144 c1223c48 c12b53c4 c12b55dc 
c1293118 fffffdf4 c1223c38
[   12.293843] NIP [c00d01b4] run_tracer_selftest+0x138/0x1b4
[   12.299265] LR [c00d01b4] run_tracer_selftest+0x138/0x1b4
[   12.304603] Call Trace:
[   12.307012] [c902be50] [c00d01b4] run_tracer_selftest+0x138/0x1b4 
(unreliable)
[   12.314155] [c902be70] [c100cf44] register_tracer+0x14c/0x218
[   12.319835] [c902be90] [c10011a0] do_one_initcall+0x8c/0x17c
[   12.325430] [c902bef0] [c10014c0] kernel_init_freeable+0x1a8/0x254
[   12.331540] [c902bf20] [c0004ba8] kernel_init+0x30/0x150
[   12.336789] [c902bf30] [c001222c] ret_from_kernel_thread+0x5c/0x64
[   12.342902] Instruction dump:
[   12.345828] 4bf9a135 813d0030 7fc4f378 7d2903a6 7fa3eb78 4e800421 
7c7e1b79 939f0f60
[   12.353657] 41820014 3c60c08a 3863644c 4bf9a109 <0fe00000> 387f00b0 
4bff76bd 893d0052
[   12.361659] ---[ end trace 0000000000000000 ]---


With the patch I get:

[    6.191286] Testing ftrace recursion: PASSED
[    6.473307] Testing ftrace recursion safe: PASSED
[    6.755758] Testing ftrace regs: PASSED
[    7.037993] Testing tracer nop: PASSED
[    7.042255] Testing tracer function_graph: PASSED

Is this patch going to be merged via another tree ?

Thanks
Christophe


> ---
>   kernel/trace/trace_selftest.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index afd937a46496..abcadbe933bb 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -784,9 +784,7 @@ static struct fgraph_ops fgraph_ops __initdata  = {
>   	.retfunc		= &trace_graph_return,
>   };
>   
> -#if defined(CONFIG_DYNAMIC_FTRACE) && \
> -    defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
> -#define TEST_DIRECT_TRAMP
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>   noinline __noclone static void trace_direct_tramp(void) { }
>   #endif
>   
> @@ -849,7 +847,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
>   		goto out;
>   	}
>   
> -#ifdef TEST_DIRECT_TRAMP
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>   	tracing_reset_online_cpus(&tr->array_buffer);
>   	set_graph_array(tr);
>   

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Steven Rostedt]

On Thu, 24 Feb 2022 13:43:02 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> Hi Michael,
> 
> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
> > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
> > direct tramp.
> > 
> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>  
> 
> You didn't apply this patch when you merged the series. Without it I get 
> the following :

Maybe they wanted my acked-by.

But I'm working on a series to send to Linus. I can pick this patch up, as
it touches just my code.

-- Steve

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Steven Rostedt]

On Thu, 24 Feb 2022 13:43:02 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> Hi Michael,
> 
> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
> > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
> > direct tramp.
> > 
> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>  
> 
> You didn't apply this patch when you merged the series. Without it I get 
> the following :

Maybe they wanted my acked-by.

But I'm working on a series to send to Linus. I can pick this patch up, as
it touches just my code.

-- Steve

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Christophe Leroy]

Le 24/02/2022 à 15:53, Steven Rostedt a écrit :
> On Thu, 24 Feb 2022 13:43:02 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> Hi Michael,
>>
>> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
>>> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
>>> direct tramp.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> You didn't apply this patch when you merged the series. Without it I get
>> the following :
> 
> Maybe they wanted my acked-by.
> 
> But I'm working on a series to send to Linus. I can pick this patch up, as
> it touches just my code.
> 

That would be great, thanks.

Christophe

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Christophe Leroy]

Le 24/02/2022 à 15:53, Steven Rostedt a écrit :
> On Thu, 24 Feb 2022 13:43:02 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> Hi Michael,
>>
>> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
>>> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
>>> direct tramp.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> You didn't apply this patch when you merged the series. Without it I get
>> the following :
> 
> Maybe they wanted my acked-by.
> 
> But I'm working on a series to send to Linus. I can pick this patch up, as
> it touches just my code.
> 

That would be great, thanks.

Christophe

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Steven Rostedt]

On Thu, 24 Feb 2022 15:13:12 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > But I'm working on a series to send to Linus. I can pick this patch up, as
> > it touches just my code.
> >   
> 
> That would be great, thanks.

It's in my queue and running through my tests, which take 7 to 13 hours to
complete (depending on the changes).

-- Steve

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Steven Rostedt]

On Thu, 24 Feb 2022 15:13:12 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > But I'm working on a series to send to Linus. I can pick this patch up, as
> > it touches just my code.
> >   
> 
> That would be great, thanks.

It's in my queue and running through my tests, which take 7 to 13 hours to
complete (depending on the changes).

-- Steve

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Michael Ellerman]

Steven Rostedt <rostedt@goodmis.org> writes:
> On Thu, 24 Feb 2022 13:43:02 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>
>> Hi Michael,
>> 
>> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
>> > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
>> > direct tramp.
>> > 
>> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>  
>> 
>> You didn't apply this patch when you merged the series. Without it I get 
>> the following :
>
> Maybe they wanted my acked-by.

Yeah, I didn't want to take it via my tree without an ack. I meant to
reply to the patch saying that but ...

> But I'm working on a series to send to Linus. I can pick this patch up, as
> it touches just my code.

Thanks.

cheers

Re: [PATCH v2 02/13] tracing: Fix selftest config check for function graph start up test

[Michael Ellerman]

Steven Rostedt <rostedt@goodmis.org> writes:
> On Thu, 24 Feb 2022 13:43:02 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>
>> Hi Michael,
>> 
>> Le 20/12/2021 à 17:38, Christophe Leroy a écrit :
>> > CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is required to test
>> > direct tramp.
>> > 
>> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>  
>> 
>> You didn't apply this patch when you merged the series. Without it I get 
>> the following :
>
> Maybe they wanted my acked-by.

Yeah, I didn't want to take it via my tree without an ack. I meant to
reply to the patch saying that but ...

> But I'm working on a series to send to Linus. I can pick this patch up, as
> it touches just my code.

Thanks.

cheers

[PATCH v2 03/13] powerpc/module_32: Fix livepatching for RO modules

[Christophe Leroy]

Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y.

R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
the types generated by the kpatch-build userspace tool or klp-convert
kernel tree observed applying a relocation to a post-init module.

Use patch_instruction() to patch those relocations.

Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
RO modules") did similar change in module_64.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index a491ad481d85..a0432ef46967 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -18,6 +18,7 @@
 #include <linux/bug.h>
 #include <linux/sort.h>
 #include <asm/setup.h>
+#include <asm/code-patching.h>
 
 /* Count how many different relocations (different symbol, different
    addend) */
@@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
 		entry++;
 	}
 
-	entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
-	entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
-	entry->jump[2] = PPC_RAW_MTCTR(_R12);
-	entry->jump[3] = PPC_RAW_BCTR();
+	if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
+		return 0;
+	if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
+		return 0;
+	if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
+		return 0;
+	if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
+		return 0;
 
 	pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
 	return (uint32_t)entry;
 }
 
+static int patch_location_16(uint32_t *loc, u16 value)
+{
+	loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
+	return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
+}
+
 int apply_relocate_add(Elf32_Shdr *sechdrs,
 		       const char *strtab,
 		       unsigned int symindex,
@@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
 
 		case R_PPC_ADDR16_LO:
 			/* Low half of the symbol */
-			*(uint16_t *)location = value;
+			if (patch_location_16(location, PPC_LO(value)))
+				return -EFAULT;
 			break;
 
 		case R_PPC_ADDR16_HI:
 			/* Higher half of the symbol */
-			*(uint16_t *)location = (value >> 16);
+			if (patch_location_16(location, PPC_HI(value)))
+				return -EFAULT;
 			break;
 
 		case R_PPC_ADDR16_HA:
-			/* Sign-adjusted lower 16 bits: PPC ELF ABI says:
-			   (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
-			   This is the same, only sane.
-			 */
-			*(uint16_t *)location = (value + 0x8000) >> 16;
+			if (patch_location_16(location, PPC_HA(value)))
+				return -EFAULT;
 			break;
 
 		case R_PPC_REL24:
 			if ((int)(value - (uint32_t)location) < -0x02000000
-			    || (int)(value - (uint32_t)location) >= 0x02000000)
+			    || (int)(value - (uint32_t)location) >= 0x02000000) {
 				value = do_plt_call(location, value,
 						    sechdrs, module);
+				if (!value)
+					return -EFAULT;
+			}
 
 			/* Only replace bits 2 through 26 */
 			pr_debug("REL24 value = %08X. location = %08X\n",
 			       value, (uint32_t)location);
 			pr_debug("Location before: %08X.\n",
 			       *(uint32_t *)location);
-			*(uint32_t *)location
-				= (*(uint32_t *)location & ~0x03fffffc)
+			value = (*(uint32_t *)location & ~0x03fffffc)
 				| ((value - (uint32_t)location)
 				   & 0x03fffffc);
+
+			if (patch_instruction(location, ppc_inst(value)))
+				return -EFAULT;
+
 			pr_debug("Location after: %08X.\n",
 			       *(uint32_t *)location);
 			pr_debug("ie. jump to %08X+%08X = %08X\n",
-- 
2.33.1

[PATCH v2 03/13] powerpc/module_32: Fix livepatching for RO modules

[Christophe Leroy]

Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y.

R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
the types generated by the kpatch-build userspace tool or klp-convert
kernel tree observed applying a relocation to a post-init module.

Use patch_instruction() to patch those relocations.

Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
RO modules") did similar change in module_64.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index a491ad481d85..a0432ef46967 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -18,6 +18,7 @@
 #include <linux/bug.h>
 #include <linux/sort.h>
 #include <asm/setup.h>
+#include <asm/code-patching.h>
 
 /* Count how many different relocations (different symbol, different
    addend) */
@@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
 		entry++;
 	}
 
-	entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
-	entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
-	entry->jump[2] = PPC_RAW_MTCTR(_R12);
-	entry->jump[3] = PPC_RAW_BCTR();
+	if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
+		return 0;
+	if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
+		return 0;
+	if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
+		return 0;
+	if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
+		return 0;
 
 	pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
 	return (uint32_t)entry;
 }
 
+static int patch_location_16(uint32_t *loc, u16 value)
+{
+	loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
+	return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
+}
+
 int apply_relocate_add(Elf32_Shdr *sechdrs,
 		       const char *strtab,
 		       unsigned int symindex,
@@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
 
 		case R_PPC_ADDR16_LO:
 			/* Low half of the symbol */
-			*(uint16_t *)location = value;
+			if (patch_location_16(location, PPC_LO(value)))
+				return -EFAULT;
 			break;
 
 		case R_PPC_ADDR16_HI:
 			/* Higher half of the symbol */
-			*(uint16_t *)location = (value >> 16);
+			if (patch_location_16(location, PPC_HI(value)))
+				return -EFAULT;
 			break;
 
 		case R_PPC_ADDR16_HA:
-			/* Sign-adjusted lower 16 bits: PPC ELF ABI says:
-			   (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
-			   This is the same, only sane.
-			 */
-			*(uint16_t *)location = (value + 0x8000) >> 16;
+			if (patch_location_16(location, PPC_HA(value)))
+				return -EFAULT;
 			break;
 
 		case R_PPC_REL24:
 			if ((int)(value - (uint32_t)location) < -0x02000000
-			    || (int)(value - (uint32_t)location) >= 0x02000000)
+			    || (int)(value - (uint32_t)location) >= 0x02000000) {
 				value = do_plt_call(location, value,
 						    sechdrs, module);
+				if (!value)
+					return -EFAULT;
+			}
 
 			/* Only replace bits 2 through 26 */
 			pr_debug("REL24 value = %08X. location = %08X\n",
 			       value, (uint32_t)location);
 			pr_debug("Location before: %08X.\n",
 			       *(uint32_t *)location);
-			*(uint32_t *)location
-				= (*(uint32_t *)location & ~0x03fffffc)
+			value = (*(uint32_t *)location & ~0x03fffffc)
 				| ((value - (uint32_t)location)
 				   & 0x03fffffc);
+
+			if (patch_instruction(location, ppc_inst(value)))
+				return -EFAULT;
+
 			pr_debug("Location after: %08X.\n",
 			       *(uint32_t *)location);
 			pr_debug("ie. jump to %08X+%08X = %08X\n",
-- 
2.33.1

Re: [PATCH v2 03/13] powerpc/module_32: Fix livepatching for RO modules

[Joe Lawrence]

On Mon, Dec 20, 2021 at 04:38:09PM +0000, Christophe Leroy wrote:
> Livepatching a loaded module involves applying relocations through
> apply_relocate_add(), which attempts to write to read-only memory when
> CONFIG_STRICT_MODULE_RWX=y.
> 
> R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
> the types generated by the kpatch-build userspace tool or klp-convert
> kernel tree observed applying a relocation to a post-init module.
> 
> Use patch_instruction() to patch those relocations.
> 
> Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
> RO modules") did similar change in module_64.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Russell Currey <ruscur@russell.cc>
> ---
>  arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
> index a491ad481d85..a0432ef46967 100644
> --- a/arch/powerpc/kernel/module_32.c
> +++ b/arch/powerpc/kernel/module_32.c
> @@ -18,6 +18,7 @@
>  #include <linux/bug.h>
>  #include <linux/sort.h>
>  #include <asm/setup.h>
> +#include <asm/code-patching.h>
>  
>  /* Count how many different relocations (different symbol, different
>     addend) */
> @@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
>  		entry++;
>  	}
>  
> -	entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
> -	entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
> -	entry->jump[2] = PPC_RAW_MTCTR(_R12);
> -	entry->jump[3] = PPC_RAW_BCTR();
> +	if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
> +		return 0;
>  
>  	pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
>  	return (uint32_t)entry;
>  }
>  
> +static int patch_location_16(uint32_t *loc, u16 value)
> +{
> +	loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
> +	return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
> +}
> +
>  int apply_relocate_add(Elf32_Shdr *sechdrs,
>  		       const char *strtab,
>  		       unsigned int symindex,
> @@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
>  
>  		case R_PPC_ADDR16_LO:
>  			/* Low half of the symbol */
> -			*(uint16_t *)location = value;
> +			if (patch_location_16(location, PPC_LO(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_ADDR16_HI:
>  			/* Higher half of the symbol */
> -			*(uint16_t *)location = (value >> 16);
> +			if (patch_location_16(location, PPC_HI(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_ADDR16_HA:
> -			/* Sign-adjusted lower 16 bits: PPC ELF ABI says:
> -			   (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
> -			   This is the same, only sane.
> -			 */
> -			*(uint16_t *)location = (value + 0x8000) >> 16;
> +			if (patch_location_16(location, PPC_HA(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_REL24:
>  			if ((int)(value - (uint32_t)location) < -0x02000000
> -			    || (int)(value - (uint32_t)location) >= 0x02000000)
> +			    || (int)(value - (uint32_t)location) >= 0x02000000) {
>  				value = do_plt_call(location, value,
>  						    sechdrs, module);
> +				if (!value)
> +					return -EFAULT;
> +			}
>  
>  			/* Only replace bits 2 through 26 */
>  			pr_debug("REL24 value = %08X. location = %08X\n",
>  			       value, (uint32_t)location);
>  			pr_debug("Location before: %08X.\n",
>  			       *(uint32_t *)location);
> -			*(uint32_t *)location
> -				= (*(uint32_t *)location & ~0x03fffffc)
> +			value = (*(uint32_t *)location & ~0x03fffffc)
>  				| ((value - (uint32_t)location)
>  				   & 0x03fffffc);
> +
> +			if (patch_instruction(location, ppc_inst(value)))
> +				return -EFAULT;
> +
>  			pr_debug("Location after: %08X.\n",
>  			       *(uint32_t *)location);
>  			pr_debug("ie. jump to %08X+%08X = %08X\n",
> -- 
> 2.33.1
> 

IIRC, offlist we hacked up klp-convert to create the klp-relocations for
a 32-bit target and then you hit the selftest late relocation crash, so
I assume that part is happy after this fix. :)  Thanks again for the
testing.

For the livepatching implications,

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH v2 03/13] powerpc/module_32: Fix livepatching for RO modules

[Joe Lawrence]

On Mon, Dec 20, 2021 at 04:38:09PM +0000, Christophe Leroy wrote:
> Livepatching a loaded module involves applying relocations through
> apply_relocate_add(), which attempts to write to read-only memory when
> CONFIG_STRICT_MODULE_RWX=y.
> 
> R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
> the types generated by the kpatch-build userspace tool or klp-convert
> kernel tree observed applying a relocation to a post-init module.
> 
> Use patch_instruction() to patch those relocations.
> 
> Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
> RO modules") did similar change in module_64.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Russell Currey <ruscur@russell.cc>
> ---
>  arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
> index a491ad481d85..a0432ef46967 100644
> --- a/arch/powerpc/kernel/module_32.c
> +++ b/arch/powerpc/kernel/module_32.c
> @@ -18,6 +18,7 @@
>  #include <linux/bug.h>
>  #include <linux/sort.h>
>  #include <asm/setup.h>
> +#include <asm/code-patching.h>
>  
>  /* Count how many different relocations (different symbol, different
>     addend) */
> @@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
>  		entry++;
>  	}
>  
> -	entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
> -	entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
> -	entry->jump[2] = PPC_RAW_MTCTR(_R12);
> -	entry->jump[3] = PPC_RAW_BCTR();
> +	if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
> +		return 0;
>  
>  	pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
>  	return (uint32_t)entry;
>  }
>  
> +static int patch_location_16(uint32_t *loc, u16 value)
> +{
> +	loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
> +	return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
> +}
> +
>  int apply_relocate_add(Elf32_Shdr *sechdrs,
>  		       const char *strtab,
>  		       unsigned int symindex,
> @@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
>  
>  		case R_PPC_ADDR16_LO:
>  			/* Low half of the symbol */
> -			*(uint16_t *)location = value;
> +			if (patch_location_16(location, PPC_LO(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_ADDR16_HI:
>  			/* Higher half of the symbol */
> -			*(uint16_t *)location = (value >> 16);
> +			if (patch_location_16(location, PPC_HI(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_ADDR16_HA:
> -			/* Sign-adjusted lower 16 bits: PPC ELF ABI says:
> -			   (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
> -			   This is the same, only sane.
> -			 */
> -			*(uint16_t *)location = (value + 0x8000) >> 16;
> +			if (patch_location_16(location, PPC_HA(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_REL24:
>  			if ((int)(value - (uint32_t)location) < -0x02000000
> -			    || (int)(value - (uint32_t)location) >= 0x02000000)
> +			    || (int)(value - (uint32_t)location) >= 0x02000000) {
>  				value = do_plt_call(location, value,
>  						    sechdrs, module);
> +				if (!value)
> +					return -EFAULT;
> +			}
>  
>  			/* Only replace bits 2 through 26 */
>  			pr_debug("REL24 value = %08X. location = %08X\n",
>  			       value, (uint32_t)location);
>  			pr_debug("Location before: %08X.\n",
>  			       *(uint32_t *)location);
> -			*(uint32_t *)location
> -				= (*(uint32_t *)location & ~0x03fffffc)
> +			value = (*(uint32_t *)location & ~0x03fffffc)
>  				| ((value - (uint32_t)location)
>  				   & 0x03fffffc);
> +
> +			if (patch_instruction(location, ppc_inst(value)))
> +				return -EFAULT;
> +
>  			pr_debug("Location after: %08X.\n",
>  			       *(uint32_t *)location);
>  			pr_debug("ie. jump to %08X+%08X = %08X\n",
> -- 
> 2.33.1
> 

IIRC, offlist we hacked up klp-convert to create the klp-relocations for
a 32-bit target and then you hit the selftest late relocation crash, so
I assume that part is happy after this fix. :)  Thanks again for the
testing.

For the livepatching implications,

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

[PATCH v2 04/13] powerpc/ftrace: Add support for livepatch to PPC32

[Christophe Leroy]

PPC64 needs some special logic to properly set up the TOC.
See commit 85baa095497f ("powerpc/livepatch: Add live patching support
on ppc64le") for details.

PPC32 doesn't have TOC so it doesn't need that logic, so adding
LIVEPATCH support is straight forward.

Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.

Livepatch sample modules all work.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig                   | 6 +++++-
 arch/powerpc/include/asm/livepatch.h   | 8 +++++---
 arch/powerpc/include/asm/thread_info.h | 2 +-
 arch/powerpc/kernel/asm-offsets.c      | 2 +-
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 0631c9241af3..cdac2115eb00 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -9,6 +9,10 @@ config 64BIT
 	bool
 	default y if PPC64
 
+config LIVEPATCH_64
+	def_bool PPC64
+	depends	on LIVEPATCH
+
 config MMU
 	bool
 	default y
@@ -230,7 +234,7 @@ config PPC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
 	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION
-	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS && PPC64
+	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_MOD_ARCH_SPECIFIC
 	select HAVE_NMI				if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
 	select HAVE_OPTPROBES
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 4fe018cc207b..37af961eb74c 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -23,12 +23,14 @@ static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
 {
 	/*
-	 * Live patch works only with -mprofile-kernel on PPC. In this case,
-	 * the ftrace location is always within the first 16 bytes.
+	 * Live patch works on PPC32 and only with -mprofile-kernel on PPC64. In
+	 * both cases, the ftrace location is always within the first 16 bytes.
 	 */
 	return ftrace_location_range(faddr, faddr + 16);
 }
+#endif /* CONFIG_LIVEPATCH */
 
+#ifdef CONFIG_LIVEPATCH_64
 static inline void klp_init_thread_info(struct task_struct *p)
 {
 	/* + 1 to account for STACK_END_MAGIC */
@@ -36,6 +38,6 @@ static inline void klp_init_thread_info(struct task_struct *p)
 }
 #else
 static inline void klp_init_thread_info(struct task_struct *p) { }
-#endif /* CONFIG_LIVEPATCH */
+#endif
 
 #endif /* _ASM_POWERPC_LIVEPATCH_H */
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 5725029aaa29..42f8a1f99036 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -51,7 +51,7 @@ struct thread_info {
 	unsigned int	cpu;
 #endif
 	unsigned long	local_flags;		/* private flags for thread */
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	unsigned long *livepatch_sp;
 #endif
 #if defined(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) && defined(CONFIG_PPC32)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 7582f3e3a330..eec536aef83a 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -94,7 +94,7 @@ int main(void)
 	OFFSET(TASK_CPU, task_struct, thread_info.cpu);
 #endif
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	OFFSET(TI_livepatch_sp, thread_info, livepatch_sp);
 #endif
 
-- 
2.33.1

[PATCH v2 04/13] powerpc/ftrace: Add support for livepatch to PPC32

[Christophe Leroy]

PPC64 needs some special logic to properly set up the TOC.
See commit 85baa095497f ("powerpc/livepatch: Add live patching support
on ppc64le") for details.

PPC32 doesn't have TOC so it doesn't need that logic, so adding
LIVEPATCH support is straight forward.

Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.

Livepatch sample modules all work.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig                   | 6 +++++-
 arch/powerpc/include/asm/livepatch.h   | 8 +++++---
 arch/powerpc/include/asm/thread_info.h | 2 +-
 arch/powerpc/kernel/asm-offsets.c      | 2 +-
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 0631c9241af3..cdac2115eb00 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -9,6 +9,10 @@ config 64BIT
 	bool
 	default y if PPC64
 
+config LIVEPATCH_64
+	def_bool PPC64
+	depends	on LIVEPATCH
+
 config MMU
 	bool
 	default y
@@ -230,7 +234,7 @@ config PPC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
 	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION
-	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS && PPC64
+	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_MOD_ARCH_SPECIFIC
 	select HAVE_NMI				if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
 	select HAVE_OPTPROBES
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 4fe018cc207b..37af961eb74c 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -23,12 +23,14 @@ static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
 {
 	/*
-	 * Live patch works only with -mprofile-kernel on PPC. In this case,
-	 * the ftrace location is always within the first 16 bytes.
+	 * Live patch works on PPC32 and only with -mprofile-kernel on PPC64. In
+	 * both cases, the ftrace location is always within the first 16 bytes.
 	 */
 	return ftrace_location_range(faddr, faddr + 16);
 }
+#endif /* CONFIG_LIVEPATCH */
 
+#ifdef CONFIG_LIVEPATCH_64
 static inline void klp_init_thread_info(struct task_struct *p)
 {
 	/* + 1 to account for STACK_END_MAGIC */
@@ -36,6 +38,6 @@ static inline void klp_init_thread_info(struct task_struct *p)
 }
 #else
 static inline void klp_init_thread_info(struct task_struct *p) { }
-#endif /* CONFIG_LIVEPATCH */
+#endif
 
 #endif /* _ASM_POWERPC_LIVEPATCH_H */
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 5725029aaa29..42f8a1f99036 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -51,7 +51,7 @@ struct thread_info {
 	unsigned int	cpu;
 #endif
 	unsigned long	local_flags;		/* private flags for thread */
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	unsigned long *livepatch_sp;
 #endif
 #if defined(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) && defined(CONFIG_PPC32)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 7582f3e3a330..eec536aef83a 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -94,7 +94,7 @@ int main(void)
 	OFFSET(TASK_CPU, task_struct, thread_info.cpu);
 #endif
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	OFFSET(TI_livepatch_sp, thread_info, livepatch_sp);
 #endif
 
-- 
2.33.1

Re: [PATCH v2 04/13] powerpc/ftrace: Add support for livepatch to PPC32

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> PPC64 needs some special logic to properly set up the TOC.
> See commit 85baa095497f ("powerpc/livepatch: Add live patching support
> on ppc64le") for details.
> 
> PPC32 doesn't have TOC so it doesn't need that logic, so adding
> LIVEPATCH support is straight forward.
> 
> Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.
> 
> Livepatch sample modules all work.

Great.
 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

FWIW the patch looks good to me.

Miroslav

Re: [PATCH v2 04/13] powerpc/ftrace: Add support for livepatch to PPC32

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> PPC64 needs some special logic to properly set up the TOC.
> See commit 85baa095497f ("powerpc/livepatch: Add live patching support
> on ppc64le") for details.
> 
> PPC32 doesn't have TOC so it doesn't need that logic, so adding
> LIVEPATCH support is straight forward.
> 
> Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.
> 
> Livepatch sample modules all work.

Great.
 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

FWIW the patch looks good to me.

Miroslav

[PATCH v2 05/13] powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32

[Christophe Leroy]

PPC32 mcount() caller already saves LR on stack,
no need to save it again.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace_32.S | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 0a02c0cb12d9..7e2fd729116b 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -53,9 +53,6 @@ _GLOBAL(ftrace_stub)
 	blr
 
 _GLOBAL(ftrace_regs_caller)
-	/* Save the original return address in A's stack frame */
-	stw	r0,LRSAVE(r1)
-
 	/* Create our stack frame + pt_regs */
 	stwu	r1,-INT_FRAME_SIZE(r1)
 
-- 
2.33.1

[PATCH v2 05/13] powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32

[Christophe Leroy]

PPC32 mcount() caller already saves LR on stack,
no need to save it again.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace_32.S | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 0a02c0cb12d9..7e2fd729116b 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -53,9 +53,6 @@ _GLOBAL(ftrace_stub)
 	blr
 
 _GLOBAL(ftrace_regs_caller)
-	/* Save the original return address in A's stack frame */
-	stw	r0,LRSAVE(r1)
-
 	/* Create our stack frame + pt_regs */
 	stwu	r1,-INT_FRAME_SIZE(r1)
 
-- 
2.33.1

[PATCH v2 06/13] powerpc/ftrace: Simplify PPC32's return_to_handler()

[Christophe Leroy]

return_to_handler() was copied from PPC64. For PPC32 it
just needs to save r3 and r4, and doesn't require any nop
after the bl.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace_32.S | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 7e2fd729116b..95ffea2bdc29 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -162,22 +162,18 @@ _GLOBAL(ftrace_graph_caller)
 
 _GLOBAL(return_to_handler)
 	/* need to save return values */
-	stwu	r1, -32(r1)
-	stw	r3, 20(r1)
-	stw	r4, 16(r1)
-	stw	r31, 12(r1)
-	mr	r31, r1
+	stwu	r1, -16(r1)
+	stw	r3, 8(r1)
+	stw	r4, 12(r1)
 
 	bl	ftrace_return_to_handler
-	nop
 
 	/* return value has real return address */
 	mtlr	r3
 
-	lwz	r3, 20(r1)
-	lwz	r4, 16(r1)
-	lwz	r31,12(r1)
-	lwz	r1, 0(r1)
+	lwz	r3, 8(r1)
+	lwz	r4, 12(r1)
+	addi	r1, r1, 16
 
 	/* Jump back to real return address */
 	blr
-- 
2.33.1

[PATCH v2 06/13] powerpc/ftrace: Simplify PPC32's return_to_handler()

[Christophe Leroy]

return_to_handler() was copied from PPC64. For PPC32 it
just needs to save r3 and r4, and doesn't require any nop
after the bl.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace_32.S | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 7e2fd729116b..95ffea2bdc29 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -162,22 +162,18 @@ _GLOBAL(ftrace_graph_caller)
 
 _GLOBAL(return_to_handler)
 	/* need to save return values */
-	stwu	r1, -32(r1)
-	stw	r3, 20(r1)
-	stw	r4, 16(r1)
-	stw	r31, 12(r1)
-	mr	r31, r1
+	stwu	r1, -16(r1)
+	stw	r3, 8(r1)
+	stw	r4, 12(r1)
 
 	bl	ftrace_return_to_handler
-	nop
 
 	/* return value has real return address */
 	mtlr	r3
 
-	lwz	r3, 20(r1)
-	lwz	r4, 16(r1)
-	lwz	r31,12(r1)
-	lwz	r1, 0(r1)
+	lwz	r3, 8(r1)
+	lwz	r4, 12(r1)
+	addi	r1, r1, 16
 
 	/* Jump back to real return address */
 	blr
-- 
2.33.1

[PATCH v2 07/13] powerpc/ftrace: Prepare PPC32's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change
ftrace_caller() stack layout to match struct pt_regs.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/ftrace.h     | 39 +--------------------------
 arch/powerpc/kernel/trace/ftrace_32.S | 29 +++++++++++++++++---
 2 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index debe8c4f7062..b3f6184f77ea 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -10,44 +10,7 @@
 
 #define HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
 
-#ifdef __ASSEMBLY__
-
-/* Based off of objdump output from glibc */
-
-#define MCOUNT_SAVE_FRAME			\
-	stwu	r1,-48(r1);			\
-	stw	r3, 12(r1);			\
-	stw	r4, 16(r1);			\
-	stw	r5, 20(r1);			\
-	stw	r6, 24(r1);			\
-	mflr	r3;				\
-	lwz	r4, 52(r1);			\
-	mfcr	r5;				\
-	stw	r7, 28(r1);			\
-	stw	r8, 32(r1);			\
-	stw	r9, 36(r1);			\
-	stw	r10,40(r1);			\
-	stw	r3, 44(r1);			\
-	stw	r5, 8(r1)
-
-#define MCOUNT_RESTORE_FRAME			\
-	lwz	r6, 8(r1);			\
-	lwz	r0, 44(r1);			\
-	lwz	r3, 12(r1);			\
-	mtctr	r0;				\
-	lwz	r4, 16(r1);			\
-	mtcr	r6;				\
-	lwz	r5, 20(r1);			\
-	lwz	r6, 24(r1);			\
-	lwz	r0, 52(r1);			\
-	lwz	r7, 28(r1);			\
-	lwz	r8, 32(r1);			\
-	mtlr	r0;				\
-	lwz	r9, 36(r1);			\
-	lwz	r10,40(r1);			\
-	addi	r1, r1, 48
-
-#else /* !__ASSEMBLY__ */
+#ifndef __ASSEMBLY__
 extern void _mcount(void);
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 95ffea2bdc29..c4055b41af5f 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -27,17 +27,38 @@ _GLOBAL(_mcount)
 EXPORT_SYMBOL(_mcount)
 
 _GLOBAL(ftrace_caller)
-	MCOUNT_SAVE_FRAME
-	/* r3 ends up with link register */
+	stwu	r1, -INT_FRAME_SIZE(r1)
+
+	SAVE_GPRS(3, 10, r1)
+
+	addi	r8, r1, INT_FRAME_SIZE
+	stw	r8, GPR1(r1)
+
+	mflr	r3
+	stw	r3, _NIP(r1)
 	subi	r3, r3, MCOUNT_INSN_SIZE
+
+	stw	r0, _LINK(r1)
+	mr	r4, r0
+
 	lis	r5,function_trace_op@ha
 	lwz	r5,function_trace_op@l(r5)
-	li	r6, 0
+
+	addi	r6, r1, STACK_FRAME_OVERHEAD
 .globl ftrace_call
 ftrace_call:
 	bl	ftrace_stub
 	nop
-	MCOUNT_RESTORE_FRAME
+
+	lwz	r3, _NIP(r1)
+	mtctr	r3
+
+	REST_GPRS(3, 10, r1)
+
+	lwz	r0, _LINK(r1)
+	mtlr	r0
+
+	addi	r1, r1, INT_FRAME_SIZE
 ftrace_caller_common:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
-- 
2.33.1

[PATCH v2 07/13] powerpc/ftrace: Prepare PPC32's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change
ftrace_caller() stack layout to match struct pt_regs.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/ftrace.h     | 39 +--------------------------
 arch/powerpc/kernel/trace/ftrace_32.S | 29 +++++++++++++++++---
 2 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index debe8c4f7062..b3f6184f77ea 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -10,44 +10,7 @@
 
 #define HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
 
-#ifdef __ASSEMBLY__
-
-/* Based off of objdump output from glibc */
-
-#define MCOUNT_SAVE_FRAME			\
-	stwu	r1,-48(r1);			\
-	stw	r3, 12(r1);			\
-	stw	r4, 16(r1);			\
-	stw	r5, 20(r1);			\
-	stw	r6, 24(r1);			\
-	mflr	r3;				\
-	lwz	r4, 52(r1);			\
-	mfcr	r5;				\
-	stw	r7, 28(r1);			\
-	stw	r8, 32(r1);			\
-	stw	r9, 36(r1);			\
-	stw	r10,40(r1);			\
-	stw	r3, 44(r1);			\
-	stw	r5, 8(r1)
-
-#define MCOUNT_RESTORE_FRAME			\
-	lwz	r6, 8(r1);			\
-	lwz	r0, 44(r1);			\
-	lwz	r3, 12(r1);			\
-	mtctr	r0;				\
-	lwz	r4, 16(r1);			\
-	mtcr	r6;				\
-	lwz	r5, 20(r1);			\
-	lwz	r6, 24(r1);			\
-	lwz	r0, 52(r1);			\
-	lwz	r7, 28(r1);			\
-	lwz	r8, 32(r1);			\
-	mtlr	r0;				\
-	lwz	r9, 36(r1);			\
-	lwz	r10,40(r1);			\
-	addi	r1, r1, 48
-
-#else /* !__ASSEMBLY__ */
+#ifndef __ASSEMBLY__
 extern void _mcount(void);
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 95ffea2bdc29..c4055b41af5f 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -27,17 +27,38 @@ _GLOBAL(_mcount)
 EXPORT_SYMBOL(_mcount)
 
 _GLOBAL(ftrace_caller)
-	MCOUNT_SAVE_FRAME
-	/* r3 ends up with link register */
+	stwu	r1, -INT_FRAME_SIZE(r1)
+
+	SAVE_GPRS(3, 10, r1)
+
+	addi	r8, r1, INT_FRAME_SIZE
+	stw	r8, GPR1(r1)
+
+	mflr	r3
+	stw	r3, _NIP(r1)
 	subi	r3, r3, MCOUNT_INSN_SIZE
+
+	stw	r0, _LINK(r1)
+	mr	r4, r0
+
 	lis	r5,function_trace_op@ha
 	lwz	r5,function_trace_op@l(r5)
-	li	r6, 0
+
+	addi	r6, r1, STACK_FRAME_OVERHEAD
 .globl ftrace_call
 ftrace_call:
 	bl	ftrace_stub
 	nop
-	MCOUNT_RESTORE_FRAME
+
+	lwz	r3, _NIP(r1)
+	mtctr	r3
+
+	REST_GPRS(3, 10, r1)
+
+	lwz	r0, _LINK(r1)
+	mtlr	r0
+
+	addi	r1, r1, INT_FRAME_SIZE
 ftrace_caller_common:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
-- 
2.33.1

[PATCH v2 08/13] powerpc/ftrace: Prepare PPC64's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
to handle LIVEPATCH the same way as frace_caller_regs().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index d636fc755f60..f6f787819273 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -172,14 +172,19 @@ _GLOBAL(ftrace_caller)
 	addi	r3, r3, function_trace_op@toc@l
 	ld	r5, 0(r3)
 
+#ifdef CONFIG_LIVEPATCH_64
+	SAVE_GPR(14, r1)
+	mr	r14,r7		/* remember old NIP */
+#endif
 	/* Calculate ip from nip-4 into r3 for call below */
 	subi    r3, r7, MCOUNT_INSN_SIZE
 
 	/* Put the original return address in r4 as parent_ip */
+	std	r0, _LINK(r1)
 	mr	r4, r0
 
-	/* Set pt_regs to NULL */
-	li	r6, 0
+	/* Load &pt_regs in r6 for call below */
+	addi    r6, r1 ,STACK_FRAME_OVERHEAD
 
 	/* ftrace_call(r3, r4, r5, r6) */
 .globl ftrace_call
@@ -189,6 +194,10 @@ ftrace_call:
 
 	ld	r3, _NIP(r1)
 	mtctr	r3
+#ifdef CONFIG_LIVEPATCH_64
+	cmpd	r14, r3		/* has NIP been altered? */
+	REST_GPR(14, r1)
+#endif
 
 	/* Restore gprs */
 	REST_GPRS(3, 10, r1)
@@ -196,13 +205,17 @@ ftrace_call:
 	/* Restore callee's TOC */
 	ld	r2, 24(r1)
 
+	/* Restore possibly modified LR */
+	ld	r0, _LINK(r1)
+	mtlr	r0
+
 	/* Pop our stack frame */
 	addi	r1, r1, SWITCH_FRAME_SIZE
 
-	/* Reload original LR */
-	ld	r0, LRSAVE(r1)
-	mtlr	r0
-
+#ifdef CONFIG_LIVEPATCH_64
+        /* Based on the cmpd above, if the NIP was altered handle livepatch */
+	bne-	livepatch_handler
+#endif
 	/* Handle function_graph or go back */
 	b	ftrace_caller_common
 
-- 
2.33.1

[PATCH v2 08/13] powerpc/ftrace: Prepare PPC64's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
to handle LIVEPATCH the same way as frace_caller_regs().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index d636fc755f60..f6f787819273 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -172,14 +172,19 @@ _GLOBAL(ftrace_caller)
 	addi	r3, r3, function_trace_op@toc@l
 	ld	r5, 0(r3)
 
+#ifdef CONFIG_LIVEPATCH_64
+	SAVE_GPR(14, r1)
+	mr	r14,r7		/* remember old NIP */
+#endif
 	/* Calculate ip from nip-4 into r3 for call below */
 	subi    r3, r7, MCOUNT_INSN_SIZE
 
 	/* Put the original return address in r4 as parent_ip */
+	std	r0, _LINK(r1)
 	mr	r4, r0
 
-	/* Set pt_regs to NULL */
-	li	r6, 0
+	/* Load &pt_regs in r6 for call below */
+	addi    r6, r1 ,STACK_FRAME_OVERHEAD
 
 	/* ftrace_call(r3, r4, r5, r6) */
 .globl ftrace_call
@@ -189,6 +194,10 @@ ftrace_call:
 
 	ld	r3, _NIP(r1)
 	mtctr	r3
+#ifdef CONFIG_LIVEPATCH_64
+	cmpd	r14, r3		/* has NIP been altered? */
+	REST_GPR(14, r1)
+#endif
 
 	/* Restore gprs */
 	REST_GPRS(3, 10, r1)
@@ -196,13 +205,17 @@ ftrace_call:
 	/* Restore callee's TOC */
 	ld	r2, 24(r1)
 
+	/* Restore possibly modified LR */
+	ld	r0, _LINK(r1)
+	mtlr	r0
+
 	/* Pop our stack frame */
 	addi	r1, r1, SWITCH_FRAME_SIZE
 
-	/* Reload original LR */
-	ld	r0, LRSAVE(r1)
-	mtlr	r0
-
+#ifdef CONFIG_LIVEPATCH_64
+        /* Based on the cmpd above, if the NIP was altered handle livepatch */
+	bne-	livepatch_handler
+#endif
 	/* Handle function_graph or go back */
 	b	ftrace_caller_common
 
-- 
2.33.1

Re: [PATCH v2 08/13] powerpc/ftrace: Prepare PPC64's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Hi Christophe,
Thanks for your work enabling DYNAMIC_FTRACE_WITH_ARGS on powerpc. Sorry 
for the late review on this series, but I have a few comments below.


Christophe Leroy wrote:
> In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
> to handle LIVEPATCH the same way as frace_caller_regs().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
>  1 file changed, 19 insertions(+), 6 deletions(-)

I think we also need to save r1 into pt_regs so that the stack pointer 
is available in the callbacks.

Other than that, a few minor nits below...

> 
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index d636fc755f60..f6f787819273 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -172,14 +172,19 @@ _GLOBAL(ftrace_caller)
>  	addi	r3, r3, function_trace_op@toc@l
>  	ld	r5, 0(r3)
>  
> +#ifdef CONFIG_LIVEPATCH_64
> +	SAVE_GPR(14, r1)
> +	mr	r14,r7		/* remember old NIP */
                    ^ add a space
> +#endif

Please add a blank line here, to match the formatting for the rest of 
this file.

>  	/* Calculate ip from nip-4 into r3 for call below */
>  	subi    r3, r7, MCOUNT_INSN_SIZE
>  
>  	/* Put the original return address in r4 as parent_ip */
> +	std	r0, _LINK(r1)
>  	mr	r4, r0
>  
> -	/* Set pt_regs to NULL */
> -	li	r6, 0
> +	/* Load &pt_regs in r6 for call below */
> +	addi    r6, r1 ,STACK_FRAME_OVERHEAD
                      ^^ incorrect spacing
>  
>  	/* ftrace_call(r3, r4, r5, r6) */
>  .globl ftrace_call
> @@ -189,6 +194,10 @@ ftrace_call:
>  
>  	ld	r3, _NIP(r1)
>  	mtctr	r3

Another blank line here.

> +#ifdef CONFIG_LIVEPATCH_64
> +	cmpd	r14, r3		/* has NIP been altered? */
> +	REST_GPR(14, r1)
> +#endif
>  
>  	/* Restore gprs */
>  	REST_GPRS(3, 10, r1)
> @@ -196,13 +205,17 @@ ftrace_call:
>  	/* Restore callee's TOC */
>  	ld	r2, 24(r1)
>  
> +	/* Restore possibly modified LR */
> +	ld	r0, _LINK(r1)
> +	mtlr	r0
> +
>  	/* Pop our stack frame */
>  	addi	r1, r1, SWITCH_FRAME_SIZE
>  
> -	/* Reload original LR */
> -	ld	r0, LRSAVE(r1)
> -	mtlr	r0
> -
> +#ifdef CONFIG_LIVEPATCH_64
> +        /* Based on the cmpd above, if the NIP was altered handle livepatch */
> +	bne-	livepatch_handler
> +#endif

Here too.

>  	/* Handle function_graph or go back */
>  	b	ftrace_caller_common
>  


- Naveen

Re: [PATCH v2 08/13] powerpc/ftrace: Prepare PPC64's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Hi Christophe,
Thanks for your work enabling DYNAMIC_FTRACE_WITH_ARGS on powerpc. Sorry 
for the late review on this series, but I have a few comments below.


Christophe Leroy wrote:
> In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
> to handle LIVEPATCH the same way as frace_caller_regs().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
>  1 file changed, 19 insertions(+), 6 deletions(-)

I think we also need to save r1 into pt_regs so that the stack pointer 
is available in the callbacks.

Other than that, a few minor nits below...

> 
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index d636fc755f60..f6f787819273 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -172,14 +172,19 @@ _GLOBAL(ftrace_caller)
>  	addi	r3, r3, function_trace_op@toc@l
>  	ld	r5, 0(r3)
>  
> +#ifdef CONFIG_LIVEPATCH_64
> +	SAVE_GPR(14, r1)
> +	mr	r14,r7		/* remember old NIP */
                    ^ add a space
> +#endif

Please add a blank line here, to match the formatting for the rest of 
this file.

>  	/* Calculate ip from nip-4 into r3 for call below */
>  	subi    r3, r7, MCOUNT_INSN_SIZE
>  
>  	/* Put the original return address in r4 as parent_ip */
> +	std	r0, _LINK(r1)
>  	mr	r4, r0
>  
> -	/* Set pt_regs to NULL */
> -	li	r6, 0
> +	/* Load &pt_regs in r6 for call below */
> +	addi    r6, r1 ,STACK_FRAME_OVERHEAD
                      ^^ incorrect spacing
>  
>  	/* ftrace_call(r3, r4, r5, r6) */
>  .globl ftrace_call
> @@ -189,6 +194,10 @@ ftrace_call:
>  
>  	ld	r3, _NIP(r1)
>  	mtctr	r3

Another blank line here.

> +#ifdef CONFIG_LIVEPATCH_64
> +	cmpd	r14, r3		/* has NIP been altered? */
> +	REST_GPR(14, r1)
> +#endif
>  
>  	/* Restore gprs */
>  	REST_GPRS(3, 10, r1)
> @@ -196,13 +205,17 @@ ftrace_call:
>  	/* Restore callee's TOC */
>  	ld	r2, 24(r1)
>  
> +	/* Restore possibly modified LR */
> +	ld	r0, _LINK(r1)
> +	mtlr	r0
> +
>  	/* Pop our stack frame */
>  	addi	r1, r1, SWITCH_FRAME_SIZE
>  
> -	/* Reload original LR */
> -	ld	r0, LRSAVE(r1)
> -	mtlr	r0
> -
> +#ifdef CONFIG_LIVEPATCH_64
> +        /* Based on the cmpd above, if the NIP was altered handle livepatch */
> +	bne-	livepatch_handler
> +#endif

Here too.

>  	/* Handle function_graph or go back */
>  	b	ftrace_caller_common
>  


- Naveen

[PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.

Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig                 |  1 +
 arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
 arch/powerpc/include/asm/livepatch.h |  4 +---
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if MPROFILE_KERNEL || PPC32
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 struct dyn_arch_ftrace {
 	struct module *mod;
 };
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+	struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+	return &fregs->regs;
+}
+
+static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
+							   unsigned long ip)
+{
+	regs_set_return_ip(&fregs->regs, ip);
+}
+#endif
 #endif /* __ASSEMBLY__ */
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 37af961eb74c..6f10de6af6e3 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -14,9 +14,7 @@
 #ifdef CONFIG_LIVEPATCH
 static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
-	struct pt_regs *regs = ftrace_get_regs(fregs);
-
-	regs_set_return_ip(regs, ip);
+	ftrace_instruction_pointer_set(fregs, ip);
 }
 
 #define klp_get_ftrace_location klp_get_ftrace_location
-- 
2.33.1

[PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.

Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig                 |  1 +
 arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
 arch/powerpc/include/asm/livepatch.h |  4 +---
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if MPROFILE_KERNEL || PPC32
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index b3f6184f77ea..45c3d6f11daa 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 struct dyn_arch_ftrace {
 	struct module *mod;
 };
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+	struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+	return &fregs->regs;
+}
+
+static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
+							   unsigned long ip)
+{
+	regs_set_return_ip(&fregs->regs, ip);
+}
+#endif
 #endif /* __ASSEMBLY__ */
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 37af961eb74c..6f10de6af6e3 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -14,9 +14,7 @@
 #ifdef CONFIG_LIVEPATCH
 static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
-	struct pt_regs *regs = ftrace_get_regs(fregs);
-
-	regs_set_return_ip(regs, ip);
+	ftrace_instruction_pointer_set(fregs, ip);
 }
 
 #define klp_get_ftrace_location klp_get_ftrace_location
-- 
2.33.1

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
> 
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.

Correct. We could replace it ftrace_instruction_pointer_set() and that is 
it. In fact, livepatch.h in both arch/x86/include/asm/ and 
arch/s390/include/asm/ could be removed with that.

On the other hand, there is arm64 live patching support being worked on 
and I am not sure what their plans about DYNAMIC_FTRACE_WITH_ARGS are. The 
above would make it a prerequisite.

Adding CCs... you can find the whole thread at 
https://lore.kernel.org/all/cover.1640017960.git.christophe.leroy@csgroup.eu/

Miroslav

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
> 
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.

Correct. We could replace it ftrace_instruction_pointer_set() and that is 
it. In fact, livepatch.h in both arch/x86/include/asm/ and 
arch/s390/include/asm/ could be removed with that.

On the other hand, there is arm64 live patching support being worked on 
and I am not sure what their plans about DYNAMIC_FTRACE_WITH_ARGS are. The 
above would make it a prerequisite.

Adding CCs... you can find the whole thread at 
https://lore.kernel.org/all/cover.1640017960.git.christophe.leroy@csgroup.eu/

Miroslav

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Miroslav Benes]

On Mon, 20 Dec 2021, Christophe Leroy wrote:

> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
> 
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.

Correct. We could replace it ftrace_instruction_pointer_set() and that is 
it. In fact, livepatch.h in both arch/x86/include/asm/ and 
arch/s390/include/asm/ could be removed with that.

On the other hand, there is arm64 live patching support being worked on 
and I am not sure what their plans about DYNAMIC_FTRACE_WITH_ARGS are. The 
above would make it a prerequisite.

Adding CCs... you can find the whole thread at 
https://lore.kernel.org/all/cover.1640017960.git.christophe.leroy@csgroup.eu/

Miroslav

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Christophe Leroy wrote:
> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
> 
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/Kconfig                 |  1 +
>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>  arch/powerpc/include/asm/livepatch.h |  4 +---
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index cdac2115eb00..e2b1792b2aae 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -210,6 +210,7 @@ config PPC
>  	select HAVE_DEBUG_KMEMLEAK
>  	select HAVE_DEBUG_STACKOVERFLOW
>  	select HAVE_DYNAMIC_FTRACE
> +	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if MPROFILE_KERNEL || PPC32
>  	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
>  	select HAVE_EBPF_JIT
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index b3f6184f77ea..45c3d6f11daa 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>  struct dyn_arch_ftrace {
>  	struct module *mod;
>  };
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> +struct ftrace_regs {
> +	struct pt_regs regs;
> +};
> +
> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
> +{
> +	return &fregs->regs;
> +}

I think this is wrong. We need to differentiate between ftrace_caller() 
and ftrace_regs_caller() here, and only return pt_regs if coming in 
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).

> +
> +static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
> +							   unsigned long ip)
> +{
> +	regs_set_return_ip(&fregs->regs, ip);

Should we use that helper here? regs_set_return_ip() also updates some 
other state related to taking interrupts and I don't think it makes 
sense for use with ftrace.


- Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Christophe Leroy wrote:
> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
> of livepatching.
> 
> Also note that powerpc being the last one to convert to
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
> klp_arch_set_pc() on all architectures.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/Kconfig                 |  1 +
>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>  arch/powerpc/include/asm/livepatch.h |  4 +---
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index cdac2115eb00..e2b1792b2aae 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -210,6 +210,7 @@ config PPC
>  	select HAVE_DEBUG_KMEMLEAK
>  	select HAVE_DEBUG_STACKOVERFLOW
>  	select HAVE_DYNAMIC_FTRACE
> +	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if MPROFILE_KERNEL || PPC32
>  	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
>  	select HAVE_EBPF_JIT
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index b3f6184f77ea..45c3d6f11daa 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>  struct dyn_arch_ftrace {
>  	struct module *mod;
>  };
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> +struct ftrace_regs {
> +	struct pt_regs regs;
> +};
> +
> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *fregs)
> +{
> +	return &fregs->regs;
> +}

I think this is wrong. We need to differentiate between ftrace_caller() 
and ftrace_regs_caller() here, and only return pt_regs if coming in 
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).

> +
> +static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *fregs,
> +							   unsigned long ip)
> +{
> +	regs_set_return_ip(&fregs->regs, ip);

Should we use that helper here? regs_set_return_ip() also updates some 
other state related to taking interrupts and I don't think it makes 
sense for use with ftrace.


- Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>> of livepatching.
>>
>> Also note that powerpc being the last one to convert to
>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>> klp_arch_set_pc() on all architectures.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>  arch/powerpc/Kconfig                 |  1 +
>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index cdac2115eb00..e2b1792b2aae 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -210,6 +210,7 @@ config PPC
>>      select HAVE_DEBUG_KMEMLEAK
>>      select HAVE_DEBUG_STACKOVERFLOW
>>      select HAVE_DYNAMIC_FTRACE
>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
>>      select HAVE_EBPF_JIT
>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN 
>> && POWER7_CPU)
>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>> b/arch/powerpc/include/asm/ftrace.h
>> index b3f6184f77ea..45c3d6f11daa 100644
>> --- a/arch/powerpc/include/asm/ftrace.h
>> +++ b/arch/powerpc/include/asm/ftrace.h
>> @@ -22,6 +22,23 @@ static inline unsigned long 
>> ftrace_call_adjust(unsigned long addr)
>>  struct dyn_arch_ftrace {
>>      struct module *mod;
>>  };
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>> +struct ftrace_regs {
>> +    struct pt_regs regs;
>> +};
>> +
>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>> ftrace_regs *fregs)
>> +{
>> +    return &fregs->regs;
>> +}
> 
> I think this is wrong. We need to differentiate between ftrace_caller() 
> and ftrace_regs_caller() here, and only return pt_regs if coming in 
> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).

Not sure I follow you.

This is based on 5740a7c71ab6 ("s390/ftrace: add 
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")

It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also 
with ftrace_caller().

Sure you only have the params, but that's the same on s390, so what did 
I miss ?


> 
>> +
>> +static __always_inline void ftrace_instruction_pointer_set(struct 
>> ftrace_regs *fregs,
>> +                               unsigned long ip)
>> +{
>> +    regs_set_return_ip(&fregs->regs, ip);
> 
> Should we use that helper here? regs_set_return_ip() also updates some 
> other state related to taking interrupts and I don't think it makes 
> sense for use with ftrace.
> 


Today we have:

	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
long ip)
	{
		struct pt_regs *regs = ftrace_get_regs(fregs);

		regs_set_return_ip(regs, ip);
	}


Which like x86 and s390 becomes:

	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
long ip)
	{
		ftrace_instruction_pointer_set(fregs, ip);
	}



That's the reason why I've been using regs_set_return_ip(). Do you think 
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?

That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR 
registers if they are still valid")

Christophe

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>> of livepatching.
>>
>> Also note that powerpc being the last one to convert to
>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>> klp_arch_set_pc() on all architectures.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>  arch/powerpc/Kconfig                 |  1 +
>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index cdac2115eb00..e2b1792b2aae 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -210,6 +210,7 @@ config PPC
>>      select HAVE_DEBUG_KMEMLEAK
>>      select HAVE_DEBUG_STACKOVERFLOW
>>      select HAVE_DYNAMIC_FTRACE
>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
>>      select HAVE_EBPF_JIT
>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN 
>> && POWER7_CPU)
>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>> b/arch/powerpc/include/asm/ftrace.h
>> index b3f6184f77ea..45c3d6f11daa 100644
>> --- a/arch/powerpc/include/asm/ftrace.h
>> +++ b/arch/powerpc/include/asm/ftrace.h
>> @@ -22,6 +22,23 @@ static inline unsigned long 
>> ftrace_call_adjust(unsigned long addr)
>>  struct dyn_arch_ftrace {
>>      struct module *mod;
>>  };
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>> +struct ftrace_regs {
>> +    struct pt_regs regs;
>> +};
>> +
>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>> ftrace_regs *fregs)
>> +{
>> +    return &fregs->regs;
>> +}
> 
> I think this is wrong. We need to differentiate between ftrace_caller() 
> and ftrace_regs_caller() here, and only return pt_regs if coming in 
> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).

Not sure I follow you.

This is based on 5740a7c71ab6 ("s390/ftrace: add 
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")

It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also 
with ftrace_caller().

Sure you only have the params, but that's the same on s390, so what did 
I miss ?


> 
>> +
>> +static __always_inline void ftrace_instruction_pointer_set(struct 
>> ftrace_regs *fregs,
>> +                               unsigned long ip)
>> +{
>> +    regs_set_return_ip(&fregs->regs, ip);
> 
> Should we use that helper here? regs_set_return_ip() also updates some 
> other state related to taking interrupts and I don't think it makes 
> sense for use with ftrace.
> 


Today we have:

	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
long ip)
	{
		struct pt_regs *regs = ftrace_get_regs(fregs);

		regs_set_return_ip(regs, ip);
	}


Which like x86 and s390 becomes:

	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
long ip)
	{
		ftrace_instruction_pointer_set(fregs, ip);
	}



That's the reason why I've been using regs_set_return_ip(). Do you think 
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?

That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR 
registers if they are still valid")

Christophe

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>> of livepatching.
>>>
>>> Also note that powerpc being the last one to convert to
>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>> klp_arch_set_pc() on all architectures.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>  arch/powerpc/Kconfig                 |  1 +
>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index cdac2115eb00..e2b1792b2aae 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -210,6 +210,7 @@ config PPC
>>>      select HAVE_DEBUG_KMEMLEAK
>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>      select HAVE_DYNAMIC_FTRACE
>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
>>>      select HAVE_EBPF_JIT
>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN 
>>> && POWER7_CPU)
>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>> b/arch/powerpc/include/asm/ftrace.h
>>> index b3f6184f77ea..45c3d6f11daa 100644
>>> --- a/arch/powerpc/include/asm/ftrace.h
>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>> ftrace_call_adjust(unsigned long addr)
>>>  struct dyn_arch_ftrace {
>>>      struct module *mod;
>>>  };
>>> +
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>> +struct ftrace_regs {
>>> +    struct pt_regs regs;
>>> +};
>>> +
>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>>> ftrace_regs *fregs)
>>> +{
>>> +    return &fregs->regs;
>>> +}
>> 
>> I think this is wrong. We need to differentiate between ftrace_caller() 
>> and ftrace_regs_caller() here, and only return pt_regs if coming in 
>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>
> Not sure I follow you.
>
> This is based on 5740a7c71ab6 ("s390/ftrace: add 
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>
> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also 
> with ftrace_caller().
>
> Sure you only have the params, but that's the same on s390, so what did 
> I miss ?

I already have this series in next, I can pull it out, but I'd rather
not.

I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.

>>> +static __always_inline void ftrace_instruction_pointer_set(struct 
>>> ftrace_regs *fregs,
>>> +                               unsigned long ip)
>>> +{
>>> +    regs_set_return_ip(&fregs->regs, ip);
>> 
>> Should we use that helper here? regs_set_return_ip() also updates some 
>> other state related to taking interrupts and I don't think it makes 
>> sense for use with ftrace.
>
>
> Today we have:
>
> 	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
> long ip)
> 	{
> 		struct pt_regs *regs = ftrace_get_regs(fregs);
>
> 		regs_set_return_ip(regs, ip);
> 	}
>
>
> Which like x86 and s390 becomes:
>
> 	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
> long ip)
> 	{
> 		ftrace_instruction_pointer_set(fregs, ip);
> 	}
>
>
>
> That's the reason why I've been using regs_set_return_ip(). Do you think 
> it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
>
> That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR 
> registers if they are still valid")

It's not wrong, but I think it's unnecessary. We need to use
regs_set_return_ip() if we're changing the regs->ip of an interrupt
frame, so that the interrupt return code will reload it.

But AIUI in this case we're not doing that, we're changing the regs->ip
of a pt_regs provided by ftrace, which shouldn't ever be an interrupt
frame.

So it's not a bug to use regs_set_return_ip(), but it is unncessary and
means we'll reload the interrupt state unnecessarily on the next
interrupt return.

cheers

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>> of livepatching.
>>>
>>> Also note that powerpc being the last one to convert to
>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>> klp_arch_set_pc() on all architectures.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>  arch/powerpc/Kconfig                 |  1 +
>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index cdac2115eb00..e2b1792b2aae 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -210,6 +210,7 @@ config PPC
>>>      select HAVE_DEBUG_KMEMLEAK
>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>      select HAVE_DYNAMIC_FTRACE
>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
>>>      select HAVE_EBPF_JIT
>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN 
>>> && POWER7_CPU)
>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>> b/arch/powerpc/include/asm/ftrace.h
>>> index b3f6184f77ea..45c3d6f11daa 100644
>>> --- a/arch/powerpc/include/asm/ftrace.h
>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>> ftrace_call_adjust(unsigned long addr)
>>>  struct dyn_arch_ftrace {
>>>      struct module *mod;
>>>  };
>>> +
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>> +struct ftrace_regs {
>>> +    struct pt_regs regs;
>>> +};
>>> +
>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>>> ftrace_regs *fregs)
>>> +{
>>> +    return &fregs->regs;
>>> +}
>> 
>> I think this is wrong. We need to differentiate between ftrace_caller() 
>> and ftrace_regs_caller() here, and only return pt_regs if coming in 
>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>
> Not sure I follow you.
>
> This is based on 5740a7c71ab6 ("s390/ftrace: add 
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>
> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also 
> with ftrace_caller().
>
> Sure you only have the params, but that's the same on s390, so what did 
> I miss ?

I already have this series in next, I can pull it out, but I'd rather
not.

I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.

>>> +static __always_inline void ftrace_instruction_pointer_set(struct 
>>> ftrace_regs *fregs,
>>> +                               unsigned long ip)
>>> +{
>>> +    regs_set_return_ip(&fregs->regs, ip);
>> 
>> Should we use that helper here? regs_set_return_ip() also updates some 
>> other state related to taking interrupts and I don't think it makes 
>> sense for use with ftrace.
>
>
> Today we have:
>
> 	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
> long ip)
> 	{
> 		struct pt_regs *regs = ftrace_get_regs(fregs);
>
> 		regs_set_return_ip(regs, ip);
> 	}
>
>
> Which like x86 and s390 becomes:
>
> 	static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned 
> long ip)
> 	{
> 		ftrace_instruction_pointer_set(fregs, ip);
> 	}
>
>
>
> That's the reason why I've been using regs_set_return_ip(). Do you think 
> it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
>
> That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR 
> registers if they are still valid")

It's not wrong, but I think it's unnecessary. We need to use
regs_set_return_ip() if we're changing the regs->ip of an interrupt
frame, so that the interrupt return code will reload it.

But AIUI in this case we're not doing that, we're changing the regs->ip
of a pt_regs provided by ftrace, which shouldn't ever be an interrupt
frame.

So it's not a bug to use regs_set_return_ip(), but it is unncessary and
means we'll reload the interrupt state unnecessarily on the next
interrupt return.

cheers

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>> Christophe Leroy wrote:
>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>> of livepatching.
>>>>
>>>> Also note that powerpc being the last one to convert to
>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>> klp_arch_set_pc() on all architectures.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>> --- a/arch/powerpc/Kconfig
>>>> +++ b/arch/powerpc/Kconfig
>>>> @@ -210,6 +210,7 @@ config PPC
>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>      select HAVE_DYNAMIC_FTRACE
>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
>>>>      select HAVE_EBPF_JIT
>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN 
>>>> && POWER7_CPU)
>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>> b/arch/powerpc/include/asm/ftrace.h
>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>> ftrace_call_adjust(unsigned long addr)
>>>>  struct dyn_arch_ftrace {
>>>>      struct module *mod;
>>>>  };
>>>> +
>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>> +struct ftrace_regs {
>>>> +    struct pt_regs regs;
>>>> +};
>>>> +
>>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>>>> ftrace_regs *fregs)
>>>> +{
>>>> +    return &fregs->regs;
>>>> +}
>>> 
>>> I think this is wrong. We need to differentiate between ftrace_caller() 
>>> and ftrace_regs_caller() here, and only return pt_regs if coming in 
>>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>>
>> Not sure I follow you.
>>
>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>
>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also 
>> with ftrace_caller().
>>
>> Sure you only have the params, but that's the same on s390, so what did 
>> I miss ?

It looks like s390 is special since it apparently saves all registers 
even for ftrace_caller: 
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/

As I understand it, the reason ftrace_get_regs() was introduced was to 
be able to only return the pt_regs, if _all_ registers were saved into 
it, which we don't do when coming in through ftrace_caller(). See the 
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for 
arguments to be passed in to ftrace_regs by default"), which returns 
pt_regs conditionally.

> 
> I already have this series in next, I can pull it out, but I'd rather
> not.

Yeah, I'm sorry about the late review on this one.

> 
> I'll leave it in for now, hopefully you two can agree overnight my time
> whether this is a big problem or something we can fix with a fixup
> patch.

I think changes to this particular patch can be added as an incremental 
patch. If anything, pt_regs won't have all valid registers, but no one 
should depend on it without also setting FL_SAVE_REGS anyway.

I was concerned about patch 8 though, where we are missing saving r1 
into pt_regs. That gets used in patch 11, and will be used during 
unwinding when the function_graph tracer is active. But, this should 
still just result in us being unable to unwind the stack, so I think 
that can also be an incremental patch.


Thanks,
Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>> Christophe Leroy wrote:
>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>> of livepatching.
>>>>
>>>> Also note that powerpc being the last one to convert to
>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>> klp_arch_set_pc() on all architectures.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>> --- a/arch/powerpc/Kconfig
>>>> +++ b/arch/powerpc/Kconfig
>>>> @@ -210,6 +210,7 @@ config PPC
>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>      select HAVE_DYNAMIC_FTRACE
>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || PPC32
>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || PPC32
>>>>      select HAVE_EBPF_JIT
>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if !(CPU_LITTLE_ENDIAN 
>>>> && POWER7_CPU)
>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>> b/arch/powerpc/include/asm/ftrace.h
>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>> ftrace_call_adjust(unsigned long addr)
>>>>  struct dyn_arch_ftrace {
>>>>      struct module *mod;
>>>>  };
>>>> +
>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>> +struct ftrace_regs {
>>>> +    struct pt_regs regs;
>>>> +};
>>>> +
>>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>>>> ftrace_regs *fregs)
>>>> +{
>>>> +    return &fregs->regs;
>>>> +}
>>> 
>>> I think this is wrong. We need to differentiate between ftrace_caller() 
>>> and ftrace_regs_caller() here, and only return pt_regs if coming in 
>>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>>
>> Not sure I follow you.
>>
>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>
>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also 
>> with ftrace_caller().
>>
>> Sure you only have the params, but that's the same on s390, so what did 
>> I miss ?

It looks like s390 is special since it apparently saves all registers 
even for ftrace_caller: 
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/

As I understand it, the reason ftrace_get_regs() was introduced was to 
be able to only return the pt_regs, if _all_ registers were saved into 
it, which we don't do when coming in through ftrace_caller(). See the 
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for 
arguments to be passed in to ftrace_regs by default"), which returns 
pt_regs conditionally.

> 
> I already have this series in next, I can pull it out, but I'd rather
> not.

Yeah, I'm sorry about the late review on this one.

> 
> I'll leave it in for now, hopefully you two can agree overnight my time
> whether this is a big problem or something we can fix with a fixup
> patch.

I think changes to this particular patch can be added as an incremental 
patch. If anything, pt_regs won't have all valid registers, but no one 
should depend on it without also setting FL_SAVE_REGS anyway.

I was concerned about patch 8 though, where we are missing saving r1 
into pt_regs. That gets used in patch 11, and will be used during 
unwinding when the function_graph tracer is active. But, this should 
still just result in us being unable to unwind the stack, so I think 
that can also be an incremental patch.


Thanks,
Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
> Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>> Christophe Leroy wrote:
>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>> of livepatching.
>>>>>
>>>>> Also note that powerpc being the last one to convert to
>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>> klp_arch_set_pc() on all architectures.
>>>>>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>> ---
>>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>> --- a/arch/powerpc/Kconfig
>>>>> +++ b/arch/powerpc/Kconfig
>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>>      select HAVE_DYNAMIC_FTRACE
>>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || 
>>>>> PPC32
>>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || 
>>>>> PPC32
>>>>>      select HAVE_EBPF_JIT
>>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if 
>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>  struct dyn_arch_ftrace {
>>>>>      struct module *mod;
>>>>>  };
>>>>> +
>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>> +struct ftrace_regs {
>>>>> +    struct pt_regs regs;
>>>>> +};
>>>>> +
>>>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>>>>> ftrace_regs *fregs)
>>>>> +{
>>>>> +    return &fregs->regs;
>>>>> +}
>>>>
>>>> I think this is wrong. We need to differentiate between 
>>>> ftrace_caller() and ftrace_regs_caller() here, and only return 
>>>> pt_regs if coming in through ftrace_regs_caller() (i.e., 
>>>> FL_SAVE_REGS is set).
>>>
>>> Not sure I follow you.
>>>
>>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>
>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs 
>>> also with ftrace_caller().
>>>
>>> Sure you only have the params, but that's the same on s390, so what 
>>> did I miss ?
> 
> It looks like s390 is special since it apparently saves all registers 
> even for ftrace_caller: 
> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/

It is not what I understand from their code, see 
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37

They have a common macro called with argument 'allregs' which is set to 
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.

But ok, I can do like x86, but I need a trick to know whether 
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?

Thanks
Christophe

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
> Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>> Christophe Leroy wrote:
>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>> of livepatching.
>>>>>
>>>>> Also note that powerpc being the last one to convert to
>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>> klp_arch_set_pc() on all architectures.
>>>>>
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>> ---
>>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>> --- a/arch/powerpc/Kconfig
>>>>> +++ b/arch/powerpc/Kconfig
>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>>      select HAVE_DYNAMIC_FTRACE
>>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || 
>>>>> PPC32
>>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || 
>>>>> PPC32
>>>>>      select HAVE_EBPF_JIT
>>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if 
>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>  struct dyn_arch_ftrace {
>>>>>      struct module *mod;
>>>>>  };
>>>>> +
>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>> +struct ftrace_regs {
>>>>> +    struct pt_regs regs;
>>>>> +};
>>>>> +
>>>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
>>>>> ftrace_regs *fregs)
>>>>> +{
>>>>> +    return &fregs->regs;
>>>>> +}
>>>>
>>>> I think this is wrong. We need to differentiate between 
>>>> ftrace_caller() and ftrace_regs_caller() here, and only return 
>>>> pt_regs if coming in through ftrace_regs_caller() (i.e., 
>>>> FL_SAVE_REGS is set).
>>>
>>> Not sure I follow you.
>>>
>>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>
>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs 
>>> also with ftrace_caller().
>>>
>>> Sure you only have the params, but that's the same on s390, so what 
>>> did I miss ?
> 
> It looks like s390 is special since it apparently saves all registers 
> even for ftrace_caller: 
> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/

It is not what I understand from their code, see 
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37

They have a common macro called with argument 'allregs' which is set to 
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.

But ok, I can do like x86, but I need a trick to know whether 
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?

Thanks
Christophe

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

+ S390 people

Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
> 
> 
> Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
>> Michael Ellerman wrote:
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>>> Christophe Leroy wrote:
>>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>>> of livepatching.
>>>>>>
>>>>>> Also note that powerpc being the last one to convert to
>>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>>> klp_arch_set_pc() on all architectures.
>>>>>>
>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>> ---
>>>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>>> --- a/arch/powerpc/Kconfig
>>>>>> +++ b/arch/powerpc/Kconfig
>>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>>>      select HAVE_DYNAMIC_FTRACE
>>>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || 
>>>>>> PPC32
>>>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || 
>>>>>> PPC32
>>>>>>      select HAVE_EBPF_JIT
>>>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if 
>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>>  struct dyn_arch_ftrace {
>>>>>>      struct module *mod;
>>>>>>  };
>>>>>> +
>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>> +struct ftrace_regs {
>>>>>> +    struct pt_regs regs;
>>>>>> +};
>>>>>> +
>>>>>> +static __always_inline struct pt_regs 
>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>> +{
>>>>>> +    return &fregs->regs;
>>>>>> +}
>>>>>
>>>>> I think this is wrong. We need to differentiate between 
>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return 
>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e., 
>>>>> FL_SAVE_REGS is set).
>>>>
>>>> Not sure I follow you.
>>>>
>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>
>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs 
>>>> also with ftrace_caller().
>>>>
>>>> Sure you only have the params, but that's the same on s390, so what 
>>>> did I miss ?
>>
>> It looks like s390 is special since it apparently saves all registers 
>> even for ftrace_caller: 
>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
> 
> It is not what I understand from their code, see 
> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37 
> 
> 
> They have a common macro called with argument 'allregs' which is set to 
> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> When allregs == 1, the macro seems to save more.
> 
> But ok, I can do like x86, but I need a trick to know whether 
> FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> Any idea what the condition can be for powerpc ?
> 

Finally, it looks like this change is done  via commit 894979689d3a 
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller 
implementations") four hours the same day after the implementation of 
arch_ftrace_get_regs()

They may have forgotten to change arch_ftrace_get_regs() which was added 
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS 
support") with the assumption that ftrace_caller and ftrace_regs_caller 
where identical.

Christophe

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Christophe Leroy]

+ S390 people

Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
> 
> 
> Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
>> Michael Ellerman wrote:
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>>> Christophe Leroy wrote:
>>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>>> of livepatching.
>>>>>>
>>>>>> Also note that powerpc being the last one to convert to
>>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>>> klp_arch_set_pc() on all architectures.
>>>>>>
>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>> ---
>>>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>>> --- a/arch/powerpc/Kconfig
>>>>>> +++ b/arch/powerpc/Kconfig
>>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>>>      select HAVE_DYNAMIC_FTRACE
>>>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || 
>>>>>> PPC32
>>>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || 
>>>>>> PPC32
>>>>>>      select HAVE_EBPF_JIT
>>>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if 
>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>>  struct dyn_arch_ftrace {
>>>>>>      struct module *mod;
>>>>>>  };
>>>>>> +
>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>> +struct ftrace_regs {
>>>>>> +    struct pt_regs regs;
>>>>>> +};
>>>>>> +
>>>>>> +static __always_inline struct pt_regs 
>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>> +{
>>>>>> +    return &fregs->regs;
>>>>>> +}
>>>>>
>>>>> I think this is wrong. We need to differentiate between 
>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return 
>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e., 
>>>>> FL_SAVE_REGS is set).
>>>>
>>>> Not sure I follow you.
>>>>
>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>
>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs 
>>>> also with ftrace_caller().
>>>>
>>>> Sure you only have the params, but that's the same on s390, so what 
>>>> did I miss ?
>>
>> It looks like s390 is special since it apparently saves all registers 
>> even for ftrace_caller: 
>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
> 
> It is not what I understand from their code, see 
> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37 
> 
> 
> They have a common macro called with argument 'allregs' which is set to 
> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> When allregs == 1, the macro seems to save more.
> 
> But ok, I can do like x86, but I need a trick to know whether 
> FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> Any idea what the condition can be for powerpc ?
> 

Finally, it looks like this change is done  via commit 894979689d3a 
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller 
implementations") four hours the same day after the implementation of 
arch_ftrace_get_regs()

They may have forgotten to change arch_ftrace_get_regs() which was added 
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS 
support") with the assumption that ftrace_caller and ftrace_regs_caller 
where identical.

Christophe

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Christophe Leroy wrote:
> + S390 people
> 
> Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
>> 
>> 
>> Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
>>> Michael Ellerman wrote:
>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>>>> Christophe Leroy wrote:
>>>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>>>> of livepatching.
>>>>>>>
>>>>>>> Also note that powerpc being the last one to convert to
>>>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>>>> klp_arch_set_pc() on all architectures.
>>>>>>>
>>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>>> ---
>>>>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>>>> --- a/arch/powerpc/Kconfig
>>>>>>> +++ b/arch/powerpc/Kconfig
>>>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>>>>      select HAVE_DYNAMIC_FTRACE
>>>>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || 
>>>>>>> PPC32
>>>>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || 
>>>>>>> PPC32
>>>>>>>      select HAVE_EBPF_JIT
>>>>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if 
>>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>>>  struct dyn_arch_ftrace {
>>>>>>>      struct module *mod;
>>>>>>>  };
>>>>>>> +
>>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>>> +struct ftrace_regs {
>>>>>>> +    struct pt_regs regs;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static __always_inline struct pt_regs 
>>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>>> +{
>>>>>>> +    return &fregs->regs;
>>>>>>> +}
>>>>>>
>>>>>> I think this is wrong. We need to differentiate between 
>>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return 
>>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e., 
>>>>>> FL_SAVE_REGS is set).
>>>>>
>>>>> Not sure I follow you.
>>>>>
>>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>>
>>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs 
>>>>> also with ftrace_caller().
>>>>>
>>>>> Sure you only have the params, but that's the same on s390, so what 
>>>>> did I miss ?

Steven has explained the rationale for this in his other response:
https://lore.kernel.org/all/20220215093849.556d5444@gandalf.local.home/

>>>
>>> It looks like s390 is special since it apparently saves all registers 
>>> even for ftrace_caller: 
>>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
>> 
>> It is not what I understand from their code, see 
>> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37 
>> 
>> 
>> They have a common macro called with argument 'allregs' which is set to 
>> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
>> When allregs == 1, the macro seems to save more.
>> 
>> But ok, I can do like x86, but I need a trick to know whether 
>> FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
>> Any idea what the condition can be for powerpc ?

We'll need to explicitly zero-out something in pt_regs in 
ftrace_caller(). We can probably use regs->msr since we don't expect it 
to be zero when saved from ftrace_regs_caller().

>> 
> 
> Finally, it looks like this change is done  via commit 894979689d3a 
> ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller 
> implementations") four hours the same day after the implementation of 
> arch_ftrace_get_regs()
> 
> They may have forgotten to change arch_ftrace_get_regs() which was added 
> in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS 
> support") with the assumption that ftrace_caller and ftrace_regs_caller 
> where identical.

Indeed, good find!


Thanks,
Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Christophe Leroy wrote:
> + S390 people
> 
> Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
>> 
>> 
>> Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
>>> Michael Ellerman wrote:
>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>>>>>> Christophe Leroy wrote:
>>>>>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>>>>>> of livepatching.
>>>>>>>
>>>>>>> Also note that powerpc being the last one to convert to
>>>>>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>>>>>> klp_arch_set_pc() on all architectures.
>>>>>>>
>>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>>> ---
>>>>>>>  arch/powerpc/Kconfig                 |  1 +
>>>>>>>  arch/powerpc/include/asm/ftrace.h    | 17 +++++++++++++++++
>>>>>>>  arch/powerpc/include/asm/livepatch.h |  4 +---
>>>>>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>>>>> index cdac2115eb00..e2b1792b2aae 100644
>>>>>>> --- a/arch/powerpc/Kconfig
>>>>>>> +++ b/arch/powerpc/Kconfig
>>>>>>> @@ -210,6 +210,7 @@ config PPC
>>>>>>>      select HAVE_DEBUG_KMEMLEAK
>>>>>>>      select HAVE_DEBUG_STACKOVERFLOW
>>>>>>>      select HAVE_DYNAMIC_FTRACE
>>>>>>> +    select HAVE_DYNAMIC_FTRACE_WITH_ARGS    if MPROFILE_KERNEL || 
>>>>>>> PPC32
>>>>>>>      select HAVE_DYNAMIC_FTRACE_WITH_REGS    if MPROFILE_KERNEL || 
>>>>>>> PPC32
>>>>>>>      select HAVE_EBPF_JIT
>>>>>>>      select HAVE_EFFICIENT_UNALIGNED_ACCESS    if 
>>>>>>> !(CPU_LITTLE_ENDIAN && POWER7_CPU)
>>>>>>> diff --git a/arch/powerpc/include/asm/ftrace.h 
>>>>>>> b/arch/powerpc/include/asm/ftrace.h
>>>>>>> index b3f6184f77ea..45c3d6f11daa 100644
>>>>>>> --- a/arch/powerpc/include/asm/ftrace.h
>>>>>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>>>>>> @@ -22,6 +22,23 @@ static inline unsigned long 
>>>>>>> ftrace_call_adjust(unsigned long addr)
>>>>>>>  struct dyn_arch_ftrace {
>>>>>>>      struct module *mod;
>>>>>>>  };
>>>>>>> +
>>>>>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>>>>>> +struct ftrace_regs {
>>>>>>> +    struct pt_regs regs;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static __always_inline struct pt_regs 
>>>>>>> *arch_ftrace_get_regs(struct ftrace_regs *fregs)
>>>>>>> +{
>>>>>>> +    return &fregs->regs;
>>>>>>> +}
>>>>>>
>>>>>> I think this is wrong. We need to differentiate between 
>>>>>> ftrace_caller() and ftrace_regs_caller() here, and only return 
>>>>>> pt_regs if coming in through ftrace_regs_caller() (i.e., 
>>>>>> FL_SAVE_REGS is set).
>>>>>
>>>>> Not sure I follow you.
>>>>>
>>>>> This is based on 5740a7c71ab6 ("s390/ftrace: add 
>>>>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>>>>
>>>>> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs 
>>>>> also with ftrace_caller().
>>>>>
>>>>> Sure you only have the params, but that's the same on s390, so what 
>>>>> did I miss ?

Steven has explained the rationale for this in his other response:
https://lore.kernel.org/all/20220215093849.556d5444@gandalf.local.home/

>>>
>>> It looks like s390 is special since it apparently saves all registers 
>>> even for ftrace_caller: 
>>> https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
>> 
>> It is not what I understand from their code, see 
>> https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37 
>> 
>> 
>> They have a common macro called with argument 'allregs' which is set to 
>> 0 for ftrace_caller() and 1 for ftrace_regs_caller().
>> When allregs == 1, the macro seems to save more.
>> 
>> But ok, I can do like x86, but I need a trick to know whether 
>> FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
>> Any idea what the condition can be for powerpc ?

We'll need to explicitly zero-out something in pt_regs in 
ftrace_caller(). We can probably use regs->msr since we don't expect it 
to be zero when saved from ftrace_regs_caller().

>> 
> 
> Finally, it looks like this change is done  via commit 894979689d3a 
> ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller 
> implementations") four hours the same day after the implementation of 
> arch_ftrace_get_regs()
> 
> They may have forgotten to change arch_ftrace_get_regs() which was added 
> in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS 
> support") with the assumption that ftrace_caller and ftrace_regs_caller 
> where identical.

Indeed, good find!


Thanks,
Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Heiko Carstens]

On Tue, Feb 15, 2022 at 09:55:52PM +0530, Naveen N. Rao wrote:
> > > > > > > I think this is wrong. We need to differentiate
> > > > > > > between ftrace_caller() and ftrace_regs_caller()
> > > > > > > here, and only return pt_regs if coming in through
> > > > > > > ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
> > > > > > 
> > > > > > Not sure I follow you.
> > > > > > 
> > > > > > This is based on 5740a7c71ab6 ("s390/ftrace: add
> > > > > > HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> > > > > > 
> > > > > > It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS,
> > > > > > have the regs also with ftrace_caller().
> > > > > > 
> > > > > > Sure you only have the params, but that's the same on
> > > > > > s390, so what did I miss ?
> 
> Steven has explained the rationale for this in his other response:
> https://lore.kernel.org/all/20220215093849.556d5444@gandalf.local.home/

Thanks for this pointer, this clarifies a couple of things!

> > > > It looks like s390 is special since it apparently saves all
> > > > registers even for ftrace_caller:
> > > > https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
> > > 
> > > It is not what I understand from their code, see https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
> > > 
> > > 
> > > They have a common macro called with argument 'allregs' which is set
> > > to 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> > > When allregs == 1, the macro seems to save more.
> > > 
> > > But ok, I can do like x86, but I need a trick to know whether
> > > FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> > > Any idea what the condition can be for powerpc ?
> 
> We'll need to explicitly zero-out something in pt_regs in ftrace_caller().
> We can probably use regs->msr since we don't expect it to be zero when saved
> from ftrace_regs_caller().
> > 
> > Finally, it looks like this change is done  via commit 894979689d3a
> > ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
> > implementations") four hours the same day after the implementation of
> > arch_ftrace_get_regs()
> > 
> > They may have forgotten to change arch_ftrace_get_regs() which was added
> > in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > support") with the assumption that ftrace_caller and ftrace_regs_caller
> > where identical.
> 
> Indeed, good find!

Thank you for bringing this up!

So, the in both variants s390 provides nearly identical data. The only
difference is that for FL_SAVE_REGS the program status word mask is
missing; therefore it is not possible to figure out the condition code
or if interrupts were enabled/disabled.

Vasily, Sven, I think we have two options here:

- don't provide sane psw mask contents at all and say (again) that
  ptregs contents are identical

- provide (finally) a full psw mask contents using epsw, and indicate
  validity with a flags bit in pt_regs

I would vote for the second option, even though epsw is slow. But this
is about the third or fourth time this came up in different
contexts. So I'd guess we should go for the slow but complete
solution. Opinions?

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Heiko Carstens]

On Tue, Feb 15, 2022 at 09:55:52PM +0530, Naveen N. Rao wrote:
> > > > > > > I think this is wrong. We need to differentiate
> > > > > > > between ftrace_caller() and ftrace_regs_caller()
> > > > > > > here, and only return pt_regs if coming in through
> > > > > > > ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
> > > > > > 
> > > > > > Not sure I follow you.
> > > > > > 
> > > > > > This is based on 5740a7c71ab6 ("s390/ftrace: add
> > > > > > HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> > > > > > 
> > > > > > It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS,
> > > > > > have the regs also with ftrace_caller().
> > > > > > 
> > > > > > Sure you only have the params, but that's the same on
> > > > > > s390, so what did I miss ?
> 
> Steven has explained the rationale for this in his other response:
> https://lore.kernel.org/all/20220215093849.556d5444@gandalf.local.home/

Thanks for this pointer, this clarifies a couple of things!

> > > > It looks like s390 is special since it apparently saves all
> > > > registers even for ftrace_caller:
> > > > https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
> > > 
> > > It is not what I understand from their code, see https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
> > > 
> > > 
> > > They have a common macro called with argument 'allregs' which is set
> > > to 0 for ftrace_caller() and 1 for ftrace_regs_caller().
> > > When allregs == 1, the macro seems to save more.
> > > 
> > > But ok, I can do like x86, but I need a trick to know whether
> > > FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
> > > Any idea what the condition can be for powerpc ?
> 
> We'll need to explicitly zero-out something in pt_regs in ftrace_caller().
> We can probably use regs->msr since we don't expect it to be zero when saved
> from ftrace_regs_caller().
> > 
> > Finally, it looks like this change is done  via commit 894979689d3a
> > ("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
> > implementations") four hours the same day after the implementation of
> > arch_ftrace_get_regs()
> > 
> > They may have forgotten to change arch_ftrace_get_regs() which was added
> > in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > support") with the assumption that ftrace_caller and ftrace_regs_caller
> > where identical.
> 
> Indeed, good find!

Thank you for bringing this up!

So, the in both variants s390 provides nearly identical data. The only
difference is that for FL_SAVE_REGS the program status word mask is
missing; therefore it is not possible to figure out the condition code
or if interrupts were enabled/disabled.

Vasily, Sven, I think we have two options here:

- don't provide sane psw mask contents at all and say (again) that
  ptregs contents are identical

- provide (finally) a full psw mask contents using epsw, and indicate
  validity with a flags bit in pt_regs

I would vote for the second option, even though epsw is slow. But this
is about the third or fourth time this came up in different
contexts. So I'd guess we should go for the slow but complete
solution. Opinions?

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Sven Schnelle]

Heiko Carstens <hca@linux.ibm.com> writes:

> So, the in both variants s390 provides nearly identical data. The only
> difference is that for FL_SAVE_REGS the program status word mask is
> missing; therefore it is not possible to figure out the condition code
> or if interrupts were enabled/disabled.
>
> Vasily, Sven, I think we have two options here:
>
> - don't provide sane psw mask contents at all and say (again) that
>   ptregs contents are identical
>
> - provide (finally) a full psw mask contents using epsw, and indicate
>   validity with a flags bit in pt_regs
>
> I would vote for the second option, even though epsw is slow. But this
> is about the third or fourth time this came up in different
> contexts. So I'd guess we should go for the slow but complete
> solution. Opinions?

Given that this only affects ftrace_regs_caller, i would also vote for the
second option.

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Sven Schnelle]

Heiko Carstens <hca@linux.ibm.com> writes:

> So, the in both variants s390 provides nearly identical data. The only
> difference is that for FL_SAVE_REGS the program status word mask is
> missing; therefore it is not possible to figure out the condition code
> or if interrupts were enabled/disabled.
>
> Vasily, Sven, I think we have two options here:
>
> - don't provide sane psw mask contents at all and say (again) that
>   ptregs contents are identical
>
> - provide (finally) a full psw mask contents using epsw, and indicate
>   validity with a flags bit in pt_regs
>
> I would vote for the second option, even though epsw is slow. But this
> is about the third or fourth time this came up in different
> contexts. So I'd guess we should go for the slow but complete
> solution. Opinions?

Given that this only affects ftrace_regs_caller, i would also vote for the
second option.

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Steven Rostedt]

On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> As I understand it, the reason ftrace_get_regs() was introduced was to 
> be able to only return the pt_regs, if _all_ registers were saved into 
> it, which we don't do when coming in through ftrace_caller(). See the 
> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for 
> arguments to be passed in to ftrace_regs by default"), which returns 
> pt_regs conditionally.

I can give you the history of ftrace_caller and ftrace_regs_caller.

ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.

As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.

I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.

The first user to use the args was live kernel patching, as they only need
that and the return pointer.

-- Steve

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Steven Rostedt]

On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> As I understand it, the reason ftrace_get_regs() was introduced was to 
> be able to only return the pt_regs, if _all_ registers were saved into 
> it, which we don't do when coming in through ftrace_caller(). See the 
> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for 
> arguments to be passed in to ftrace_regs by default"), which returns 
> pt_regs conditionally.

I can give you the history of ftrace_caller and ftrace_regs_caller.

ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.

As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.

I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.

The first user to use the args was live kernel patching, as they only need
that and the return pointer.

-- Steve

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Steven Rostedt wrote:
> On Tue, 15 Feb 2022 19:06:48 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> As I understand it, the reason ftrace_get_regs() was introduced was to 
>> be able to only return the pt_regs, if _all_ registers were saved into 
>> it, which we don't do when coming in through ftrace_caller(). See the 
>> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for 
>> arguments to be passed in to ftrace_regs by default"), which returns 
>> pt_regs conditionally.
> 
> I can give you the history of ftrace_caller and ftrace_regs_caller.
> 
> ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
> The new fentry which happens at the start of the function, whereas mcount
> happens after the stack frame is set up, may change the rules on some
> architectures.
> 
> As for ftrace_regs_caller, that was created for kprobes. As the majority of
> kprobes were added at the start of the function, it made sense to hook into
> ftrace as the ftrace trampoline call is much faster than taking a
> breakpoint interrupt. But to keep compatibility with breakpoint
> interrupts, we needed to fill in all the registers, and make it act just
> like a breakpoint interrupt.
> 
> I've been wanting to record function parameters, and because the ftrace
> trampoline must at a minimum save the function parameters before calling
> the ftrace callbacks, all the information for those parameters were being
> saved but were never exposed to the ftrace callbacks. I created the the
> DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
> just the parameters filled in, but that was criticized as it could be
> confusing where the non filled in pt_regs might be used and thinking they
> are legitimate. So I created ftrace_regs that would give you just the
> function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
> give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
> not, it will give you a NULL pointer.
> 
> The first user to use the args was live kernel patching, as they only need
> that and the return pointer.

Thanks, that helps.

- Naveen

Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS

[Naveen N. Rao]

Steven Rostedt wrote:
> On Tue, 15 Feb 2022 19:06:48 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> As I understand it, the reason ftrace_get_regs() was introduced was to 
>> be able to only return the pt_regs, if _all_ registers were saved into 
>> it, which we don't do when coming in through ftrace_caller(). See the 
>> x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for 
>> arguments to be passed in to ftrace_regs by default"), which returns 
>> pt_regs conditionally.
> 
> I can give you the history of ftrace_caller and ftrace_regs_caller.
> 
> ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
> The new fentry which happens at the start of the function, whereas mcount
> happens after the stack frame is set up, may change the rules on some
> architectures.
> 
> As for ftrace_regs_caller, that was created for kprobes. As the majority of
> kprobes were added at the start of the function, it made sense to hook into
> ftrace as the ftrace trampoline call is much faster than taking a
> breakpoint interrupt. But to keep compatibility with breakpoint
> interrupts, we needed to fill in all the registers, and make it act just
> like a breakpoint interrupt.
> 
> I've been wanting to record function parameters, and because the ftrace
> trampoline must at a minimum save the function parameters before calling
> the ftrace callbacks, all the information for those parameters were being
> saved but were never exposed to the ftrace callbacks. I created the the
> DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
> just the parameters filled in, but that was criticized as it could be
> confusing where the non filled in pt_regs might be used and thinking they
> are legitimate. So I created ftrace_regs that would give you just the
> function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
> give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
> not, it will give you a NULL pointer.
> 
> The first user to use the args was live kernel patching, as they only need
> that and the return pointer.

Thanks, that helps.

- Naveen

[PATCH v2 10/13] powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller

[Christophe Leroy]

ftrace_enable_ftrace_graph_caller() and
ftrace_disable_ftrace_graph_caller() have common code.

They will have even more common code after following patch.

Refactor into a single ftrace_modify_ftrace_graph_caller() function.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 80b6285769f2..ce673764cb69 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -910,30 +910,27 @@ int __init ftrace_dyn_arch_init(void)
 extern void ftrace_graph_call(void);
 extern void ftrace_graph_stub(void);
 
-int ftrace_enable_ftrace_graph_caller(void)
+static int ftrace_modify_ftrace_graph_caller(bool enable)
 {
 	unsigned long ip = (unsigned long)(&ftrace_graph_call);
 	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
 	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
 	ppc_inst_t old, new;
 
-	old = ftrace_call_replace(ip, stub, 0);
-	new = ftrace_call_replace(ip, addr, 0);
+	old = ftrace_call_replace(ip, enable ? stub : addr, 0);
+	new = ftrace_call_replace(ip, enable ? addr : stub, 0);
 
 	return ftrace_modify_code(ip, old, new);
 }
 
-int ftrace_disable_ftrace_graph_caller(void)
+int ftrace_enable_ftrace_graph_caller(void)
 {
-	unsigned long ip = (unsigned long)(&ftrace_graph_call);
-	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
-	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	ppc_inst_t old, new;
-
-	old = ftrace_call_replace(ip, addr, 0);
-	new = ftrace_call_replace(ip, stub, 0);
+	return ftrace_modify_ftrace_graph_caller(true);
+}
 
-	return ftrace_modify_code(ip, old, new);
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	return ftrace_modify_ftrace_graph_caller(false);
 }
 
 /*
-- 
2.33.1

[PATCH v2 10/13] powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller

[Christophe Leroy]

ftrace_enable_ftrace_graph_caller() and
ftrace_disable_ftrace_graph_caller() have common code.

They will have even more common code after following patch.

Refactor into a single ftrace_modify_ftrace_graph_caller() function.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 80b6285769f2..ce673764cb69 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -910,30 +910,27 @@ int __init ftrace_dyn_arch_init(void)
 extern void ftrace_graph_call(void);
 extern void ftrace_graph_stub(void);
 
-int ftrace_enable_ftrace_graph_caller(void)
+static int ftrace_modify_ftrace_graph_caller(bool enable)
 {
 	unsigned long ip = (unsigned long)(&ftrace_graph_call);
 	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
 	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
 	ppc_inst_t old, new;
 
-	old = ftrace_call_replace(ip, stub, 0);
-	new = ftrace_call_replace(ip, addr, 0);
+	old = ftrace_call_replace(ip, enable ? stub : addr, 0);
+	new = ftrace_call_replace(ip, enable ? addr : stub, 0);
 
 	return ftrace_modify_code(ip, old, new);
 }
 
-int ftrace_disable_ftrace_graph_caller(void)
+int ftrace_enable_ftrace_graph_caller(void)
 {
-	unsigned long ip = (unsigned long)(&ftrace_graph_call);
-	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
-	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	ppc_inst_t old, new;
-
-	old = ftrace_call_replace(ip, addr, 0);
-	new = ftrace_call_replace(ip, stub, 0);
+	return ftrace_modify_ftrace_graph_caller(true);
+}
 
-	return ftrace_modify_code(ip, old, new);
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	return ftrace_modify_ftrace_graph_caller(false);
 }
 
 /*
-- 
2.33.1

[PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller

[Christophe Leroy]

Modify function graph tracer to be handled directly by the standard
ftrace caller.

This is made possible as powerpc now supports
CONFIG_DYNAMIC_FTRACE_WITH_ARGS.

This change simplifies the call of function graph ftrace.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/ftrace.h             |  6 ++
 arch/powerpc/kernel/trace/ftrace.c            | 11 ++++
 arch/powerpc/kernel/trace/ftrace_32.S         | 53 +--------------
 .../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
 4 files changed, 20 insertions(+), 114 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index 45c3d6f11daa..70b457097098 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
 {
 	regs_set_return_ip(&fregs->regs, ip);
 }
+
+struct ftrace_ops;
+
+#define ftrace_graph_func ftrace_graph_func
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
 #endif
 #endif /* __ASSEMBLY__ */
 
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index ce673764cb69..74a176e394ef 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
 	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
 	ppc_inst_t old, new;
 
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
+		return 0;
+
 	old = ftrace_call_replace(ip, enable ? stub : addr, 0);
 	new = ftrace_call_replace(ip, enable ? addr : stub, 0);
 
@@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
 out:
 	return parent;
 }
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	fregs->regs.link = prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]);
+}
+#endif
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
 #ifdef PPC64_ELF_ABI_v1
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index c4055b41af5f..2b425da97a6b 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -59,13 +59,6 @@ ftrace_call:
 	mtlr	r0
 
 	addi	r1, r1, INT_FRAME_SIZE
-ftrace_caller_common:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
-	b	ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
 	/* old link register ends up in ctr reg */
 	bctr
 
@@ -135,52 +128,10 @@ ftrace_regs_call:
 
 	/* Pop our stack frame */
 	addi r1, r1, INT_FRAME_SIZE
-
-	b	ftrace_caller_common
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
-	stwu	r1,-48(r1)
-	stw	r3, 12(r1)
-	stw	r4, 16(r1)
-	stw	r5, 20(r1)
-	stw	r6, 24(r1)
-	stw	r7, 28(r1)
-	stw	r8, 32(r1)
-	stw	r9, 36(r1)
-	stw	r10,40(r1)
-
-	addi	r5, r1, 48
-	mfctr	r4		/* ftrace_caller has moved local addr here */
-	stw	r4, 44(r1)
-	mflr	r3		/* ftrace_caller has restored LR from stack */
-	subi	r4, r4, MCOUNT_INSN_SIZE
-
-	bl	prepare_ftrace_return
-	nop
-
-        /*
-         * prepare_ftrace_return gives us the address we divert to.
-         * Change the LR in the callers stack frame to this.
-         */
-	stw	r3,52(r1)
-	mtlr	r3
-	lwz	r0,44(r1)
-	mtctr	r0
-
-	lwz	r3, 12(r1)
-	lwz	r4, 16(r1)
-	lwz	r5, 20(r1)
-	lwz	r6, 24(r1)
-	lwz	r7, 28(r1)
-	lwz	r8, 32(r1)
-	lwz	r9, 36(r1)
-	lwz	r10,40(r1)
-
-	addi	r1, r1, 48
-
+	/* old link register ends up in ctr reg */
 	bctr
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 _GLOBAL(return_to_handler)
 	/* need to save return values */
 	stwu	r1, -16(r1)
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index f6f787819273..6071e0122797 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -124,15 +124,6 @@ ftrace_regs_call:
         /* Based on the cmpd above, if the NIP was altered handle livepatch */
 	bne-	livepatch_handler
 #endif
-
-ftrace_caller_common:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
-	b	ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
-
 	bctr			/* jump after _mcount site */
 
 _GLOBAL(ftrace_stub)
@@ -216,8 +207,7 @@ ftrace_call:
         /* Based on the cmpd above, if the NIP was altered handle livepatch */
 	bne-	livepatch_handler
 #endif
-	/* Handle function_graph or go back */
-	b	ftrace_caller_common
+	bctr			/* jump after _mcount site */
 
 #ifdef CONFIG_LIVEPATCH
 	/*
@@ -286,55 +276,3 @@ livepatch_handler:
 	/* Return to original caller of live patched function */
 	blr
 #endif /* CONFIG_LIVEPATCH */
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
-	stdu	r1, -112(r1)
-	/* with -mprofile-kernel, parameter regs are still alive at _mcount */
-	std	r10, 104(r1)
-	std	r9, 96(r1)
-	std	r8, 88(r1)
-	std	r7, 80(r1)
-	std	r6, 72(r1)
-	std	r5, 64(r1)
-	std	r4, 56(r1)
-	std	r3, 48(r1)
-
-	/* Save callee's TOC in the ABI compliant location */
-	std	r2, 24(r1)
-	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
-
-	addi	r5, r1, 112
-	mfctr	r4		/* ftrace_caller has moved local addr here */
-	std	r4, 40(r1)
-	mflr	r3		/* ftrace_caller has restored LR from stack */
-	subi	r4, r4, MCOUNT_INSN_SIZE
-
-	bl	prepare_ftrace_return
-	nop
-
-	/*
-	 * prepare_ftrace_return gives us the address we divert to.
-	 * Change the LR to this.
-	 */
-	mtlr	r3
-
-	ld	r0, 40(r1)
-	mtctr	r0
-	ld	r10, 104(r1)
-	ld	r9, 96(r1)
-	ld	r8, 88(r1)
-	ld	r7, 80(r1)
-	ld	r6, 72(r1)
-	ld	r5, 64(r1)
-	ld	r4, 56(r1)
-	ld	r3, 48(r1)
-
-	/* Restore callee's TOC */
-	ld	r2, 24(r1)
-
-	addi	r1, r1, 112
-	mflr	r0
-	std	r0, LRSAVE(r1)
-	bctr
-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.33.1

[PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller

[Christophe Leroy]

Modify function graph tracer to be handled directly by the standard
ftrace caller.

This is made possible as powerpc now supports
CONFIG_DYNAMIC_FTRACE_WITH_ARGS.

This change simplifies the call of function graph ftrace.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/ftrace.h             |  6 ++
 arch/powerpc/kernel/trace/ftrace.c            | 11 ++++
 arch/powerpc/kernel/trace/ftrace_32.S         | 53 +--------------
 .../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
 4 files changed, 20 insertions(+), 114 deletions(-)

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index 45c3d6f11daa..70b457097098 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
 {
 	regs_set_return_ip(&fregs->regs, ip);
 }
+
+struct ftrace_ops;
+
+#define ftrace_graph_func ftrace_graph_func
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
 #endif
 #endif /* __ASSEMBLY__ */
 
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index ce673764cb69..74a176e394ef 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
 	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
 	ppc_inst_t old, new;
 
+	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
+		return 0;
+
 	old = ftrace_call_replace(ip, enable ? stub : addr, 0);
 	new = ftrace_call_replace(ip, enable ? addr : stub, 0);
 
@@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
 out:
 	return parent;
 }
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	fregs->regs.link = prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]);
+}
+#endif
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
 #ifdef PPC64_ELF_ABI_v1
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index c4055b41af5f..2b425da97a6b 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -59,13 +59,6 @@ ftrace_call:
 	mtlr	r0
 
 	addi	r1, r1, INT_FRAME_SIZE
-ftrace_caller_common:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
-	b	ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
 	/* old link register ends up in ctr reg */
 	bctr
 
@@ -135,52 +128,10 @@ ftrace_regs_call:
 
 	/* Pop our stack frame */
 	addi r1, r1, INT_FRAME_SIZE
-
-	b	ftrace_caller_common
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
-	stwu	r1,-48(r1)
-	stw	r3, 12(r1)
-	stw	r4, 16(r1)
-	stw	r5, 20(r1)
-	stw	r6, 24(r1)
-	stw	r7, 28(r1)
-	stw	r8, 32(r1)
-	stw	r9, 36(r1)
-	stw	r10,40(r1)
-
-	addi	r5, r1, 48
-	mfctr	r4		/* ftrace_caller has moved local addr here */
-	stw	r4, 44(r1)
-	mflr	r3		/* ftrace_caller has restored LR from stack */
-	subi	r4, r4, MCOUNT_INSN_SIZE
-
-	bl	prepare_ftrace_return
-	nop
-
-        /*
-         * prepare_ftrace_return gives us the address we divert to.
-         * Change the LR in the callers stack frame to this.
-         */
-	stw	r3,52(r1)
-	mtlr	r3
-	lwz	r0,44(r1)
-	mtctr	r0
-
-	lwz	r3, 12(r1)
-	lwz	r4, 16(r1)
-	lwz	r5, 20(r1)
-	lwz	r6, 24(r1)
-	lwz	r7, 28(r1)
-	lwz	r8, 32(r1)
-	lwz	r9, 36(r1)
-	lwz	r10,40(r1)
-
-	addi	r1, r1, 48
-
+	/* old link register ends up in ctr reg */
 	bctr
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 _GLOBAL(return_to_handler)
 	/* need to save return values */
 	stwu	r1, -16(r1)
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index f6f787819273..6071e0122797 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -124,15 +124,6 @@ ftrace_regs_call:
         /* Based on the cmpd above, if the NIP was altered handle livepatch */
 	bne-	livepatch_handler
 #endif
-
-ftrace_caller_common:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
-	b	ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
-
 	bctr			/* jump after _mcount site */
 
 _GLOBAL(ftrace_stub)
@@ -216,8 +207,7 @@ ftrace_call:
         /* Based on the cmpd above, if the NIP was altered handle livepatch */
 	bne-	livepatch_handler
 #endif
-	/* Handle function_graph or go back */
-	b	ftrace_caller_common
+	bctr			/* jump after _mcount site */
 
 #ifdef CONFIG_LIVEPATCH
 	/*
@@ -286,55 +276,3 @@ livepatch_handler:
 	/* Return to original caller of live patched function */
 	blr
 #endif /* CONFIG_LIVEPATCH */
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
-	stdu	r1, -112(r1)
-	/* with -mprofile-kernel, parameter regs are still alive at _mcount */
-	std	r10, 104(r1)
-	std	r9, 96(r1)
-	std	r8, 88(r1)
-	std	r7, 80(r1)
-	std	r6, 72(r1)
-	std	r5, 64(r1)
-	std	r4, 56(r1)
-	std	r3, 48(r1)
-
-	/* Save callee's TOC in the ABI compliant location */
-	std	r2, 24(r1)
-	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
-
-	addi	r5, r1, 112
-	mfctr	r4		/* ftrace_caller has moved local addr here */
-	std	r4, 40(r1)
-	mflr	r3		/* ftrace_caller has restored LR from stack */
-	subi	r4, r4, MCOUNT_INSN_SIZE
-
-	bl	prepare_ftrace_return
-	nop
-
-	/*
-	 * prepare_ftrace_return gives us the address we divert to.
-	 * Change the LR to this.
-	 */
-	mtlr	r3
-
-	ld	r0, 40(r1)
-	mtctr	r0
-	ld	r10, 104(r1)
-	ld	r9, 96(r1)
-	ld	r8, 88(r1)
-	ld	r7, 80(r1)
-	ld	r6, 72(r1)
-	ld	r5, 64(r1)
-	ld	r4, 56(r1)
-	ld	r3, 48(r1)
-
-	/* Restore callee's TOC */
-	ld	r2, 24(r1)
-
-	addi	r1, r1, 112
-	mflr	r0
-	std	r0, LRSAVE(r1)
-	bctr
-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.33.1

Re: [PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller

[Naveen N. Rao]

Christophe Leroy wrote:
> Modify function graph tracer to be handled directly by the standard
> ftrace caller.
> 
> This is made possible as powerpc now supports
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
> 
> This change simplifies the call of function graph ftrace.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/ftrace.h             |  6 ++
>  arch/powerpc/kernel/trace/ftrace.c            | 11 ++++
>  arch/powerpc/kernel/trace/ftrace_32.S         | 53 +--------------
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
>  4 files changed, 20 insertions(+), 114 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index 45c3d6f11daa..70b457097098 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
>  {
>  	regs_set_return_ip(&fregs->regs, ip);
>  }
> +
> +struct ftrace_ops;
> +
> +#define ftrace_graph_func ftrace_graph_func
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs);
>  #endif
>  #endif /* __ASSEMBLY__ */
>  
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index ce673764cb69..74a176e394ef 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
>  	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
>  	ppc_inst_t old, new;
>  
> +	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
> +		return 0;
> +
>  	old = ftrace_call_replace(ip, enable ? stub : addr, 0);
>  	new = ftrace_call_replace(ip, enable ? addr : stub, 0);
>  
> @@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>  out:
>  	return parent;
>  }

For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use 
ftrace directly") also adds recursion check before the call to 
function_graph_enter() in prepare_ftrace_return(). Do we need that on
powerpc as well?


- Naveen

Re: [PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller

[Naveen N. Rao]

Christophe Leroy wrote:
> Modify function graph tracer to be handled directly by the standard
> ftrace caller.
> 
> This is made possible as powerpc now supports
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
> 
> This change simplifies the call of function graph ftrace.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/ftrace.h             |  6 ++
>  arch/powerpc/kernel/trace/ftrace.c            | 11 ++++
>  arch/powerpc/kernel/trace/ftrace_32.S         | 53 +--------------
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
>  4 files changed, 20 insertions(+), 114 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index 45c3d6f11daa..70b457097098 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
>  {
>  	regs_set_return_ip(&fregs->regs, ip);
>  }
> +
> +struct ftrace_ops;
> +
> +#define ftrace_graph_func ftrace_graph_func
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs);
>  #endif
>  #endif /* __ASSEMBLY__ */
>  
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index ce673764cb69..74a176e394ef 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
>  	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
>  	ppc_inst_t old, new;
>  
> +	if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
> +		return 0;
> +
>  	old = ftrace_call_replace(ip, enable ? stub : addr, 0);
>  	new = ftrace_call_replace(ip, enable ? addr : stub, 0);
>  
> @@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>  out:
>  	return parent;
>  }

For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use 
ftrace directly") also adds recursion check before the call to 
function_graph_enter() in prepare_ftrace_return(). Do we need that on
powerpc as well?


- Naveen

Re: [PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller

[Steven Rostedt]

On Mon, 14 Feb 2022 22:54:23 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use 
> ftrace directly") also adds recursion check before the call to 
> function_graph_enter() in prepare_ftrace_return(). Do we need that on
> powerpc as well?

Yes. The function_graph_enter() does not provide any recursion protection,
so if it were to call something that gets function graph traced, it will
crash the machine.

-- Steve

Re: [PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller

[Steven Rostedt]

On Mon, 14 Feb 2022 22:54:23 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use 
> ftrace directly") also adds recursion check before the call to 
> function_graph_enter() in prepare_ftrace_return(). Do we need that on
> powerpc as well?

Yes. The function_graph_enter() does not provide any recursion protection,
so if it were to call something that gets function graph traced, it will
crash the machine.

-- Steve

[PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32

[Christophe Leroy]

PPC64 mprofile versions and PPC32 are very similar.

Modify PPC64 version so that if can be reused for PPC32.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index 6071e0122797..56da60e98327 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -34,13 +34,16 @@
  */
 _GLOBAL(ftrace_regs_caller)
 	/* Save the original return address in A's stack frame */
-	std	r0,LRSAVE(r1)
+#ifdef CONFIG_MPROFILE_KERNEL
+	PPC_STL	r0,LRSAVE(r1)
+#endif
 
 	/* Create our stack frame + pt_regs */
-	stdu	r1,-SWITCH_FRAME_SIZE(r1)
+	PPC_STLU	r1,-SWITCH_FRAME_SIZE(r1)
 
 	/* Save all gprs to pt_regs */
 	SAVE_GPR(0, r1)
+#ifdef CONFIG_PPC64
 	SAVE_GPRS(2, 11, r1)
 
 	/* Ok to continue? */
@@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
 	beq	ftrace_no_trace
 
 	SAVE_GPRS(12, 31, r1)
+#else
+	stmw	r2, GPR2(r1)
+#endif
 
 	/* Save previous stack pointer (r1) */
 	addi	r8, r1, SWITCH_FRAME_SIZE
-	std	r8, GPR1(r1)
+	PPC_STL	r8, GPR1(r1)
 
 	/* Load special regs for save below */
 	mfmsr   r8
@@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
 	/* Get the _mcount() call site out of LR */
 	mflr	r7
 	/* Save it as pt_regs->nip */
-	std     r7, _NIP(r1)
+	PPC_STL	r7, _NIP(r1)
 	/* Save the read LR in pt_regs->link */
-	std     r0, _LINK(r1)
+	PPC_STL	r0, _LINK(r1)
 
+#ifdef CONFIG_PPC64
 	/* Save callee's TOC in the ABI compliant location */
 	std	r2, 24(r1)
 	ld	r2,PACATOC(r13)	/* get kernel TOC in r2 */
@@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
 	addis	r3,r2,function_trace_op@toc@ha
 	addi	r3,r3,function_trace_op@toc@l
 	ld	r5,0(r3)
+#else
+	lis	r3,function_trace_op@ha
+	lwz	r5,function_trace_op@l(r3)
+#endif
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	mr	r14,r7		/* remember old NIP */
 #endif
 	/* Calculate ip from nip-4 into r3 for call below */
@@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
 	mr	r4, r0
 
 	/* Save special regs */
-	std     r8, _MSR(r1)
-	std     r9, _CTR(r1)
-	std     r10, _XER(r1)
-	std     r11, _CCR(r1)
+	PPC_STL	r8, _MSR(r1)
+	PPC_STL	r9, _CTR(r1)
+	PPC_STL	r10, _XER(r1)
+	PPC_STL	r11, _CCR(r1)
 
 	/* Load &pt_regs in r6 for call below */
 	addi    r6, r1 ,STACK_FRAME_OVERHEAD
@@ -100,27 +111,32 @@ ftrace_regs_call:
 	nop
 
 	/* Load ctr with the possibly modified NIP */
-	ld	r3, _NIP(r1)
+	PPC_LL	r3, _NIP(r1)
 	mtctr	r3
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	cmpd	r14, r3		/* has NIP been altered? */
 #endif
 
 	/* Restore gprs */
-	REST_GPR(0, r1)
+#ifdef CONFIG_PPC64
 	REST_GPRS(2, 31, r1)
+#else
+	lmw	r2, GPR2(r1)
+#endif
 
 	/* Restore possibly modified LR */
-	ld	r0, _LINK(r1)
+	PPC_LL	r0, _LINK(r1)
 	mtlr	r0
 
+#ifdef CONFIG_PPC64
 	/* Restore callee's TOC */
 	ld	r2, 24(r1)
+#endif
 
 	/* Pop our stack frame */
 	addi r1, r1, SWITCH_FRAME_SIZE
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
         /* Based on the cmpd above, if the NIP was altered handle livepatch */
 	bne-	livepatch_handler
 #endif
@@ -129,6 +145,7 @@ ftrace_regs_call:
 _GLOBAL(ftrace_stub)
 	blr
 
+#ifdef CONFIG_PPC64
 ftrace_no_trace:
 	mflr	r3
 	mtctr	r3
@@ -136,25 +153,31 @@ ftrace_no_trace:
 	addi	r1, r1, SWITCH_FRAME_SIZE
 	mtlr	r0
 	bctr
+#endif
 
 _GLOBAL(ftrace_caller)
 	/* Save the original return address in A's stack frame */
-	std	r0, LRSAVE(r1)
+#ifdef CONFIG_MPROFILE_KERNEL
+	PPC_STL	r0, LRSAVE(r1)
+#endif
 
 	/* Create our stack frame + pt_regs */
-	stdu	r1, -SWITCH_FRAME_SIZE(r1)
+	PPC_STLU	r1, -SWITCH_FRAME_SIZE(r1)
 
 	/* Save all gprs to pt_regs */
 	SAVE_GPRS(3, 10, r1)
 
+#ifdef CONFIG_PPC64
 	lbz	r3, PACA_FTRACE_ENABLED(r13)
 	cmpdi	r3, 0
 	beq	ftrace_no_trace
+#endif
 
 	/* Get the _mcount() call site out of LR */
 	mflr	r7
-	std     r7, _NIP(r1)
+	PPC_STL     r7, _NIP(r1)
 
+#ifdef CONFIG_PPC64
 	/* Save callee's TOC in the ABI compliant location */
 	std	r2, 24(r1)
 	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
@@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
 	addis	r3, r2, function_trace_op@toc@ha
 	addi	r3, r3, function_trace_op@toc@l
 	ld	r5, 0(r3)
+#else
+	lis	r3,function_trace_op@ha
+	lwz	r5,function_trace_op@l(r3)
+#endif
 
 #ifdef CONFIG_LIVEPATCH_64
 	SAVE_GPR(14, r1)
@@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
 	subi    r3, r7, MCOUNT_INSN_SIZE
 
 	/* Put the original return address in r4 as parent_ip */
-	std	r0, _LINK(r1)
+	PPC_STL	r0, _LINK(r1)
 	mr	r4, r0
 
 	/* Load &pt_regs in r6 for call below */
@@ -183,7 +210,7 @@ ftrace_call:
 	bl	ftrace_stub
 	nop
 
-	ld	r3, _NIP(r1)
+	PPC_LL	r3, _NIP(r1)
 	mtctr	r3
 #ifdef CONFIG_LIVEPATCH_64
 	cmpd	r14, r3		/* has NIP been altered? */
@@ -193,11 +220,13 @@ ftrace_call:
 	/* Restore gprs */
 	REST_GPRS(3, 10, r1)
 
+#ifdef CONFIG_PPC64
 	/* Restore callee's TOC */
 	ld	r2, 24(r1)
+#endif
 
 	/* Restore possibly modified LR */
-	ld	r0, _LINK(r1)
+	PPC_LL	r0, _LINK(r1)
 	mtlr	r0
 
 	/* Pop our stack frame */
@@ -209,7 +238,7 @@ ftrace_call:
 #endif
 	bctr			/* jump after _mcount site */
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	/*
 	 * This function runs in the mcount context, between two functions. As
 	 * such it can only clobber registers which are volatile and used in
-- 
2.33.1

[PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32

[Christophe Leroy]

PPC64 mprofile versions and PPC32 are very similar.

Modify PPC64 version so that if can be reused for PPC32.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index 6071e0122797..56da60e98327 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -34,13 +34,16 @@
  */
 _GLOBAL(ftrace_regs_caller)
 	/* Save the original return address in A's stack frame */
-	std	r0,LRSAVE(r1)
+#ifdef CONFIG_MPROFILE_KERNEL
+	PPC_STL	r0,LRSAVE(r1)
+#endif
 
 	/* Create our stack frame + pt_regs */
-	stdu	r1,-SWITCH_FRAME_SIZE(r1)
+	PPC_STLU	r1,-SWITCH_FRAME_SIZE(r1)
 
 	/* Save all gprs to pt_regs */
 	SAVE_GPR(0, r1)
+#ifdef CONFIG_PPC64
 	SAVE_GPRS(2, 11, r1)
 
 	/* Ok to continue? */
@@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
 	beq	ftrace_no_trace
 
 	SAVE_GPRS(12, 31, r1)
+#else
+	stmw	r2, GPR2(r1)
+#endif
 
 	/* Save previous stack pointer (r1) */
 	addi	r8, r1, SWITCH_FRAME_SIZE
-	std	r8, GPR1(r1)
+	PPC_STL	r8, GPR1(r1)
 
 	/* Load special regs for save below */
 	mfmsr   r8
@@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
 	/* Get the _mcount() call site out of LR */
 	mflr	r7
 	/* Save it as pt_regs->nip */
-	std     r7, _NIP(r1)
+	PPC_STL	r7, _NIP(r1)
 	/* Save the read LR in pt_regs->link */
-	std     r0, _LINK(r1)
+	PPC_STL	r0, _LINK(r1)
 
+#ifdef CONFIG_PPC64
 	/* Save callee's TOC in the ABI compliant location */
 	std	r2, 24(r1)
 	ld	r2,PACATOC(r13)	/* get kernel TOC in r2 */
@@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
 	addis	r3,r2,function_trace_op@toc@ha
 	addi	r3,r3,function_trace_op@toc@l
 	ld	r5,0(r3)
+#else
+	lis	r3,function_trace_op@ha
+	lwz	r5,function_trace_op@l(r3)
+#endif
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	mr	r14,r7		/* remember old NIP */
 #endif
 	/* Calculate ip from nip-4 into r3 for call below */
@@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
 	mr	r4, r0
 
 	/* Save special regs */
-	std     r8, _MSR(r1)
-	std     r9, _CTR(r1)
-	std     r10, _XER(r1)
-	std     r11, _CCR(r1)
+	PPC_STL	r8, _MSR(r1)
+	PPC_STL	r9, _CTR(r1)
+	PPC_STL	r10, _XER(r1)
+	PPC_STL	r11, _CCR(r1)
 
 	/* Load &pt_regs in r6 for call below */
 	addi    r6, r1 ,STACK_FRAME_OVERHEAD
@@ -100,27 +111,32 @@ ftrace_regs_call:
 	nop
 
 	/* Load ctr with the possibly modified NIP */
-	ld	r3, _NIP(r1)
+	PPC_LL	r3, _NIP(r1)
 	mtctr	r3
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	cmpd	r14, r3		/* has NIP been altered? */
 #endif
 
 	/* Restore gprs */
-	REST_GPR(0, r1)
+#ifdef CONFIG_PPC64
 	REST_GPRS(2, 31, r1)
+#else
+	lmw	r2, GPR2(r1)
+#endif
 
 	/* Restore possibly modified LR */
-	ld	r0, _LINK(r1)
+	PPC_LL	r0, _LINK(r1)
 	mtlr	r0
 
+#ifdef CONFIG_PPC64
 	/* Restore callee's TOC */
 	ld	r2, 24(r1)
+#endif
 
 	/* Pop our stack frame */
 	addi r1, r1, SWITCH_FRAME_SIZE
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
         /* Based on the cmpd above, if the NIP was altered handle livepatch */
 	bne-	livepatch_handler
 #endif
@@ -129,6 +145,7 @@ ftrace_regs_call:
 _GLOBAL(ftrace_stub)
 	blr
 
+#ifdef CONFIG_PPC64
 ftrace_no_trace:
 	mflr	r3
 	mtctr	r3
@@ -136,25 +153,31 @@ ftrace_no_trace:
 	addi	r1, r1, SWITCH_FRAME_SIZE
 	mtlr	r0
 	bctr
+#endif
 
 _GLOBAL(ftrace_caller)
 	/* Save the original return address in A's stack frame */
-	std	r0, LRSAVE(r1)
+#ifdef CONFIG_MPROFILE_KERNEL
+	PPC_STL	r0, LRSAVE(r1)
+#endif
 
 	/* Create our stack frame + pt_regs */
-	stdu	r1, -SWITCH_FRAME_SIZE(r1)
+	PPC_STLU	r1, -SWITCH_FRAME_SIZE(r1)
 
 	/* Save all gprs to pt_regs */
 	SAVE_GPRS(3, 10, r1)
 
+#ifdef CONFIG_PPC64
 	lbz	r3, PACA_FTRACE_ENABLED(r13)
 	cmpdi	r3, 0
 	beq	ftrace_no_trace
+#endif
 
 	/* Get the _mcount() call site out of LR */
 	mflr	r7
-	std     r7, _NIP(r1)
+	PPC_STL     r7, _NIP(r1)
 
+#ifdef CONFIG_PPC64
 	/* Save callee's TOC in the ABI compliant location */
 	std	r2, 24(r1)
 	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
@@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
 	addis	r3, r2, function_trace_op@toc@ha
 	addi	r3, r3, function_trace_op@toc@l
 	ld	r5, 0(r3)
+#else
+	lis	r3,function_trace_op@ha
+	lwz	r5,function_trace_op@l(r3)
+#endif
 
 #ifdef CONFIG_LIVEPATCH_64
 	SAVE_GPR(14, r1)
@@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
 	subi    r3, r7, MCOUNT_INSN_SIZE
 
 	/* Put the original return address in r4 as parent_ip */
-	std	r0, _LINK(r1)
+	PPC_STL	r0, _LINK(r1)
 	mr	r4, r0
 
 	/* Load &pt_regs in r6 for call below */
@@ -183,7 +210,7 @@ ftrace_call:
 	bl	ftrace_stub
 	nop
 
-	ld	r3, _NIP(r1)
+	PPC_LL	r3, _NIP(r1)
 	mtctr	r3
 #ifdef CONFIG_LIVEPATCH_64
 	cmpd	r14, r3		/* has NIP been altered? */
@@ -193,11 +220,13 @@ ftrace_call:
 	/* Restore gprs */
 	REST_GPRS(3, 10, r1)
 
+#ifdef CONFIG_PPC64
 	/* Restore callee's TOC */
 	ld	r2, 24(r1)
+#endif
 
 	/* Restore possibly modified LR */
-	ld	r0, _LINK(r1)
+	PPC_LL	r0, _LINK(r1)
 	mtlr	r0
 
 	/* Pop our stack frame */
@@ -209,7 +238,7 @@ ftrace_call:
 #endif
 	bctr			/* jump after _mcount site */
 
-#ifdef CONFIG_LIVEPATCH
+#ifdef CONFIG_LIVEPATCH_64
 	/*
 	 * This function runs in the mcount context, between two functions. As
 	 * such it can only clobber registers which are volatile and used in
-- 
2.33.1

Re: [PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32

[Naveen N. Rao]

Christophe Leroy wrote:
> PPC64 mprofile versions and PPC32 are very similar.
> 
> Modify PPC64 version so that if can be reused for PPC32.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
>  1 file changed, 51 insertions(+), 22 deletions(-)

While I agree that ppc32 and -mprofile-kernel ftrace code are very 
similar, I think this patch adds way too many #ifdefs. IMHO, this
makes the resultant code quite difficult to follow.


- Naveen

> 
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index 6071e0122797..56da60e98327 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -34,13 +34,16 @@
>   */
>  _GLOBAL(ftrace_regs_caller)
>  	/* Save the original return address in A's stack frame */
> -	std	r0,LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> +	PPC_STL	r0,LRSAVE(r1)
> +#endif
>  
>  	/* Create our stack frame + pt_regs */
> -	stdu	r1,-SWITCH_FRAME_SIZE(r1)
> +	PPC_STLU	r1,-SWITCH_FRAME_SIZE(r1)
>  
>  	/* Save all gprs to pt_regs */
>  	SAVE_GPR(0, r1)
> +#ifdef CONFIG_PPC64
>  	SAVE_GPRS(2, 11, r1)
>  
>  	/* Ok to continue? */
> @@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
>  	beq	ftrace_no_trace
>  
>  	SAVE_GPRS(12, 31, r1)
> +#else
> +	stmw	r2, GPR2(r1)
> +#endif
>  
>  	/* Save previous stack pointer (r1) */
>  	addi	r8, r1, SWITCH_FRAME_SIZE
> -	std	r8, GPR1(r1)
> +	PPC_STL	r8, GPR1(r1)
>  
>  	/* Load special regs for save below */
>  	mfmsr   r8
> @@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
>  	/* Get the _mcount() call site out of LR */
>  	mflr	r7
>  	/* Save it as pt_regs->nip */
> -	std     r7, _NIP(r1)
> +	PPC_STL	r7, _NIP(r1)
>  	/* Save the read LR in pt_regs->link */
> -	std     r0, _LINK(r1)
> +	PPC_STL	r0, _LINK(r1)
>  
> +#ifdef CONFIG_PPC64
>  	/* Save callee's TOC in the ABI compliant location */
>  	std	r2, 24(r1)
>  	ld	r2,PACATOC(r13)	/* get kernel TOC in r2 */
> @@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
>  	addis	r3,r2,function_trace_op@toc@ha
>  	addi	r3,r3,function_trace_op@toc@l
>  	ld	r5,0(r3)
> +#else
> +	lis	r3,function_trace_op@ha
> +	lwz	r5,function_trace_op@l(r3)
> +#endif
>  
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>  	mr	r14,r7		/* remember old NIP */
>  #endif
>  	/* Calculate ip from nip-4 into r3 for call below */
> @@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
>  	mr	r4, r0
>  
>  	/* Save special regs */
> -	std     r8, _MSR(r1)
> -	std     r9, _CTR(r1)
> -	std     r10, _XER(r1)
> -	std     r11, _CCR(r1)
> +	PPC_STL	r8, _MSR(r1)
> +	PPC_STL	r9, _CTR(r1)
> +	PPC_STL	r10, _XER(r1)
> +	PPC_STL	r11, _CCR(r1)
>  
>  	/* Load &pt_regs in r6 for call below */
>  	addi    r6, r1 ,STACK_FRAME_OVERHEAD
> @@ -100,27 +111,32 @@ ftrace_regs_call:
>  	nop
>  
>  	/* Load ctr with the possibly modified NIP */
> -	ld	r3, _NIP(r1)
> +	PPC_LL	r3, _NIP(r1)
>  	mtctr	r3
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>  	cmpd	r14, r3		/* has NIP been altered? */
>  #endif
>  
>  	/* Restore gprs */
> -	REST_GPR(0, r1)
> +#ifdef CONFIG_PPC64
>  	REST_GPRS(2, 31, r1)
> +#else
> +	lmw	r2, GPR2(r1)
> +#endif
>  
>  	/* Restore possibly modified LR */
> -	ld	r0, _LINK(r1)
> +	PPC_LL	r0, _LINK(r1)
>  	mtlr	r0
>  
> +#ifdef CONFIG_PPC64
>  	/* Restore callee's TOC */
>  	ld	r2, 24(r1)
> +#endif
>  
>  	/* Pop our stack frame */
>  	addi r1, r1, SWITCH_FRAME_SIZE
>  
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>          /* Based on the cmpd above, if the NIP was altered handle livepatch */
>  	bne-	livepatch_handler
>  #endif
> @@ -129,6 +145,7 @@ ftrace_regs_call:
>  _GLOBAL(ftrace_stub)
>  	blr
>  
> +#ifdef CONFIG_PPC64
>  ftrace_no_trace:
>  	mflr	r3
>  	mtctr	r3
> @@ -136,25 +153,31 @@ ftrace_no_trace:
>  	addi	r1, r1, SWITCH_FRAME_SIZE
>  	mtlr	r0
>  	bctr
> +#endif
>  
>  _GLOBAL(ftrace_caller)
>  	/* Save the original return address in A's stack frame */
> -	std	r0, LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> +	PPC_STL	r0, LRSAVE(r1)
> +#endif
>  
>  	/* Create our stack frame + pt_regs */
> -	stdu	r1, -SWITCH_FRAME_SIZE(r1)
> +	PPC_STLU	r1, -SWITCH_FRAME_SIZE(r1)
>  
>  	/* Save all gprs to pt_regs */
>  	SAVE_GPRS(3, 10, r1)
>  
> +#ifdef CONFIG_PPC64
>  	lbz	r3, PACA_FTRACE_ENABLED(r13)
>  	cmpdi	r3, 0
>  	beq	ftrace_no_trace
> +#endif
>  
>  	/* Get the _mcount() call site out of LR */
>  	mflr	r7
> -	std     r7, _NIP(r1)
> +	PPC_STL     r7, _NIP(r1)
>  
> +#ifdef CONFIG_PPC64
>  	/* Save callee's TOC in the ABI compliant location */
>  	std	r2, 24(r1)
>  	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
> @@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
>  	addis	r3, r2, function_trace_op@toc@ha
>  	addi	r3, r3, function_trace_op@toc@l
>  	ld	r5, 0(r3)
> +#else
> +	lis	r3,function_trace_op@ha
> +	lwz	r5,function_trace_op@l(r3)
> +#endif
>  
>  #ifdef CONFIG_LIVEPATCH_64
>  	SAVE_GPR(14, r1)
> @@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
>  	subi    r3, r7, MCOUNT_INSN_SIZE
>  
>  	/* Put the original return address in r4 as parent_ip */
> -	std	r0, _LINK(r1)
> +	PPC_STL	r0, _LINK(r1)
>  	mr	r4, r0
>  
>  	/* Load &pt_regs in r6 for call below */
> @@ -183,7 +210,7 @@ ftrace_call:
>  	bl	ftrace_stub
>  	nop
>  
> -	ld	r3, _NIP(r1)
> +	PPC_LL	r3, _NIP(r1)
>  	mtctr	r3
>  #ifdef CONFIG_LIVEPATCH_64
>  	cmpd	r14, r3		/* has NIP been altered? */
> @@ -193,11 +220,13 @@ ftrace_call:
>  	/* Restore gprs */
>  	REST_GPRS(3, 10, r1)
>  
> +#ifdef CONFIG_PPC64
>  	/* Restore callee's TOC */
>  	ld	r2, 24(r1)
> +#endif
>  
>  	/* Restore possibly modified LR */
> -	ld	r0, _LINK(r1)
> +	PPC_LL	r0, _LINK(r1)
>  	mtlr	r0
>  
>  	/* Pop our stack frame */
> @@ -209,7 +238,7 @@ ftrace_call:
>  #endif
>  	bctr			/* jump after _mcount site */
>  
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>  	/*
>  	 * This function runs in the mcount context, between two functions. As
>  	 * such it can only clobber registers which are volatile and used in
> -- 
> 2.33.1
> 

Re: [PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32

[Naveen N. Rao]

Christophe Leroy wrote:
> PPC64 mprofile versions and PPC32 are very similar.
> 
> Modify PPC64 version so that if can be reused for PPC32.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
>  1 file changed, 51 insertions(+), 22 deletions(-)

While I agree that ppc32 and -mprofile-kernel ftrace code are very 
similar, I think this patch adds way too many #ifdefs. IMHO, this
makes the resultant code quite difficult to follow.


- Naveen

> 
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index 6071e0122797..56da60e98327 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -34,13 +34,16 @@
>   */
>  _GLOBAL(ftrace_regs_caller)
>  	/* Save the original return address in A's stack frame */
> -	std	r0,LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> +	PPC_STL	r0,LRSAVE(r1)
> +#endif
>  
>  	/* Create our stack frame + pt_regs */
> -	stdu	r1,-SWITCH_FRAME_SIZE(r1)
> +	PPC_STLU	r1,-SWITCH_FRAME_SIZE(r1)
>  
>  	/* Save all gprs to pt_regs */
>  	SAVE_GPR(0, r1)
> +#ifdef CONFIG_PPC64
>  	SAVE_GPRS(2, 11, r1)
>  
>  	/* Ok to continue? */
> @@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
>  	beq	ftrace_no_trace
>  
>  	SAVE_GPRS(12, 31, r1)
> +#else
> +	stmw	r2, GPR2(r1)
> +#endif
>  
>  	/* Save previous stack pointer (r1) */
>  	addi	r8, r1, SWITCH_FRAME_SIZE
> -	std	r8, GPR1(r1)
> +	PPC_STL	r8, GPR1(r1)
>  
>  	/* Load special regs for save below */
>  	mfmsr   r8
> @@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
>  	/* Get the _mcount() call site out of LR */
>  	mflr	r7
>  	/* Save it as pt_regs->nip */
> -	std     r7, _NIP(r1)
> +	PPC_STL	r7, _NIP(r1)
>  	/* Save the read LR in pt_regs->link */
> -	std     r0, _LINK(r1)
> +	PPC_STL	r0, _LINK(r1)
>  
> +#ifdef CONFIG_PPC64
>  	/* Save callee's TOC in the ABI compliant location */
>  	std	r2, 24(r1)
>  	ld	r2,PACATOC(r13)	/* get kernel TOC in r2 */
> @@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
>  	addis	r3,r2,function_trace_op@toc@ha
>  	addi	r3,r3,function_trace_op@toc@l
>  	ld	r5,0(r3)
> +#else
> +	lis	r3,function_trace_op@ha
> +	lwz	r5,function_trace_op@l(r3)
> +#endif
>  
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>  	mr	r14,r7		/* remember old NIP */
>  #endif
>  	/* Calculate ip from nip-4 into r3 for call below */
> @@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
>  	mr	r4, r0
>  
>  	/* Save special regs */
> -	std     r8, _MSR(r1)
> -	std     r9, _CTR(r1)
> -	std     r10, _XER(r1)
> -	std     r11, _CCR(r1)
> +	PPC_STL	r8, _MSR(r1)
> +	PPC_STL	r9, _CTR(r1)
> +	PPC_STL	r10, _XER(r1)
> +	PPC_STL	r11, _CCR(r1)
>  
>  	/* Load &pt_regs in r6 for call below */
>  	addi    r6, r1 ,STACK_FRAME_OVERHEAD
> @@ -100,27 +111,32 @@ ftrace_regs_call:
>  	nop
>  
>  	/* Load ctr with the possibly modified NIP */
> -	ld	r3, _NIP(r1)
> +	PPC_LL	r3, _NIP(r1)
>  	mtctr	r3
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>  	cmpd	r14, r3		/* has NIP been altered? */
>  #endif
>  
>  	/* Restore gprs */
> -	REST_GPR(0, r1)
> +#ifdef CONFIG_PPC64
>  	REST_GPRS(2, 31, r1)
> +#else
> +	lmw	r2, GPR2(r1)
> +#endif
>  
>  	/* Restore possibly modified LR */
> -	ld	r0, _LINK(r1)
> +	PPC_LL	r0, _LINK(r1)
>  	mtlr	r0
>  
> +#ifdef CONFIG_PPC64
>  	/* Restore callee's TOC */
>  	ld	r2, 24(r1)
> +#endif
>  
>  	/* Pop our stack frame */
>  	addi r1, r1, SWITCH_FRAME_SIZE
>  
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>          /* Based on the cmpd above, if the NIP was altered handle livepatch */
>  	bne-	livepatch_handler
>  #endif
> @@ -129,6 +145,7 @@ ftrace_regs_call:
>  _GLOBAL(ftrace_stub)
>  	blr
>  
> +#ifdef CONFIG_PPC64
>  ftrace_no_trace:
>  	mflr	r3
>  	mtctr	r3
> @@ -136,25 +153,31 @@ ftrace_no_trace:
>  	addi	r1, r1, SWITCH_FRAME_SIZE
>  	mtlr	r0
>  	bctr
> +#endif
>  
>  _GLOBAL(ftrace_caller)
>  	/* Save the original return address in A's stack frame */
> -	std	r0, LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> +	PPC_STL	r0, LRSAVE(r1)
> +#endif
>  
>  	/* Create our stack frame + pt_regs */
> -	stdu	r1, -SWITCH_FRAME_SIZE(r1)
> +	PPC_STLU	r1, -SWITCH_FRAME_SIZE(r1)
>  
>  	/* Save all gprs to pt_regs */
>  	SAVE_GPRS(3, 10, r1)
>  
> +#ifdef CONFIG_PPC64
>  	lbz	r3, PACA_FTRACE_ENABLED(r13)
>  	cmpdi	r3, 0
>  	beq	ftrace_no_trace
> +#endif
>  
>  	/* Get the _mcount() call site out of LR */
>  	mflr	r7
> -	std     r7, _NIP(r1)
> +	PPC_STL     r7, _NIP(r1)
>  
> +#ifdef CONFIG_PPC64
>  	/* Save callee's TOC in the ABI compliant location */
>  	std	r2, 24(r1)
>  	ld	r2, PACATOC(r13)	/* get kernel TOC in r2 */
> @@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
>  	addis	r3, r2, function_trace_op@toc@ha
>  	addi	r3, r3, function_trace_op@toc@l
>  	ld	r5, 0(r3)
> +#else
> +	lis	r3,function_trace_op@ha
> +	lwz	r5,function_trace_op@l(r3)
> +#endif
>  
>  #ifdef CONFIG_LIVEPATCH_64
>  	SAVE_GPR(14, r1)
> @@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
>  	subi    r3, r7, MCOUNT_INSN_SIZE
>  
>  	/* Put the original return address in r4 as parent_ip */
> -	std	r0, _LINK(r1)
> +	PPC_STL	r0, _LINK(r1)
>  	mr	r4, r0
>  
>  	/* Load &pt_regs in r6 for call below */
> @@ -183,7 +210,7 @@ ftrace_call:
>  	bl	ftrace_stub
>  	nop
>  
> -	ld	r3, _NIP(r1)
> +	PPC_LL	r3, _NIP(r1)
>  	mtctr	r3
>  #ifdef CONFIG_LIVEPATCH_64
>  	cmpd	r14, r3		/* has NIP been altered? */
> @@ -193,11 +220,13 @@ ftrace_call:
>  	/* Restore gprs */
>  	REST_GPRS(3, 10, r1)
>  
> +#ifdef CONFIG_PPC64
>  	/* Restore callee's TOC */
>  	ld	r2, 24(r1)
> +#endif
>  
>  	/* Restore possibly modified LR */
> -	ld	r0, _LINK(r1)
> +	PPC_LL	r0, _LINK(r1)
>  	mtlr	r0
>  
>  	/* Pop our stack frame */
> @@ -209,7 +238,7 @@ ftrace_call:
>  #endif
>  	bctr			/* jump after _mcount site */
>  
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
>  	/*
>  	 * This function runs in the mcount context, between two functions. As
>  	 * such it can only clobber registers which are volatile and used in
> -- 
> 2.33.1
> 

Re: [PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32

[Christophe Leroy]

Le 14/02/2022 à 18:51, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> PPC64 mprofile versions and PPC32 are very similar.
>>
>> Modify PPC64 version so that if can be reused for PPC32.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
>>  1 file changed, 51 insertions(+), 22 deletions(-)
> 
> While I agree that ppc32 and -mprofile-kernel ftrace code are very 
> similar, I think this patch adds way too many #ifdefs. IMHO, this
> makes the resultant code quite difficult to follow.

Ok, I can introduce some GAS macros for a few of them in a followup patch.

Christophe

Re: [PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32

[Christophe Leroy]

Le 14/02/2022 à 18:51, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> PPC64 mprofile versions and PPC32 are very similar.
>>
>> Modify PPC64 version so that if can be reused for PPC32.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
>>  1 file changed, 51 insertions(+), 22 deletions(-)
> 
> While I agree that ppc32 and -mprofile-kernel ftrace code are very 
> similar, I think this patch adds way too many #ifdefs. IMHO, this
> makes the resultant code quite difficult to follow.

Ok, I can introduce some GAS macros for a few of them in a followup patch.

Christophe

[PATCH v2 13/13] powerpc/ftrace: Remove ftrace_32.S

[Christophe Leroy]

Functions in ftrace_32.S are common with PPC64.

Reuse the ones defined for PPC64 with slight modification
when required.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/Makefile            |   7 +-
 arch/powerpc/kernel/trace/ftrace_32.S         | 152 ------------------
 .../trace/{ftrace_64.S => ftrace_low.S}       |  14 ++
 ...ftrace_64_mprofile.S => ftrace_mprofile.S} |   0
 4 files changed, 17 insertions(+), 156 deletions(-)
 delete mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
 rename arch/powerpc/kernel/trace/{ftrace_64.S => ftrace_low.S} (85%)
 rename arch/powerpc/kernel/trace/{ftrace_64_mprofile.S => ftrace_mprofile.S} (100%)

diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index 858503775c58..ac7d42a4e8d0 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -8,15 +8,14 @@ ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
 endif
 
-obj32-$(CONFIG_FUNCTION_TRACER)		+= ftrace_32.o
-obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64.o
+obj32-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o
 ifdef CONFIG_MPROFILE_KERNEL
-obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_mprofile.o
+obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o
 else
 obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_pg.o
 endif
 obj-$(CONFIG_DYNAMIC_FTRACE)		+= ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o ftrace_low.o
 obj-$(CONFIG_FTRACE_SYSCALLS)		+= ftrace.o
 obj-$(CONFIG_TRACING)			+= trace_clock.o
 
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
deleted file mode 100644
index 2b425da97a6b..000000000000
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ /dev/null
@@ -1,152 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Split from entry_32.S
- */
-
-#include <linux/magic.h>
-#include <asm/reg.h>
-#include <asm/ppc_asm.h>
-#include <asm/asm-offsets.h>
-#include <asm/ftrace.h>
-#include <asm/export.h>
-#include <asm/ptrace.h>
-
-_GLOBAL(mcount)
-_GLOBAL(_mcount)
-	/*
-	 * It is required that _mcount on PPC32 must preserve the
-	 * link register. But we have r12 to play with. We use r12
-	 * to push the return address back to the caller of mcount
-	 * into the ctr register, restore the link register and
-	 * then jump back using the ctr register.
-	 */
-	mflr	r12
-	mtctr	r12
-	mtlr	r0
-	bctr
-EXPORT_SYMBOL(_mcount)
-
-_GLOBAL(ftrace_caller)
-	stwu	r1, -INT_FRAME_SIZE(r1)
-
-	SAVE_GPRS(3, 10, r1)
-
-	addi	r8, r1, INT_FRAME_SIZE
-	stw	r8, GPR1(r1)
-
-	mflr	r3
-	stw	r3, _NIP(r1)
-	subi	r3, r3, MCOUNT_INSN_SIZE
-
-	stw	r0, _LINK(r1)
-	mr	r4, r0
-
-	lis	r5,function_trace_op@ha
-	lwz	r5,function_trace_op@l(r5)
-
-	addi	r6, r1, STACK_FRAME_OVERHEAD
-.globl ftrace_call
-ftrace_call:
-	bl	ftrace_stub
-	nop
-
-	lwz	r3, _NIP(r1)
-	mtctr	r3
-
-	REST_GPRS(3, 10, r1)
-
-	lwz	r0, _LINK(r1)
-	mtlr	r0
-
-	addi	r1, r1, INT_FRAME_SIZE
-	/* old link register ends up in ctr reg */
-	bctr
-
-
-_GLOBAL(ftrace_stub)
-	blr
-
-_GLOBAL(ftrace_regs_caller)
-	/* Create our stack frame + pt_regs */
-	stwu	r1,-INT_FRAME_SIZE(r1)
-
-	/* Save all gprs to pt_regs */
-	stw	r0, GPR0(r1)
-	stmw	r2, GPR2(r1)
-
-	/* Save previous stack pointer (r1) */
-	addi	r8, r1, INT_FRAME_SIZE
-	stw	r8, GPR1(r1)
-
-	/* Load special regs for save below */
-	mfmsr   r8
-	mfctr   r9
-	mfxer   r10
-	mfcr	r11
-
-	/* Get the _mcount() call site out of LR */
-	mflr	r7
-	/* Save it as pt_regs->nip */
-	stw     r7, _NIP(r1)
-	/* Save the read LR in pt_regs->link */
-	stw     r0, _LINK(r1)
-
-	lis	r3,function_trace_op@ha
-	lwz	r5,function_trace_op@l(r3)
-
-	/* Calculate ip from nip-4 into r3 for call below */
-	subi    r3, r7, MCOUNT_INSN_SIZE
-
-	/* Put the original return address in r4 as parent_ip */
-	mr	r4, r0
-
-	/* Save special regs */
-	stw     r8, _MSR(r1)
-	stw     r9, _CTR(r1)
-	stw     r10, _XER(r1)
-	stw     r11, _CCR(r1)
-
-	/* Load &pt_regs in r6 for call below */
-	addi    r6, r1, STACK_FRAME_OVERHEAD
-
-	/* ftrace_call(r3, r4, r5, r6) */
-.globl ftrace_regs_call
-ftrace_regs_call:
-	bl	ftrace_stub
-	nop
-
-	/* Load ctr with the possibly modified NIP */
-	lwz	r3, _NIP(r1)
-	mtctr	r3
-
-	/* Restore gprs */
-	lmw	r2, GPR2(r1)
-
-	/* Restore possibly modified LR */
-	lwz	r0, _LINK(r1)
-	mtlr	r0
-
-	/* Pop our stack frame */
-	addi r1, r1, INT_FRAME_SIZE
-	/* old link register ends up in ctr reg */
-	bctr
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(return_to_handler)
-	/* need to save return values */
-	stwu	r1, -16(r1)
-	stw	r3, 8(r1)
-	stw	r4, 12(r1)
-
-	bl	ftrace_return_to_handler
-
-	/* return value has real return address */
-	mtlr	r3
-
-	lwz	r3, 8(r1)
-	lwz	r4, 12(r1)
-	addi	r1, r1, 16
-
-	/* Jump back to real return address */
-	blr
-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/powerpc/kernel/trace/ftrace_64.S b/arch/powerpc/kernel/trace/ftrace_low.S
similarity index 85%
rename from arch/powerpc/kernel/trace/ftrace_64.S
rename to arch/powerpc/kernel/trace/ftrace_low.S
index 25e5b9e47c06..0bddf1fa6636 100644
--- a/arch/powerpc/kernel/trace/ftrace_64.S
+++ b/arch/powerpc/kernel/trace/ftrace_low.S
@@ -10,6 +10,7 @@
 #include <asm/ppc-opcode.h>
 #include <asm/export.h>
 
+#ifdef CONFIG_PPC64
 .pushsection ".tramp.ftrace.text","aw",@progbits;
 .globl ftrace_tramp_text
 ftrace_tramp_text:
@@ -21,6 +22,7 @@ ftrace_tramp_text:
 ftrace_tramp_init:
 	.space 64
 .popsection
+#endif
 
 _GLOBAL(mcount)
 _GLOBAL(_mcount)
@@ -33,6 +35,7 @@ EXPORT_SYMBOL(_mcount)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 _GLOBAL(return_to_handler)
 	/* need to save return values */
+#ifdef CONFIG_PPC64
 	std	r4,  -32(r1)
 	std	r3,  -24(r1)
 	/* save TOC */
@@ -46,6 +49,11 @@ _GLOBAL(return_to_handler)
 	 * Switch to our TOC to run inside the core kernel.
 	 */
 	ld	r2, PACATOC(r13)
+#else
+	stwu	r1, -16(r1)
+	stw	r3, 8(r1)
+	stw	r4, 12(r1)
+#endif
 
 	bl	ftrace_return_to_handler
 	nop
@@ -53,11 +61,17 @@ _GLOBAL(return_to_handler)
 	/* return value has real return address */
 	mtlr	r3
 
+#ifdef CONFIG_PPC64
 	ld	r1, 0(r1)
 	ld	r4,  -32(r1)
 	ld	r3,  -24(r1)
 	ld	r2,  -16(r1)
 	ld	r31, -8(r1)
+#else
+	lwz	r3, 8(r1)
+	lwz	r4, 12(r1)
+	addi	r1, r1, 16
+#endif
 
 	/* Jump back to real return address */
 	blr
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_mprofile.S
similarity index 100%
rename from arch/powerpc/kernel/trace/ftrace_64_mprofile.S
rename to arch/powerpc/kernel/trace/ftrace_mprofile.S
-- 
2.33.1

[PATCH v2 13/13] powerpc/ftrace: Remove ftrace_32.S

[Christophe Leroy]

Functions in ftrace_32.S are common with PPC64.

Reuse the ones defined for PPC64 with slight modification
when required.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/Makefile            |   7 +-
 arch/powerpc/kernel/trace/ftrace_32.S         | 152 ------------------
 .../trace/{ftrace_64.S => ftrace_low.S}       |  14 ++
 ...ftrace_64_mprofile.S => ftrace_mprofile.S} |   0
 4 files changed, 17 insertions(+), 156 deletions(-)
 delete mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
 rename arch/powerpc/kernel/trace/{ftrace_64.S => ftrace_low.S} (85%)
 rename arch/powerpc/kernel/trace/{ftrace_64_mprofile.S => ftrace_mprofile.S} (100%)

diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index 858503775c58..ac7d42a4e8d0 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -8,15 +8,14 @@ ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
 endif
 
-obj32-$(CONFIG_FUNCTION_TRACER)		+= ftrace_32.o
-obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64.o
+obj32-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o
 ifdef CONFIG_MPROFILE_KERNEL
-obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_mprofile.o
+obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o
 else
 obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_pg.o
 endif
 obj-$(CONFIG_DYNAMIC_FTRACE)		+= ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o ftrace_low.o
 obj-$(CONFIG_FTRACE_SYSCALLS)		+= ftrace.o
 obj-$(CONFIG_TRACING)			+= trace_clock.o
 
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
deleted file mode 100644
index 2b425da97a6b..000000000000
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ /dev/null
@@ -1,152 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Split from entry_32.S
- */
-
-#include <linux/magic.h>
-#include <asm/reg.h>
-#include <asm/ppc_asm.h>
-#include <asm/asm-offsets.h>
-#include <asm/ftrace.h>
-#include <asm/export.h>
-#include <asm/ptrace.h>
-
-_GLOBAL(mcount)
-_GLOBAL(_mcount)
-	/*
-	 * It is required that _mcount on PPC32 must preserve the
-	 * link register. But we have r12 to play with. We use r12
-	 * to push the return address back to the caller of mcount
-	 * into the ctr register, restore the link register and
-	 * then jump back using the ctr register.
-	 */
-	mflr	r12
-	mtctr	r12
-	mtlr	r0
-	bctr
-EXPORT_SYMBOL(_mcount)
-
-_GLOBAL(ftrace_caller)
-	stwu	r1, -INT_FRAME_SIZE(r1)
-
-	SAVE_GPRS(3, 10, r1)
-
-	addi	r8, r1, INT_FRAME_SIZE
-	stw	r8, GPR1(r1)
-
-	mflr	r3
-	stw	r3, _NIP(r1)
-	subi	r3, r3, MCOUNT_INSN_SIZE
-
-	stw	r0, _LINK(r1)
-	mr	r4, r0
-
-	lis	r5,function_trace_op@ha
-	lwz	r5,function_trace_op@l(r5)
-
-	addi	r6, r1, STACK_FRAME_OVERHEAD
-.globl ftrace_call
-ftrace_call:
-	bl	ftrace_stub
-	nop
-
-	lwz	r3, _NIP(r1)
-	mtctr	r3
-
-	REST_GPRS(3, 10, r1)
-
-	lwz	r0, _LINK(r1)
-	mtlr	r0
-
-	addi	r1, r1, INT_FRAME_SIZE
-	/* old link register ends up in ctr reg */
-	bctr
-
-
-_GLOBAL(ftrace_stub)
-	blr
-
-_GLOBAL(ftrace_regs_caller)
-	/* Create our stack frame + pt_regs */
-	stwu	r1,-INT_FRAME_SIZE(r1)
-
-	/* Save all gprs to pt_regs */
-	stw	r0, GPR0(r1)
-	stmw	r2, GPR2(r1)
-
-	/* Save previous stack pointer (r1) */
-	addi	r8, r1, INT_FRAME_SIZE
-	stw	r8, GPR1(r1)
-
-	/* Load special regs for save below */
-	mfmsr   r8
-	mfctr   r9
-	mfxer   r10
-	mfcr	r11
-
-	/* Get the _mcount() call site out of LR */
-	mflr	r7
-	/* Save it as pt_regs->nip */
-	stw     r7, _NIP(r1)
-	/* Save the read LR in pt_regs->link */
-	stw     r0, _LINK(r1)
-
-	lis	r3,function_trace_op@ha
-	lwz	r5,function_trace_op@l(r3)
-
-	/* Calculate ip from nip-4 into r3 for call below */
-	subi    r3, r7, MCOUNT_INSN_SIZE
-
-	/* Put the original return address in r4 as parent_ip */
-	mr	r4, r0
-
-	/* Save special regs */
-	stw     r8, _MSR(r1)
-	stw     r9, _CTR(r1)
-	stw     r10, _XER(r1)
-	stw     r11, _CCR(r1)
-
-	/* Load &pt_regs in r6 for call below */
-	addi    r6, r1, STACK_FRAME_OVERHEAD
-
-	/* ftrace_call(r3, r4, r5, r6) */
-.globl ftrace_regs_call
-ftrace_regs_call:
-	bl	ftrace_stub
-	nop
-
-	/* Load ctr with the possibly modified NIP */
-	lwz	r3, _NIP(r1)
-	mtctr	r3
-
-	/* Restore gprs */
-	lmw	r2, GPR2(r1)
-
-	/* Restore possibly modified LR */
-	lwz	r0, _LINK(r1)
-	mtlr	r0
-
-	/* Pop our stack frame */
-	addi r1, r1, INT_FRAME_SIZE
-	/* old link register ends up in ctr reg */
-	bctr
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(return_to_handler)
-	/* need to save return values */
-	stwu	r1, -16(r1)
-	stw	r3, 8(r1)
-	stw	r4, 12(r1)
-
-	bl	ftrace_return_to_handler
-
-	/* return value has real return address */
-	mtlr	r3
-
-	lwz	r3, 8(r1)
-	lwz	r4, 12(r1)
-	addi	r1, r1, 16
-
-	/* Jump back to real return address */
-	blr
-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/powerpc/kernel/trace/ftrace_64.S b/arch/powerpc/kernel/trace/ftrace_low.S
similarity index 85%
rename from arch/powerpc/kernel/trace/ftrace_64.S
rename to arch/powerpc/kernel/trace/ftrace_low.S
index 25e5b9e47c06..0bddf1fa6636 100644
--- a/arch/powerpc/kernel/trace/ftrace_64.S
+++ b/arch/powerpc/kernel/trace/ftrace_low.S
@@ -10,6 +10,7 @@
 #include <asm/ppc-opcode.h>
 #include <asm/export.h>
 
+#ifdef CONFIG_PPC64
 .pushsection ".tramp.ftrace.text","aw",@progbits;
 .globl ftrace_tramp_text
 ftrace_tramp_text:
@@ -21,6 +22,7 @@ ftrace_tramp_text:
 ftrace_tramp_init:
 	.space 64
 .popsection
+#endif
 
 _GLOBAL(mcount)
 _GLOBAL(_mcount)
@@ -33,6 +35,7 @@ EXPORT_SYMBOL(_mcount)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 _GLOBAL(return_to_handler)
 	/* need to save return values */
+#ifdef CONFIG_PPC64
 	std	r4,  -32(r1)
 	std	r3,  -24(r1)
 	/* save TOC */
@@ -46,6 +49,11 @@ _GLOBAL(return_to_handler)
 	 * Switch to our TOC to run inside the core kernel.
 	 */
 	ld	r2, PACATOC(r13)
+#else
+	stwu	r1, -16(r1)
+	stw	r3, 8(r1)
+	stw	r4, 12(r1)
+#endif
 
 	bl	ftrace_return_to_handler
 	nop
@@ -53,11 +61,17 @@ _GLOBAL(return_to_handler)
 	/* return value has real return address */
 	mtlr	r3
 
+#ifdef CONFIG_PPC64
 	ld	r1, 0(r1)
 	ld	r4,  -32(r1)
 	ld	r3,  -24(r1)
 	ld	r2,  -16(r1)
 	ld	r31, -8(r1)
+#else
+	lwz	r3, 8(r1)
+	lwz	r4, 12(r1)
+	addi	r1, r1, 16
+#endif
 
 	/* Jump back to real return address */
 	blr
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_mprofile.S
similarity index 100%
rename from arch/powerpc/kernel/trace/ftrace_64_mprofile.S
rename to arch/powerpc/kernel/trace/ftrace_mprofile.S
-- 
2.33.1

[PATCH] Fixup for next-test 3a1a8f078670 ("powerpc/ftrace: Remove ftrace_32.S")

[Christophe Leroy]

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index ac7d42a4e8d0..542aa7a8b2b4 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -14,8 +14,9 @@ obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o
 else
 obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_pg.o
 endif
+obj-$(CONFIG_FUNCTION_TRACER)		+= ftrace_low.o
 obj-$(CONFIG_DYNAMIC_FTRACE)		+= ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o ftrace_low.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)		+= ftrace.o
 obj-$(CONFIG_TRACING)			+= trace_clock.o
 
-- 
2.34.1

[PATCH] Fixup for next-test 3a1a8f078670 ("powerpc/ftrace: Remove ftrace_32.S")

[Christophe Leroy]

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index ac7d42a4e8d0..542aa7a8b2b4 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -14,8 +14,9 @@ obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o
 else
 obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_pg.o
 endif
+obj-$(CONFIG_FUNCTION_TRACER)		+= ftrace_low.o
 obj-$(CONFIG_DYNAMIC_FTRACE)		+= ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o ftrace_low.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)		+= ftrace.o
 obj-$(CONFIG_TRACING)			+= trace_clock.o
 
-- 
2.34.1

Re: [PATCH v2 00/13] Implement livepatch on PPC32 and more

[Michael Ellerman]

On Mon, 20 Dec 2021 16:37:58 +0000, Christophe Leroy wrote:
> This series implements livepatch on PPC32 and implements
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS to simplify ftracing.
> 
> v2:
> - Fix problem with strict modules RWX
> - Convert powerpc to CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> - Convert function graph tracing to C
> - Refactor PPC32 versus PPC64
> 
> [...]

Patches 1 and 3-13 applied to powerpc/next.

[01/13] livepatch: Fix build failure on 32 bits processors
        https://git.kernel.org/powerpc/c/2f293651eca3eacaeb56747dede31edace7329d2
[03/13] powerpc/module_32: Fix livepatching for RO modules
        https://git.kernel.org/powerpc/c/0c850965d6909d39fd69d6a3602bb62b48cad417
[04/13] powerpc/ftrace: Add support for livepatch to PPC32
        https://git.kernel.org/powerpc/c/a4520b25276500f1abcfc55d24f1251b7b08eff6
[05/13] powerpc/ftrace: Don't save again LR in ftrace_regs_caller() on PPC32
        https://git.kernel.org/powerpc/c/7875bc9b07cde868784195e215f4deaa0fa928a2
[06/13] powerpc/ftrace: Simplify PPC32's return_to_handler()
        https://git.kernel.org/powerpc/c/7bdb478c1d15cfd3a92db6331cb2d3dd3a8b9436
[07/13] powerpc/ftrace: Prepare PPC32's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS
        https://git.kernel.org/powerpc/c/d95bf254be5f74c1e4c8f7cb64e2e21b9cc91717
[08/13] powerpc/ftrace: Prepare PPC64's ftrace_caller() for CONFIG_DYNAMIC_FTRACE_WITH_ARGS
        https://git.kernel.org/powerpc/c/c75388a8ceffbf1bf72c61afe66a72e58aa20c74
[09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
        https://git.kernel.org/powerpc/c/40b035efe288f42bbf4483236cde652584ccb64e
[10/13] powerpc/ftrace: Refactor ftrace_{en/dis}able_ftrace_graph_caller
        https://git.kernel.org/powerpc/c/0c81ed5ed43863d313cf253b0ebada6ea2f17676
[11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller
        https://git.kernel.org/powerpc/c/830213786c498b0c488fedd2abc15a7ce442b42f
[12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
        https://git.kernel.org/powerpc/c/41315494beed087011f256b4f1439bb3d8236904
[13/13] powerpc/ftrace: Remove ftrace_32.S
        https://git.kernel.org/powerpc/c/4ee83a2cfbc46c13f2a08fe6d48dbcede53cdbf8

cheers