linux-kernel - Re: [PATCH V9 2/4] riscv: ftrace: Add ftrace_graph

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87zg6bjqf6.fsf@all.your.base.are.belong.to.us>
Date:   Thu, 11 May 2023 09:08:45 +0200
From:   Björn Töpel <bjorn@...nel.org>
To:     Song Shuai <suagrfillet@...il.com>, paul.walmsley@...ive.com,
        palmer@...belt.com, aou@...s.berkeley.edu, rostedt@...dmis.org,
        mhiramat@...nel.org, mark.rutland@....com, guoren@...nel.org,
        suagrfillet@...il.com, jszhang@...nel.org, e.shatokhin@...ro.com
Cc:     linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH V9 2/4] riscv: ftrace: Add ftrace_graph_func

Song Shuai <suagrfillet@...il.com> writes:

> Here implements ftrace_graph_func as the function graph tracing function
> with FTRACE_WITH_REGS defined.
>
> function_graph_func gets the point of the parent IP and the frame pointer
> from fregs and call prepare_ftrace_return for function graph tracing.
>
> If FTRACE_WITH_REGS isn't defined, the enable/disable helpers of
> ftrace_graph_[regs]_call are revised for serving only ftrace_graph_call
> in the !FTRACE_WITH_REGS version ftrace_caller.
>
> Signed-off-by: Song Shuai <suagrfillet@...il.com>
> Tested-by: Guo Ren <guoren@...nel.org>
> Signed-off-by: Guo Ren <guoren@...nel.org>

[...]

> +
> +	// always save the ABI regs
> +
> +	REG_S x10, PT_A0(sp)
> +	REG_S x11, PT_A1(sp)
> +	REG_S x12, PT_A2(sp)
> +	REG_S x13, PT_A3(sp)
> +	REG_S x14, PT_A4(sp)
> +	REG_S x15, PT_A5(sp)
> +	REG_S x16, PT_A6(sp)
> +	REG_S x17, PT_A7(sp)

Really a nit/more general comment; the RISC-V assembly files is a bit
all over the place in terms of style; When doing changes, try to
prettify it with proper tabs, and maybe we'll have eventual
consistency. ;-)

No tabs ^^^...

> +
> +	// save the leftover regs
> +
> +	.if \all == 1
>  	REG_S x2,  PT_SP(sp)
>  	REG_S x3,  PT_GP(sp)
>  	REG_S x4,  PT_TP(sp)
>  	REG_S x5,  PT_T0(sp)
> -	save_from_x6_to_x31
> +	REG_S x6,  PT_T1(sp)
> +	REG_S x7,  PT_T2(sp)
> +	REG_S x8,  PT_S0(sp)
> +	REG_S x9,  PT_S1(sp)
> +	REG_S x18, PT_S2(sp)
> +	REG_S x19, PT_S3(sp)
> +	REG_S x20, PT_S4(sp)
> +	REG_S x21, PT_S5(sp)
> +	REG_S x22, PT_S6(sp)
> +	REG_S x23, PT_S7(sp)
> +	REG_S x24, PT_S8(sp)
> +	REG_S x25, PT_S9(sp)
> +	REG_S x26, PT_S10(sp)
> +	REG_S x27, PT_S11(sp)
> +	REG_S x28, PT_T3(sp)
> +	REG_S x29, PT_T4(sp)
> +	REG_S x30, PT_T5(sp)
> +	REG_S x31, PT_T6(sp)
> +
> +	// save s0 if FP_TEST defined
> +
> +	.else
> +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> +	REG_S x8,  PT_S0(sp)
> +#endif
> +	.endif
>  	.endm
>  
> -	.macro RESTORE_ALL
> +	.macro RESTORE_ABI_REGS, all=0
> +	REG_L t0,  PT_EPC(sp)
>  	REG_L x1,  PT_RA(sp)
> +	REG_L x10, PT_A0(sp)
> +	REG_L x11, PT_A1(sp)
> +	REG_L x12, PT_A2(sp)
> +	REG_L x13, PT_A3(sp)
> +	REG_L x14, PT_A4(sp)
> +	REG_L x15, PT_A5(sp)
> +	REG_L x16, PT_A6(sp)
> +	REG_L x17, PT_A7(sp)
> +
> +	.if \all == 1
>  	REG_L x2,  PT_SP(sp)
>  	REG_L x3,  PT_GP(sp)
>  	REG_L x4,  PT_TP(sp)
> -	/* Restore t0 with PT_EPC */
> -	REG_L x5,  PT_EPC(sp)
> -	restore_from_x6_to_x31
> +	REG_L x6,  PT_T1(sp)
> +	REG_L x7,  PT_T2(sp)
> +	REG_L x8,  PT_S0(sp)
> +	REG_L x9,  PT_S1(sp)
> +	REG_L x18, PT_S2(sp)
> +	REG_L x19, PT_S3(sp)
> +	REG_L x20, PT_S4(sp)
> +	REG_L x21, PT_S5(sp)
> +	REG_L x22, PT_S6(sp)
> +	REG_L x23, PT_S7(sp)
> +	REG_L x24, PT_S8(sp)
> +	REG_L x25, PT_S9(sp)
> +	REG_L x26, PT_S10(sp)
> +	REG_L x27, PT_S11(sp)
> +	REG_L x28, PT_T3(sp)
> +	REG_L x29, PT_T4(sp)
> +	REG_L x30, PT_T5(sp)
> +	REG_L x31, PT_T6(sp)
>  
> +	.else
> +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> +	REG_L x8,  PT_S0(sp)
> +#endif
> +	.endif
>  	addi	sp, sp, PT_SIZE_ON_STACK
>  	.endm
> +
> +	.macro PREPARE_ARGS
> +	addi	a0, t0, -FENTRY_RA_OFFSET	// ip
> +	la	a1, function_trace_op
> +	REG_L	a2, 0(a1)			// op
> +	mv	a1, ra				// parent_ip
> +	mv	a3, sp				// fregs
> +	.endm

...but here...

> +
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>  
> +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  ENTRY(ftrace_caller)
>  	SAVE_ABI
>  
> @@ -110,33 +229,28 @@ ftrace_graph_call:
>  	jr t0
>  ENDPROC(ftrace_caller)
>  
> -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>  ENTRY(ftrace_regs_caller)
> -	SAVE_ALL
> -
> -	addi	a0, t0, -FENTRY_RA_OFFSET
> -	la	a1, function_trace_op
> -	REG_L	a2, 0(a1)
> -	mv	a1, ra
> -	mv	a3, sp
> +	SAVE_ABI_REGS 1
> +	PREPARE_ARGS
>  
>  ftrace_regs_call:
>  	.global ftrace_regs_call
>  	call	ftrace_stub
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -	addi	a0, sp, PT_RA
> -	REG_L	a1, PT_EPC(sp)
> -	addi	a1, a1, -FENTRY_RA_OFFSET
> -#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> -	mv	a2, s0
> -#endif
> -ftrace_graph_regs_call:
> -	.global ftrace_graph_regs_call
> +	RESTORE_ABI_REGS 1
> +	jr t0

...and not here.

Not a biggie! Nice cleanup!

Acked-by: Björn Töpel <bjorn@...osinc.com>