Subject: Re: [PATCH 07/17] powerpc/ftrace: Consolidate ftrace support into fewer files

From: Naveen N Rao <[email-masked]>
Date: Wed, 28 Jun 2023 13:02:23 +0530

Hi Christophe,

Christophe Leroy wrote:
> 
> 
> Le 19/06/2023 à 11:47, Naveen N Rao a écrit :
>> ftrace_low.S has just the _mcount stub and return_to_handler(). Merge
>> this back into ftrace_mprofile.S and ftrace_64_pg.S to keep all ftrace
>> code together, and to allow those to evolve independently.
>> 
>> ftrace_mprofile.S is also not an entirely accurate name since this also
>> holds ppc32 code. This will be all the more incorrect once support for
>> -fpatchable-function-entry is added. Rename files here to more
>> accurately describe the code:
>> - ftrace_mprofile.S is renamed to ftrace_entry.S
>> - ftrace_pg.c is renamed to ftrace_64_pg.c
>> - ftrace_64_pg.S is rename to ftrace_64_pg_entry.S
> 
> The stats from git do not match., it says {ftrace_low.S => 
> ftrace_64_pg_entry.S

I suppose that's git diff view. You can see from the patch below that 
the header of ftrace_64_pg_entry.S carries the line 'Split from 
ftrace_64.S'. In reality, this is still ftrace_low.S being merged into 
ftrace_64_pg.S.

> 
>> 
>> Signed-off-by: Naveen N Rao <[email-masked]>
> 
> Reviewed-by: Christophe Leroy <[email-masked]>

Thanks for the review.

- Naveen

> 
>> ---
>>   arch/powerpc/kernel/trace/Makefile            | 17 +++--
>>   arch/powerpc/kernel/trace/ftrace_64_pg.S      | 67 -------------------
>>   .../trace/{ftrace_pg.c => ftrace_64_pg.c}     |  0
>>   .../{ftrace_low.S => ftrace_64_pg_entry.S}    | 58 +++++++++++++++-
>>   .../{ftrace_mprofile.S => ftrace_entry.S}     | 65 ++++++++++++++++++
>>   5 files changed, 130 insertions(+), 77 deletions(-)
>>   delete mode 100644 arch/powerpc/kernel/trace/ftrace_64_pg.S
>>   rename arch/powerpc/kernel/trace/{ftrace_pg.c => ftrace_64_pg.c} (100%)
>>   rename arch/powerpc/kernel/trace/{ftrace_low.S => ftrace_64_pg_entry.S} (55%)
>>   rename arch/powerpc/kernel/trace/{ftrace_mprofile.S => ftrace_entry.S} (83%)
>> 
>> diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
>> index 342a2d1ae86cd0..125f4ca588b98a 100644
>> --- a/arch/powerpc/kernel/trace/Makefile
>> +++ b/arch/powerpc/kernel/trace/Makefile
>> @@ -6,16 +6,15 @@
>>   ifdef CONFIG_FUNCTION_TRACER
>>   # do not trace tracer code
>>   CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
>> -CFLAGS_REMOVE_ftrace_pg.o = $(CC_FLAGS_FTRACE)
>> +CFLAGS_REMOVE_ftrace_64_pg.o = $(CC_FLAGS_FTRACE)
>>   endif
>>   
>> -obj32-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o ftrace.o
>> +obj32-$(CONFIG_FUNCTION_TRACER)		+= ftrace.o ftrace_entry.o
>>   ifdef CONFIG_MPROFILE_KERNEL
>> -obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_mprofile.o ftrace.o
>> +obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace.o ftrace_entry.o
>>   else
>> -obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_pg.o ftrace_pg.o
>> +obj64-$(CONFIG_FUNCTION_TRACER)		+= ftrace_64_pg.o ftrace_64_pg_entry.o
>>   endif
>> -obj-$(CONFIG_FUNCTION_TRACER)		+= ftrace_low.o
>>   obj-$(CONFIG_TRACING)			+= trace_clock.o
>>   
>>   obj-$(CONFIG_PPC64)			+= $(obj64-y)
>> @@ -26,7 +25,7 @@ GCOV_PROFILE_ftrace.o := n
>>   KCOV_INSTRUMENT_ftrace.o := n
>>   KCSAN_SANITIZE_ftrace.o := n
>>   UBSAN_SANITIZE_ftrace.o := n
>> -GCOV_PROFILE_ftrace_pg.o := n
>> -KCOV_INSTRUMENT_ftrace_pg.o := n
>> -KCSAN_SANITIZE_ftrace_pg.o := n
>> -UBSAN_SANITIZE_ftrace_pg.o := n
>> +GCOV_PROFILE_ftrace_64_pg.o := n
>> +KCOV_INSTRUMENT_ftrace_64_pg.o := n
>> +KCSAN_SANITIZE_ftrace_64_pg.o := n
>> +UBSAN_SANITIZE_ftrace_64_pg.o := n
>> diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.S b/arch/powerpc/kernel/trace/ftrace_64_pg.S
>> deleted file mode 100644
>> index 6708e24db0aba8..00000000000000
>> --- a/arch/powerpc/kernel/trace/ftrace_64_pg.S
>> +++ /dev/null
>> @@ -1,67 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>> -/*
>> - * Split from ftrace_64.S
>> - */
>> -
>> -#include <linux/magic.h>
>> -#include <asm/ppc_asm.h>
>> -#include <asm/asm-offsets.h>
>> -#include <asm/ftrace.h>
>> -#include <asm/ppc-opcode.h>
>> -#include <asm/export.h>
>> -
>> -_GLOBAL_TOC(ftrace_caller)
>> -	lbz	r3, PACA_FTRACE_ENABLED(r13)
>> -	cmpdi	r3, 0
>> -	beqlr
>> -
>> -	/* Taken from output of objdump from lib64/glibc */
>> -	mflr	r3
>> -	ld	r11, 0(r1)
>> -	stdu	r1, -112(r1)
>> -	std	r3, 128(r1)
>> -	ld	r4, 16(r11)
>> -	subi	r3, r3, MCOUNT_INSN_SIZE
>> -.globl ftrace_call
>> -ftrace_call:
>> -	bl	ftrace_stub
>> -	nop
>> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> -.globl ftrace_graph_call
>> -ftrace_graph_call:
>> -	b	ftrace_graph_stub
>> -_GLOBAL(ftrace_graph_stub)
>> -#endif
>> -	ld	r0, 128(r1)
>> -	mtlr	r0
>> -	addi	r1, r1, 112
>> -
>> -_GLOBAL(ftrace_stub)
>> -	blr
>> -
>> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> -_GLOBAL(ftrace_graph_caller)
>> -	addi	r5, r1, 112
>> -	/* load r4 with local address */
>> -	ld	r4, 128(r1)
>> -	subi	r4, r4, MCOUNT_INSN_SIZE
>> -
>> -	/* Grab the LR out of the caller stack frame */
>> -	ld	r11, 112(r1)
>> -	ld	r3, 16(r11)
>> -
>> -	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.
>> -	 */
>> -	ld	r11, 112(r1)
>> -	std	r3, 16(r11)
>> -
>> -	ld	r0, 128(r1)
>> -	mtlr	r0
>> -	addi	r1, r1, 112
>> -	blr
>> -#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>> diff --git a/arch/powerpc/kernel/trace/ftrace_pg.c b/arch/powerpc/kernel/trace/ftrace_64_pg.c
>> similarity index 100%
>> rename from arch/powerpc/kernel/trace/ftrace_pg.c
>> rename to arch/powerpc/kernel/trace/ftrace_64_pg.c
>> diff --git a/arch/powerpc/kernel/trace/ftrace_low.S b/arch/powerpc/kernel/trace/ftrace_64_pg_entry.S
>> similarity index 55%
>> rename from arch/powerpc/kernel/trace/ftrace_low.S
>> rename to arch/powerpc/kernel/trace/ftrace_64_pg_entry.S
>> index 2fc7dd0a5ae968..81dbaf70b1513a 100644
>> --- a/arch/powerpc/kernel/trace/ftrace_low.S
>> +++ b/arch/powerpc/kernel/trace/ftrace_64_pg_entry.S
>> @@ -1,6 +1,6 @@
>>   /* SPDX-License-Identifier: GPL-2.0-or-later */
>>   /*
>> - * Split from entry_64.S
>> + * Split from ftrace_64.S
>>    */
>>   
>>   #include <linux/magic.h>
>> @@ -10,6 +10,62 @@
>>   #include <asm/ppc-opcode.h>
>>   #include <asm/export.h>
>>   
>> +_GLOBAL_TOC(ftrace_caller)
>> +	lbz	r3, PACA_FTRACE_ENABLED(r13)
>> +	cmpdi	r3, 0
>> +	beqlr
>> +
>> +	/* Taken from output of objdump from lib64/glibc */
>> +	mflr	r3
>> +	ld	r11, 0(r1)
>> +	stdu	r1, -112(r1)
>> +	std	r3, 128(r1)
>> +	ld	r4, 16(r11)
>> +	subi	r3, r3, MCOUNT_INSN_SIZE
>> +.globl ftrace_call
>> +ftrace_call:
>> +	bl	ftrace_stub
>> +	nop
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +.globl ftrace_graph_call
>> +ftrace_graph_call:
>> +	b	ftrace_graph_stub
>> +_GLOBAL(ftrace_graph_stub)
>> +#endif
>> +	ld	r0, 128(r1)
>> +	mtlr	r0
>> +	addi	r1, r1, 112
>> +
>> +_GLOBAL(ftrace_stub)
>> +	blr
>> +
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +_GLOBAL(ftrace_graph_caller)
>> +	addi	r5, r1, 112
>> +	/* load r4 with local address */
>> +	ld	r4, 128(r1)
>> +	subi	r4, r4, MCOUNT_INSN_SIZE
>> +
>> +	/* Grab the LR out of the caller stack frame */
>> +	ld	r11, 112(r1)
>> +	ld	r3, 16(r11)
>> +
>> +	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.
>> +	 */
>> +	ld	r11, 112(r1)
>> +	std	r3, 16(r11)
>> +
>> +	ld	r0, 128(r1)
>> +	mtlr	r0
>> +	addi	r1, r1, 112
>> +	blr
>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>> +
>>   .pushsection ".tramp.ftrace.text","aw",@progbits;
>>   .globl ftrace_tramp_text
>>   ftrace_tramp_text:
>> diff --git a/arch/powerpc/kernel/trace/ftrace_mprofile.S b/arch/powerpc/kernel/trace/ftrace_entry.S
>> similarity index 83%
>> rename from arch/powerpc/kernel/trace/ftrace_mprofile.S
>> rename to arch/powerpc/kernel/trace/ftrace_entry.S
>> index ffb1db38684998..e8339706e735b1 100644
>> --- a/arch/powerpc/kernel/trace/ftrace_mprofile.S
>> +++ b/arch/powerpc/kernel/trace/ftrace_entry.S
>> @@ -249,3 +249,68 @@ livepatch_handler:
>>   	/* Return to original caller of live patched function */
>>   	blr
>>   #endif /* CONFIG_LIVEPATCH */
>> +
>> +_GLOBAL(mcount)
>> +_GLOBAL(_mcount)
>> +EXPORT_SYMBOL(_mcount)
>> +	mflr	r12
>> +	mtctr	r12
>> +	mtlr	r0
>> +	bctr
>> +
>> +#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 */
>> +	std	r2,  -16(r1)
>> +	std	r31, -8(r1)
>> +	mr	r31, r1
>> +	stdu	r1, -112(r1)
>> +
>> +	/*
>> +	 * We might be called from a module.
>> +	 * Switch to our TOC to run inside the core kernel.
>> +	 */
>> +	LOAD_PACA_TOC()
>> +#else
>> +	stwu	r1, -16(r1)
>> +	stw	r3, 8(r1)
>> +	stw	r4, 12(r1)
>> +#endif
>> +
>> +	bl	ftrace_return_to_handler
>> +	nop
>> +
>> +	/* 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
>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>> +
>> +.pushsection ".tramp.ftrace.text","aw",@progbits;
>> +.globl ftrace_tramp_text
>> +ftrace_tramp_text:
>> +	.space 32
>> +.popsection
>> +
>> +.pushsection ".tramp.ftrace.init","aw",@progbits;
>> +.globl ftrace_tramp_init
>> +ftrace_tramp_init:
>> +	.space 32
>> +.popsection
>