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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] 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

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] wrote:

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 */

Nicholas Piggin wrote:

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] wrote:
=20

quoted

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
=

Steven Rostedt wrote:

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] wrote:
=20

quoted

diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/power=

pc/kernel/trace/ftrace_64_mprofile.S

quoted

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

=

Nicholas Piggin [off-list ref] writes:

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] wrote:

quoted

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

On Tue, 20 Mar 2018 09:34:43 +1100
Michael Ellerman [off-list ref] wrote:

Nicholas Piggin [off-list ref] writes:

quoted

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] wrote:
 

quoted

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

Michael Ellerman wrote:

Nicholas Piggin [off-list ref] writes:
=20

quoted

On Mon, 19 Mar 2018 14:43:00 +0530
"Naveen N. Rao" [off-list ref] wrote:

quoted

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

quoted

quoted

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

=