Re: [PATCH] powerpc/64: ftrace don't trace real mode

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Hi Nick,

Nicholas Piggin wrote:
> This warns and prevents tracing attempted in a real-mode context.

Is this something you're seeing often? Last time we looked at this, KVM 
was the biggest offender and we introduced paca->ftrace_enabled as a way 
to disable ftrace while in KVM code.

While this is cheap when handling ftrace_regs_caller() as done in this 
patch, for simple function tracing (see below), we will have to grab the 
MSR which will slow things down slightly.

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/kernel/trace/ftrace.c            |  3 +++
>  .../powerpc/kernel/trace/ftrace_64_mprofile.S | 19 +++++++++++++++----
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 7ea0ca044b65..ef965815fcb9 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -949,6 +949,9 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>  {
>  	unsigned long return_hooker;
> 
> +	if (WARN_ON_ONCE((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR)))
> +		goto out;
> +

This is called on function entry to redirect function return to a 
trampoline if needed. I am not sure if we have (or will have) too many C 
functions that disable MSR_IR|MSR_DR. Unless the number of such 
functions is large, it might be preferable to mark specific functions as 
notrace.

>  	if (unlikely(ftrace_graph_is_dead()))
>  		goto out;
> 
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index f9fd5f743eba..6205f15cb603 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -51,16 +51,21 @@ _GLOBAL(ftrace_regs_caller)
>  	SAVE_10GPRS(12, r1)
>  	SAVE_10GPRS(22, r1)
> 
> -	/* Save previous stack pointer (r1) */
> -	addi	r8, r1, SWITCH_FRAME_SIZE
> -	std	r8, GPR1(r1)
> -
>  	/* Load special regs for save below */
>  	mfmsr   r8
>  	mfctr   r9
>  	mfxer   r10
>  	mfcr	r11
> 
> +	/* Shouldn't be called in real mode */
> +	andi.	r3,r8,(MSR_IR|MSR_DR)
> +	cmpdi	r3,(MSR_IR|MSR_DR)
> +	bne	ftrace_bad_realmode
> +
> +	/* Save previous stack pointer (r1) */
> +	addi	r8, r1, SWITCH_FRAME_SIZE
> +	std	r8, GPR1(r1)
> +

This stomps on the MSR value in r8, which is saved into pt_regs further 
below.

You'll also have to handle ftrace_caller() which is used for simple 
function tracing. We don't read the MSR there today, but that will be 
needed if we want to suppress tracing.


- Naveen