Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Naveen N. Rao]

Michael Ellerman wrote:

Nicholas Piggin  writes:


On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao"  wrote:


We have some C code that we call into from real mode where we cannot
take any exceptions. Though the C functions themselves are mostly safe,
if these functions are traced, there is a possibility that we may take
an exception. For instance, in certain conditions, the ftrace code uses
WARN(), which uses a 'trap' to do its job.

For such scenarios, introduce a new field in paca 'ftrace_disabled',
which is checked on ftrace entry before continuing. This field can then
be set to a non-zero value to disable/pause ftrace, and reset to zero to
resume ftrace.

Since KVM is the only user for this currently, we guard the
ftrace/mcount checks within CONFIG_KVM. This can later be removed
if/when there are other users.


Why not test HSTATE_IN_GUEST then? Add ftrace_disabled if non-KVM users
come along.


We want to use it for the kexec down path, we've already had bugs there.


I had looked at kexec and we seem to be disabling ftrace in 
machine_kexec(). Has that been problematic?


I didn't actually realize that you wanted to use this in kexec path as 
well.




So please keep the separate flag and pull it out of #ifdef KVM.


Sure.

- Naveen

Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Nicholas Piggin]

On Tue, 20 Mar 2018 09:34:43 +1100
Michael Ellerman  wrote:

> Nicholas Piggin  writes:
> 
> > On Mon, 19 Mar 2018 14:43:00 +0530
> > "Naveen N. Rao"  wrote:
> >  
> >> We have some C code that we call into from real mode where we cannot
> >> take any exceptions. Though the C functions themselves are mostly safe,
> >> if these functions are traced, there is a possibility that we may take
> >> an exception. For instance, in certain conditions, the ftrace code uses
> >> WARN(), which uses a 'trap' to do its job.
> >> 
> >> For such scenarios, introduce a new field in paca 'ftrace_disabled',
> >> which is checked on ftrace entry before continuing. This field can then
> >> be set to a non-zero value to disable/pause ftrace, and reset to zero to
> >> resume ftrace.
> >> 
> >> Since KVM is the only user for this currently, we guard the
> >> ftrace/mcount checks within CONFIG_KVM. This can later be removed
> >> if/when there are other users.  
> >
> > Why not test HSTATE_IN_GUEST then? Add ftrace_disabled if non-KVM users
> > come along.  
> 
> We want to use it for the kexec down path, we've already had bugs there.
> 
> So please keep the separate flag and pull it out of #ifdef KVM.

Okay that's fine.

> If we're worried about space usage in the paca we can probably
> consolidate this and some other things into a flags word.

I'm not too concerned about some more u8 flags.

Thanks,
Nick

Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Michael Ellerman]

Nicholas Piggin  writes:

> On Mon, 19 Mar 2018 14:43:00 +0530
> "Naveen N. Rao"  wrote:
>
>> We have some C code that we call into from real mode where we cannot
>> take any exceptions. Though the C functions themselves are mostly safe,
>> if these functions are traced, there is a possibility that we may take
>> an exception. For instance, in certain conditions, the ftrace code uses
>> WARN(), which uses a 'trap' to do its job.
>> 
>> For such scenarios, introduce a new field in paca 'ftrace_disabled',
>> which is checked on ftrace entry before continuing. This field can then
>> be set to a non-zero value to disable/pause ftrace, and reset to zero to
>> resume ftrace.
>> 
>> Since KVM is the only user for this currently, we guard the
>> ftrace/mcount checks within CONFIG_KVM. This can later be removed
>> if/when there are other users.
>
> Why not test HSTATE_IN_GUEST then? Add ftrace_disabled if non-KVM users
> come along.

We want to use it for the kexec down path, we've already had bugs there.

So please keep the separate flag and pull it out of #ifdef KVM.

If we're worried about space usage in the paca we can probably
consolidate this and some other things into a flags word.

cheers

Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Naveen N. Rao]

Steven Rostedt wrote:

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao"  wrote:


diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S 
b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index 3f3e81852422..fdf702b4df25 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -60,6 +60,19 @@ _GLOBAL(ftrace_caller)
mfxer   r10
mfcrr11
 
+#ifdef CONFIG_KVM

+   lbz r3, PACA_FTRACE_DISABLED(r13)
+   cmpdi   r3, 0
+   beq 1f
+   mflrr3
+   mtctr   r3
+   REST_GPR(3, r1)
+   addir1, r1, SWITCH_FRAME_SIZE
+   mtlrr0
+   bctr
+1:
+#endif


I wonder if we should try to move the return out of the fast path (for
cache reasons), as most of the time the above compare will be true. That
is:

#ifdef CONFIG_KVM
lbz r3, PACA_FTRACE_DISABLED(r13)
cmpdi   r3, 0
bne no_trace
#endif

/* rest of ftrace_caller code */

/* after ftrace_caller... */
bctr/* jump after _mcount site */

#ifdef  CONFIG_KVM
no_trace:
mflrr3
mtctr   r3
REST_GPR(3, r1)
addir1, r1, SWITCH_FRAME_SIZE
mtlrr0
bctr
#endif


Thanks, I'll incorporate those changes in my next version.

- Naveen

Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Naveen N. Rao]

Nicholas Piggin wrote:

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao"  wrote:


We have some C code that we call into from real mode where we cannot
take any exceptions. Though the C functions themselves are mostly safe,
if these functions are traced, there is a possibility that we may take
an exception. For instance, in certain conditions, the ftrace code uses
WARN(), which uses a 'trap' to do its job.

For such scenarios, introduce a new field in paca 'ftrace_disabled',
which is checked on ftrace entry before continuing. This field can then
be set to a non-zero value to disable/pause ftrace, and reset to zero to
resume ftrace.

Since KVM is the only user for this currently, we guard the
ftrace/mcount checks within CONFIG_KVM. This can later be removed
if/when there are other users.


Why not test HSTATE_IN_GUEST then? Add ftrace_disabled if non-KVM users
come along.


That's indeed simpler -- thanks for the suggestion!

- Naveen

Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Steven Rostedt]

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao"  wrote:

> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S 
> b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index 3f3e81852422..fdf702b4df25 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -60,6 +60,19 @@ _GLOBAL(ftrace_caller)
>   mfxer   r10
>   mfcrr11
>  
> +#ifdef CONFIG_KVM
> + lbz r3, PACA_FTRACE_DISABLED(r13)
> + cmpdi   r3, 0
> + beq 1f
> + mflrr3
> + mtctr   r3
> + REST_GPR(3, r1)
> + addir1, r1, SWITCH_FRAME_SIZE
> + mtlrr0
> + bctr
> +1:
> +#endif

I wonder if we should try to move the return out of the fast path (for
cache reasons), as most of the time the above compare will be true. That
is:

#ifdef CONFIG_KVM
lbz r3, PACA_FTRACE_DISABLED(r13)
cmpdi   r3, 0
bne no_trace
#endif

/* rest of ftrace_caller code */

/* after ftrace_caller... */
bctr/* jump after _mcount site */

#ifdef  CONFIG_KVM
no_trace:
mflrr3
mtctr   r3
REST_GPR(3, r1)
addir1, r1, SWITCH_FRAME_SIZE
mtlrr0
bctr
#endif


-- Steve

> +
>   /* Get the _mcount() call site out of LR */
>   mflrr7
>   /* Save it as pt_regs->nip */

Re: [PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Nicholas Piggin]

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao"  wrote:

> We have some C code that we call into from real mode where we cannot
> take any exceptions. Though the C functions themselves are mostly safe,
> if these functions are traced, there is a possibility that we may take
> an exception. For instance, in certain conditions, the ftrace code uses
> WARN(), which uses a 'trap' to do its job.
> 
> For such scenarios, introduce a new field in paca 'ftrace_disabled',
> which is checked on ftrace entry before continuing. This field can then
> be set to a non-zero value to disable/pause ftrace, and reset to zero to
> resume ftrace.
> 
> Since KVM is the only user for this currently, we guard the
> ftrace/mcount checks within CONFIG_KVM. This can later be removed
> if/when there are other users.

Why not test HSTATE_IN_GUEST then? Add ftrace_disabled if non-KVM users
come along.

Thanks,
Nick

[PATCH 1/2] powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code paths

[Naveen N. Rao]

We have some C code that we call into from real mode where we cannot
take any exceptions. Though the C functions themselves are mostly safe,
if these functions are traced, there is a possibility that we may take
an exception. For instance, in certain conditions, the ftrace code uses
WARN(), which uses a 'trap' to do its job.

For such scenarios, introduce a new field in paca 'ftrace_disabled',
which is checked on ftrace entry before continuing. This field can then
be set to a non-zero value to disable/pause ftrace, and reset to zero to
resume ftrace.

Since KVM is the only user for this currently, we guard the
ftrace/mcount checks within CONFIG_KVM. This can later be removed
if/when there are other users.

Signed-off-by: Naveen N. Rao 
---
 arch/powerpc/include/asm/paca.h|  1 +
 arch/powerpc/kernel/asm-offsets.c  |  1 +
 arch/powerpc/kernel/trace/ftrace_64_mprofile.S | 13 +
 arch/powerpc/kernel/trace/ftrace_64_pg.S   |  6 ++
 4 files changed, 21 insertions(+)

diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index d2bf71dddbef..4f47adc2a408 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -211,6 +211,7 @@ struct paca_struct {
u16 in_mce;
u8 hmi_event_available; /* HMI event is available */
u8 hmi_p9_special_emu;  /* HMI P9 special emulation */
+   u8 ftrace_disabled;
 #endif
 
/* Stuff for accurate time accounting */
diff --git a/arch/powerpc/kernel/asm-offsets.c 
b/arch/powerpc/kernel/asm-offsets.c
index ea5eb91b836e..8e4fc96ff6bc 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -240,6 +240,7 @@ int main(void)
OFFSET(PACA_RFI_FLUSH_FALLBACK_AREA, paca_struct, 
rfi_flush_fallback_area);
OFFSET(PACA_EXRFI, paca_struct, exrfi);
OFFSET(PACA_L1D_FLUSH_SIZE, paca_struct, l1d_flush_size);
+   OFFSET(PACA_FTRACE_DISABLED, paca_struct, ftrace_disabled);
 
 #endif
OFFSET(PACAHWCPUID, paca_struct, hw_cpu_id);
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S 
b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
index 3f3e81852422..fdf702b4df25 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
@@ -60,6 +60,19 @@ _GLOBAL(ftrace_caller)
mfxer   r10
mfcrr11
 
+#ifdef CONFIG_KVM
+   lbz r3, PACA_FTRACE_DISABLED(r13)
+   cmpdi   r3, 0
+   beq 1f
+   mflrr3
+   mtctr   r3
+   REST_GPR(3, r1)
+   addir1, r1, SWITCH_FRAME_SIZE
+   mtlrr0
+   bctr
+1:
+#endif
+
/* Get the _mcount() call site out of LR */
mflrr7
/* Save it as pt_regs->nip */
diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.S 
b/arch/powerpc/kernel/trace/ftrace_64_pg.S
index f095358da96e..5b2a99129322 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_pg.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_pg.S
@@ -16,6 +16,12 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 _GLOBAL_TOC(ftrace_caller)
+#ifdef CONFIG_KVM
+   lbz r3, PACA_FTRACE_DISABLED(r13)
+   cmpdi   r3, 0
+   bnelr
+#endif
+
/* Taken from output of objdump from lib64/glibc */
mflrr3
ld  r11, 0(r1)
-- 
2.16.2