[PATCH 0/2] Disable ftrace during kvm guest entry/exit

[PATCH 0/2] Disable ftrace during kvm guest entry/exit

[Naveen N. Rao]

This is a follow on from the RFC posted at:
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg130047.html

This series implements a new field in paca called 'ftrace_disabled' to 
be set whenever we want to skip function tracing. This is currently only 
used by KVM guest entry/exit and as such, it is guarded in CONFIG_KVM as 
suggested by Steven Rostedt. This has had some minimal testing, and I 
will continue to test it this week and report back if I see any issues.

- Naveen


Naveen N. Rao (2):
  powerpc64/ftrace: Add a field in paca to disable ftrace in unsafe code
    paths
  powerpc64/ftrace: Disable ftrace during kvm guest entry/exit

 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 ++++++
 arch/powerpc/kvm/book3s_hv_rmhandlers.S        |  8 ++++++++
 5 files changed, 29 insertions(+)

-- 
2.16.2

[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 <naveen.n.rao@linux.vnet.ibm.com>
---
 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
 	mfcr	r11
 
+#ifdef CONFIG_KVM
+	lbz	r3, PACA_FTRACE_DISABLED(r13)
+	cmpdi	r3, 0
+	beq	1f
+	mflr	r3
+	mtctr	r3
+	REST_GPR(3, r1)
+	addi	r1, r1, SWITCH_FRAME_SIZE
+	mtlr	r0
+	bctr
+1:
+#endif
+
 	/* Get the _mcount() call site out of LR */
 	mflr	r7
 	/* 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 */
 	mflr	r3
 	ld	r11, 0(r1)
-- 
2.16.2

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" <naveen.n.rao@linux.vnet.ibm.com> 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

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" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>=20
> 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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> On Mon, 19 Mar 2018 14:43:00 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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

[Nicholas Piggin]

On Tue, 20 Mar 2018 09:34:43 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > On Mon, 19 Mar 2018 14:43:00 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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

[Naveen N. Rao]

Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>=20
>> On Mon, 19 Mar 2018 14:43:00 +0530
>> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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.
>>>=20
>>> 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 t=
o
>>> resume ftrace.
>>>=20
>>> 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.
>=20
> 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=20
machine_kexec(). Has that been problematic?

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

>=20
> 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

[Steven Rostedt]

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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
>  	mfcr	r11
>  
> +#ifdef CONFIG_KVM
> +	lbz	r3, PACA_FTRACE_DISABLED(r13)
> +	cmpdi	r3, 0
> +	beq	1f
> +	mflr	r3
> +	mtctr	r3
> +	REST_GPR(3, r1)
> +	addi	r1, r1, SWITCH_FRAME_SIZE
> +	mtlr	r0
> +	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:
	mflr	r3
	mtctr	r3
	REST_GPR(3, r1)
	addi	r1, r1, SWITCH_FRAME_SIZE
	mtlr	r0
	bctr
#endif


-- Steve

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

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" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>=20
>> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/power=
pc/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
>>  	mfcr	r11
>> =20
>> +#ifdef CONFIG_KVM
>> +	lbz	r3, PACA_FTRACE_DISABLED(r13)
>> +	cmpdi	r3, 0
>> +	beq	1f
>> +	mflr	r3
>> +	mtctr	r3
>> +	REST_GPR(3, r1)
>> +	addi	r1, r1, SWITCH_FRAME_SIZE
>> +	mtlr	r0
>> +	bctr
>> +1:
>> +#endif
>=20
> 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:
>=20
> #ifdef CONFIG_KVM
> 	lbz	r3, PACA_FTRACE_DISABLED(r13)
> 	cmpdi	r3, 0
> 	bne	no_trace
> #endif
>=20
> /* rest of ftrace_caller code */
>=20
> /* after ftrace_caller... */
>         bctr                    /* jump after _mcount site */
>=20
> #ifdef	CONFIG_KVM
> no_trace:
> 	mflr	r3
> 	mtctr	r3
> 	REST_GPR(3, r1)
> 	addi	r1, r1, SWITCH_FRAME_SIZE
> 	mtlr	r0
> 	bctr
> #endif

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

- Naveen

=

[PATCH 2/2] powerpc64/ftrace: Disable ftrace during kvm guest entry/exit

[Naveen N. Rao]

During guest entry/exit, we switch over to/from the guest MMU context.
While doing so, we set our state to KVM_GUEST_MODE_HOST_HV to note down
the fact that we cannot take any exceptions in the hypervisor code.

Since ftrace may be enabled and since it can result in us taking a trap,
disable ftrace by setting paca->ftrace_disabled. Once we exit the guest
and restore host MMU context, we re-enable ftrace.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index f31f357b8c5a..9292087adb68 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -600,6 +600,10 @@ kvmppc_hv_entry:
 	/* Save R1 in the PACA */
 	std	r1, HSTATE_HOST_R1(r13)
 
+	/* Disable ftrace since we can't take a trap any more */
+	li	r6, 1
+	stb	r6, PACA_FTRACE_DISABLED(r13)
+
 	li	r6, KVM_GUEST_MODE_HOST_HV
 	stb	r6, HSTATE_IN_GUEST(r13)
 
@@ -2048,6 +2052,8 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
 	/* Unset guest mode */
 	li	r0, KVM_GUEST_MODE_NONE
 	stb	r0, HSTATE_IN_GUEST(r13)
+	li	r0, 0
+	stb	r0, PACA_FTRACE_DISABLED(r13)
 
 	ld	r0, SFS+PPC_LR_STKOFF(r1)
 	addi	r1, r1, SFS
@@ -3379,6 +3385,8 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX)
 	ld	r8, KVM_HOST_LPCR(r10)
 	mtspr	SPRN_LPCR, r8
 	isync
+	li	r0, 0
+	stb	r0, PACA_FTRACE_DISABLED(r13)
 	li	r0, KVM_GUEST_MODE_NONE
 	stb	r0, HSTATE_IN_GUEST(r13)
 
-- 
2.16.2