arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

PATCH Superseded Abel Vesa 4 participants 9 messages Feb 24, 2017 – Mar 2, 2017

Subsystems (2)

Arm (32-bit) ftrace

lore

Share & tools

lore

Abel Vesa

author

2017-02-24 22:58 UTC #0 lore raw

PATCH v4 1/1 arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

The DYNAMIC_FTRACE_WITH_REGS configuration makes it possible for a ftrace operation to specify if registers need to saved/restored by the ftrace handler. This is needed by kgraft and possibly other ftrace-based tools, and the ARM architecture is currently lacking this feature. It would also be the first step to support the "Kprobes-on-ftrace" optimization on ARM.

This patch introduces a new ftrace handler that stores the registers on the stack before calling the next stage. The registers are restored from the stack before going back to the instrumented function.

A side-effect of this patch is to activate the support for ftrace_modify_call() as it defines ARCH_SUPPORTS_FTRACE_OPS for the ARM architecture.

Signed-off-by Abel Vesa <[email protected]>

---
 arch/arm/Kconfig               |  1 +
 arch/arm/include/asm/ftrace.h  |  4 ++
 arch/arm/kernel/entry-ftrace.S | 83 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/ftrace.c       | 37 +++++++++++++++++++
 4 files changed, 125 insertions(+)

arch/arm/Kconfig +1 -0

index fda6a46..877df5b 100644
@@ -55,6 +55,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)

arch/arm/include/asm/ftrace.h +4 -0

index 22b7311..f379881 100644
@@ -1,6 +1,10 @@
 #ifndef _ASM_ARM_FTRACE
 #define _ASM_ARM_FTRACE
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
+
 #ifdef CONFIG_FUNCTION_TRACER
 #define MCOUNT_ADDR		((unsigned long)(__gnu_mcount_nc))
 #define MCOUNT_INSN_SIZE	4 /* sizeof mcount call */

arch/arm/kernel/entry-ftrace.S +83 -0

107 lines not on this page · raw message

arch/arm/kernel/ftrace.c +37 -0

index 3f17594..cb0358c 100644
@@ -139,6 +139,15 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	ret = ftrace_modify_code(pc, 0, new, false);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (!ret) {
+		pc = (unsigned long)&ftrace_regs_call;
+		new = ftrace_call_replace(pc, (unsigned long)func);
+
+		ret = ftrace_modify_code(pc, 0, new, false);
+	}
+#endif
+
 #ifdef CONFIG_OLD_MCOUNT
 	if (!ret) {
 		pc = (unsigned long)&ftrace_call_old;

@@ -157,11 +166,29 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned long ip = rec->ip;
 
 	old = ftrace_nop_replace(rec);
+
+	new = ftrace_call_replace(ip, adjust_address(rec, addr));
+
+	return ftrace_modify_code(rec->ip, old, new, true);
+}
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+				unsigned long addr)
+{
+	unsigned long new, old;
+	unsigned long ip = rec->ip;
+
+	old = ftrace_call_replace(ip, adjust_address(rec, old_addr));
+
 	new = ftrace_call_replace(ip, adjust_address(rec, addr));
 
 	return ftrace_modify_code(rec->ip, old, new, true);
 }
 
+#endif
+
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {

@@ -229,6 +256,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 extern unsigned long ftrace_graph_call;
 extern unsigned long ftrace_graph_call_old;
 extern void ftrace_graph_caller_old(void);
+extern unsigned long ftrace_graph_regs_call;
+extern void ftrace_graph_regs_caller(void);
 
 static int __ftrace_modify_caller(unsigned long *callsite,
 				  void (*func) (void), bool enable)

@@ -251,6 +280,14 @@ static int ftrace_modify_graph_caller(bool enable)
 				     ftrace_graph_caller,
 				     enable);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (!ret)
+		ret = __ftrace_modify_caller(&ftrace_graph_regs_call,
+				     ftrace_graph_regs_caller,
+				     enable);
+#endif
+
+
 #ifdef CONFIG_OLD_MCOUNT
 	if (!ret)
 		ret = __ftrace_modify_caller(&ftrace_graph_call_old,

Signature · 1 line

2.7.4

Nicolai Stange

2017-02-27 15:52 UTC #1 lore raw

Hi Abel,

On Fri, Feb 24 2017, Abel Vesa wrote:

<snip>

arch/arm/Kconfig config ARM · line 58

Quoted 9 lines

arch/arm/Kconfig +1 -0

index fda6a46..877df5b 100644
@@ -55,6 +55,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT

AFAICS, your code depends on the __gnu_mcount_nc calling conventions, i.e. on gcc pushing the original lr on the stack. In particular, there's no implementation of a ftrace_regs_caller_old or so.

So shouldn't this read as !OLD_MCOUNT instead?

Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.

Quoted 3 lines

arch/arm/Kconfig +0 -0

 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)

<snip>

Quoted 12 lines

arch/arm/kernel/entry-ftrace.S +4 -0

index c73c403..3916dd6 100644
@@ -92,12 +92,78 @@
 2:	mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+

arch/arm/kernel/entry-ftrace.S line 108

Quoted 10 lines

arch/arm/kernel/entry-ftrace.S +10 -0

+	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
+
+	add 	ip, sp, #12	@ move in IP the value of SP as it was
+				@ before the push {lr} of the mcount mechanism
+	stmdb	sp!, {ip,lr,pc}
+	stmdb	sp!, {r0-r11,lr}
+
+	@ stack content at this point:
+	@ 0  4          48   52       56   60   64    68       72
+	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |

Being a constant, the saved pc is not very useful, I think.

Wouldn't it be better (and more consistent with other archs) to have

pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr

instead?

A (hypothetical) klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) would do the more intuitive

  regs->ARM_pc = ip;

rather than a

regs->ARM_lr = ip

then.

In addition, the original lr register would be made available to ftrace handlers this way.

Quoted 25 lines

arch/arm/kernel/entry-ftrace.S +23 -0

+	mov r3, sp				@ struct pt_regs*
+	ldr r2, =function_trace_op
+	ldr r2, [r2]				@ pointer to the current
+						@ function tracing op
+	ldr	r1, [sp, #PT_REGS_SIZE]		@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call
+ftrace_regs_call:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_graph_regs_call
+ftrace_graph_regs_call:
+	mov	r0, r0
+#endif
+	@ pop saved regs
+	@ first, get the previous LR value from stack
+	ldr	lr, [sp, #PT_REGS_SIZE]
+	@ now pop the rest of the saved registers INCLUDING stack pointer
+	ldmia   sp, {r0-r11, ip, sp, pc}
+.endm
+

arch/arm/kernel/entry-ftrace.S

Quoted 10 lines

arch/arm/kernel/entry-ftrace.S +10 -0

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+.macro __ftrace_graph_regs_caller
+
+	sub	r0, fp, #4		@ lr of instrumented routine (parent)
+
+	@ called from __ftrace_regs_caller
+	ldr     r1, [sp, #S_LR]		@ instrumented routine (func)
+	mcount_adjust_addr	r1, r1
+
+	sub	r2, r0, #4		@ frame pointer

Given that r2 is prepare_ftrace_return()'s frame_pointer argument, is

r2 = fp - 4 - 4 = fp - 8

really correct / what is wanted here?

arch/arm/kernel/entry-ftrace.S line 153

Quoted 11 lines

arch/arm/kernel/entry-ftrace.S +11 -0

+	bl	prepare_ftrace_return
+
+	@ pop registers saved in ftrace_regs_caller
+	@ first, get the previous LR value from stack
+	ldr	lr, [sp, #PT_REGS_SIZE]
+	@ now pop the rest of the saved registers INCLUDING stack pointer
+	ldmia   sp, {r0-r11, ip, sp, pc}
+.endm
+#endif
+#endif
+

<snip>

Thanks,

Nicolai

Abel Vesa

2017-02-28 00:56 UTC #2 lore raw

Quoted 21 lines · Nicolai Stange

Hi Abel,
On Fri, Feb 24 2017, Abel Vesa wrote:
<snip>
arch/arm/Kconfig config ARM · line 58
Quoted 9 lines
arch/arm/Kconfig +1 -0
index fda6a46..877df5b 100644
@@ -55,6 +55,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
AFAICS, your code depends on the __gnu_mcount_nc calling conventions, i.e. on gcc pushing the original lr on the stack. In particular, there's no implementation of a ftrace_regs_caller_old or so.

Quoted 1 line

So shouldn't this read as !OLD_MCOUNT instead?

The implementation of __ftrace_modify_code which sets the kernel text to rw needs OLD_MCOUNT (that is, the arch specific one, not the generic one).

Quoted 18 lines

Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.
Quoted 3 lines
arch/arm/Kconfig +0 -0
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
<snip>
Quoted 8 lines
arch/arm/kernel/entry-ftrace.S +0 -0
index c73c403..3916dd6 100644
@@ -92,12 +92,78 @@
 2:	mcount_exit
 .endm
 

Quoted 16 lines

Quoted 4 lines

arch/arm/kernel/entry-ftrace.S +4 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+

arch/arm/kernel/entry-ftrace.S line 108

Quoted 10 lines

arch/arm/kernel/entry-ftrace.S +10 -0

+	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
+
+	add 	ip, sp, #12	@ move in IP the value of SP as it was
+				@ before the push {lr} of the mcount mechanism
+	stmdb	sp!, {ip,lr,pc}
+	stmdb	sp!, {r0-r11,lr}
+
+	@ stack content at this point:
+	@ 0  4          48   52       56   60   64    68       72
+	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |

Being a constant, the saved pc is not very useful, I think.

So you're saying skip it ? But you still need to leave space for it. So why not just save it even if the value is not useful?

Quoted 11 lines

Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr
instead?
A (hypothetical) klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) would do the more intuitive

Quoted 7 lines

  regs->ARM_pc = ip;
rather than a
regs->ARM_lr = ip
then.

You are right. There is a subsequent patch I'm currently working on that will enable livepatch and will bring an implementation for klp_arch_set_pc as you described it. I still don't get what is wrong with the code though?

Quoted 43 lines

In addition, the original lr register would be made available to ftrace handlers this way.

Quoted 25 lines

arch/arm/kernel/entry-ftrace.S +23 -0

+	mov r3, sp				@ struct pt_regs*
+	ldr r2, =function_trace_op
+	ldr r2, [r2]				@ pointer to the current
+						@ function tracing op
+	ldr	r1, [sp, #PT_REGS_SIZE]		@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call
+ftrace_regs_call:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_graph_regs_call
+ftrace_graph_regs_call:
+	mov	r0, r0
+#endif
+	@ pop saved regs
+	@ first, get the previous LR value from stack
+	ldr	lr, [sp, #PT_REGS_SIZE]
+	@ now pop the rest of the saved registers INCLUDING stack pointer
+	ldmia   sp, {r0-r11, ip, sp, pc}
+.endm
+

arch/arm/kernel/entry-ftrace.S

Quoted 10 lines

arch/arm/kernel/entry-ftrace.S +10 -0

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+.macro __ftrace_graph_regs_caller
+
+	sub	r0, fp, #4		@ lr of instrumented routine (parent)
+
+	@ called from __ftrace_regs_caller
+	ldr     r1, [sp, #S_LR]		@ instrumented routine (func)
+	mcount_adjust_addr	r1, r1
+
+	sub	r2, r0, #4		@ frame pointer

Given that r2 is prepare_ftrace_return()'s frame_pointer argument, is

Quoted 4 lines

r2 = fp - 4 - 4 = fp - 8
really correct / what is wanted here?

You are right.

-	sub     r2, r0, #4              @ frame pointer
+	mov     r2, fp                  @ frame pointer

Quoted 15 lines

Quoted 11 lines

arch/arm/kernel/entry-ftrace.S +11 -0

+	bl	prepare_ftrace_return
+
+	@ pop registers saved in ftrace_regs_caller
+	@ first, get the previous LR value from stack
+	ldr	lr, [sp, #PT_REGS_SIZE]
+	@ now pop the rest of the saved registers INCLUDING stack pointer
+	ldmia   sp, {r0-r11, ip, sp, pc}
+.endm
+#endif
+#endif
+

<snip>

Quoted 3 lines

Thanks,
Nicolai

Thanks

Nicolai Stange

2017-02-28 10:58 UTC #3 lore raw

Hi Abel,

Quoted 20 lines · Abel Vesa

Quoted 19 lines · Nicolai Stange
On Fri, Feb 24 2017, Abel Vesa wrote:
<snip>
arch/arm/Kconfig config ARM · line 58
Quoted 9 lines
arch/arm/Kconfig +1 -0
index fda6a46..877df5b 100644
@@ -55,6 +55,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
AFAICS, your code depends on the __gnu_mcount_nc calling conventions, i.e. on gcc pushing the original lr on the stack. In particular, there's no implementation of a ftrace_regs_caller_old or so.

Quoted 3 lines

Quoted 1 line
So shouldn't this read as !OLD_MCOUNT instead?
The implementation of __ftrace_modify_code which sets the kernel text to rw needs OLD_MCOUNT (that is, the arch specific one, not the generic one).

You're right that ARM's implementation of __ftrace_modify_code() is hidden within an #ifdef CONFIG_OLD_MCOUNT.

But,
- its implementation doesn't "need" or depend on OLD_MCOUNT
- and it's true in general that the kernel text must be made writable before ftrace_modify_all_code() attempts to write to it.

So, I bet that the set_kernel_text_rw()-calling ARM implementations of arch_ftrace_update_code() and __ftrace_modify_code() resp. have been inserted under that CONFIG_OLD_MCOUNT #ifdef by mistake with commit 80d6b0c2eed2 ("ARM: mm: allow text and rodata sections to be read-only").

In conclusion, I claim that DYNAMIC_FTRACE w/o OLD_MCOUNT had been broken before your patch already. I didn't explicitly test that though.

I think that should be fixed rather than your DYNAMIC_FTRACE_WITH_REGS pulling in OLD_MCOUNT in order to repair DYNAMIC_FTRACE.

Especially since your implementation seems to require !OLD_MCOUNT...

Quoted 33 lines

Quoted 17 lines
Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.
Quoted 3 lines
arch/arm/Kconfig +0 -0
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
<snip>
Quoted 8 lines
arch/arm/kernel/entry-ftrace.S +0 -0
index c73c403..3916dd6 100644
@@ -92,12 +92,78 @@
 2:	mcount_exit
 .endm
 
Quoted 16 lines
Quoted 4 lines
arch/arm/kernel/entry-ftrace.S +4 -0
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
arch/arm/kernel/entry-ftrace.S line 108
Quoted 10 lines
arch/arm/kernel/entry-ftrace.S +10 -0
+	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
+
+	add 	ip, sp, #12	@ move in IP the value of SP as it was
+				@ before the push {lr} of the mcount mechanism
+	stmdb	sp!, {ip,lr,pc}
+	stmdb	sp!, {r0-r11,lr}
+
+	@ stack content at this point:
+	@ 0  4          48   52       56   60   64    68       72
+	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |
Being a constant, the saved pc is not very useful, I think.

Quoted 2 lines

So you're saying skip it ? But you still need to leave space for it. So why not just save it even if the value is not useful?

No, no, I don't want to skip it. I'd just prefer to have the pt_regs' ->ARM_lr and ->ARM_pc slots filled with different, perhaps more useful values:

Quoted 7 lines

Quoted 7 lines
Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr
instead?

The stack would look like this then

@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |

I.e. the pt_regs would capture almost the full context of the instrumented function (except for ip).

Thanks,

Nicolai

Abel Vesa

2017-02-28 11:22 UTC #4 lore raw

Quoted 36 lines · Nicolai Stange

Hi Abel,
Quoted 20 lines · Abel Vesa
Quoted 19 lines · Nicolai Stange
On Fri, Feb 24 2017, Abel Vesa wrote:
<snip>
arch/arm/Kconfig config ARM · line 58
Quoted 9 lines
arch/arm/Kconfig +1 -0
index fda6a46..877df5b 100644
@@ -55,6 +55,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
AFAICS, your code depends on the __gnu_mcount_nc calling conventions, i.e. on gcc pushing the original lr on the stack. In particular, there's no implementation of a ftrace_regs_caller_old or so.
Quoted 3 lines
Quoted 1 line
So shouldn't this read as !OLD_MCOUNT instead?
The implementation of __ftrace_modify_code which sets the kernel text to rw needs OLD_MCOUNT (that is, the arch specific one, not the generic one).
You're right that ARM's implementation of __ftrace_modify_code() is hidden within an #ifdef CONFIG_OLD_MCOUNT.
But,
- its implementation doesn't "need" or depend on OLD_MCOUNT
- and it's true in general that the kernel text must be made writable before ftrace_modify_all_code() attempts to write to it.

Quoted 9 lines

So, I bet that the set_kernel_text_rw()-calling ARM implementations of arch_ftrace_update_code() and __ftrace_modify_code() resp. have been inserted under that CONFIG_OLD_MCOUNT #ifdef by mistake with commit 80d6b0c2eed2 ("ARM: mm: allow text and rodata sections to be read-only").
In conclusion, I claim that DYNAMIC_FTRACE w/o OLD_MCOUNT had been broken before your patch already. I didn't explicitly test that though.

That is correct. The DYNAMIC_FTRACE w/o OLD_MCOUNT doesn't work.

Quoted 5 lines

I think that should be fixed rather than your DYNAMIC_FTRACE_WITH_REGS pulling in OLD_MCOUNT in order to repair DYNAMIC_FTRACE.
Especially since your implementation seems to require !OLD_MCOUNT...

So making arch specific __ftrace_modify_code to be OLD_MCOUNT independent might fix DYNAMIC_FTRACE w/o OLD_MCOUNT and implicitly make DYNAMIC_FTRACE_WITH_REGS not dependent on OLD_MCOUNT.

I will investigate this further today. Probably the whole dependancy between FUNCTION_TRACER and OLD_MCOUNT will need to be changed/updated.

Quoted 48 lines

Quoted 33 lines
Quoted 17 lines
Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.
Quoted 3 lines
arch/arm/Kconfig +0 -0
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
<snip>
Quoted 8 lines
arch/arm/kernel/entry-ftrace.S +0 -0
index c73c403..3916dd6 100644
@@ -92,12 +92,78 @@
 2:	mcount_exit
 .endm
 
Quoted 16 lines
Quoted 4 lines
arch/arm/kernel/entry-ftrace.S +4 -0
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
arch/arm/kernel/entry-ftrace.S line 108
Quoted 10 lines
arch/arm/kernel/entry-ftrace.S +10 -0
+	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
+
+	add 	ip, sp, #12	@ move in IP the value of SP as it was
+				@ before the push {lr} of the mcount mechanism
+	stmdb	sp!, {ip,lr,pc}
+	stmdb	sp!, {r0-r11,lr}
+
+	@ stack content at this point:
+	@ 0  4          48   52       56   60   64    68       72
+	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |
Being a constant, the saved pc is not very useful, I think.
Quoted 2 lines
So you're saying skip it ? But you still need to leave space for it. So why not just save it even if the value is not useful?
No, no, I don't want to skip it. I'd just prefer to have the pt_regs' ->ARM_lr and ->ARM_pc slots filled with different, perhaps more useful values:
Quoted 7 lines
Quoted 7 lines
Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr
instead?

Quoted 9 lines

The stack would look like this then
@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |
I.e. the pt_regs would capture almost the full context of the instrumented function (except for ip).

So basicly what you are saying is:
- instead of current LR save original LR (previous one saved in instrumented function epilog)
- instead of current PC save original PC (previous one saved in instrumented function epilog)

I still don't see the point of saving the actual value of PC since nobody will ever restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree, it could be the original one in pt_regs.

I'll look into this sometime today or tomorrow and get back with updates.

Quoted 3 lines

Thanks,
Nicolai

Thanks

Russell King - ARM Linux

maintainer

2017-02-28 11:46 UTC #5 lore raw

Quoted 27 lines · Abel Vesa

Quoted 13 lines · Nicolai Stange
Hi Abel,
Quoted 8 lines · Abel Vesa
Quoted 7 lines · Nicolai Stange
On Fri, Feb 24 2017, Abel Vesa wrote:
Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr
instead?
Quoted 9 lines
The stack would look like this then
@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |
I.e. the pt_regs would capture almost the full context of the instrumented function (except for ip).
So basicly what you are saying is:
- instead of current LR save original LR (previous one saved in instrumented function epilog)
- instead of current PC save original PC (previous one saved in instrumented function epilog)

Quoted 5 lines

I still don't see the point of saving the actual value of PC since nobody will ever restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree, it could be the original one in pt_regs.
I'll look into this sometime today or tomorrow and get back with updates.

Which is exactly what I proposed, with code, on one of the previous iterations of this patch...

Signature · 3 lines

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Abel Vesa

2017-02-28 11:54 UTC #6 lore raw

Quoted 28 lines · Russell King - ARM Linux

Quoted 27 lines · Abel Vesa
Quoted 13 lines · Nicolai Stange
Hi Abel,
Quoted 8 lines · Abel Vesa
Quoted 7 lines · Nicolai Stange
On Fri, Feb 24 2017, Abel Vesa wrote:
Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr
instead?
Quoted 9 lines
The stack would look like this then
@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |
I.e. the pt_regs would capture almost the full context of the instrumented function (except for ip).
So basicly what you are saying is:
- instead of current LR save original LR (previous one saved in instrumented function epilog)
- instead of current PC save original PC (previous one saved in instrumented function epilog)

Quoted 8 lines

Quoted 5 lines
I still don't see the point of saving the actual value of PC since nobody will ever restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree, it could be the original one in pt_regs.
I'll look into this sometime today or tomorrow and get back with updates.
Which is exactly what I proposed, with code, on one of the previous iterations of this patch...

Fair enough. I probably missunderstood your comments then.

Thanks.

Quoted 5 lines

Signature · 4 lines

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Abel Vesa

2017-03-02 21:01 UTC #7 lore raw

Quoted 14 lines · Abel Vesa

Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Abel Vesa
Quoted 11 lines · Nicolai Stange
Hi Abel,
Quoted 7 lines · Abel Vesa
Quoted 6 lines · Nicolai Stange
On Fri, Feb 24 2017, Abel Vesa wrote:
Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
instead?
The stack would look like this then
@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |

Just to make sure we're on the same page. If we are replacing the LR with the original_LR is it worth keeping around the one pushed before the ftrace_regs_caller is called?

Another thing, PC needs to be new_LR and then we can restore all regs r0 through r15 like this:

	ldmia   sp, {r0-r15}

Quoted 24 lines

Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
I.e. the pt_regs would capture almost the full context of the instrumented function (except for ip).
So basicly what you are saying is:
- instead of current LR save original LR (previous one saved in instrumented function epilog)
- instead of current PC save original PC (previous one saved in instrumented function epilog)
Quoted 8 lines
Quoted 5 lines
I still don't see the point of saving the actual value of PC since nobody will ever restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree, it could be the original one in pt_regs.
I'll look into this sometime today or tomorrow and get back with updates.
Which is exactly what I proposed, with code, on one of the previous iterations of this patch...
Fair enough. I probably missunderstood your comments then.
Thanks.
Quoted 5 lines
Signature · 4 lines
-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Russell King - ARM Linux

maintainer

2017-03-02 23:03 UTC #8 lore raw

Quoted 22 lines · Abel Vesa

Quoted 14 lines · Abel Vesa
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Abel Vesa
Quoted 11 lines · Nicolai Stange
Hi Abel,
Quoted 7 lines · Abel Vesa
Quoted 6 lines · Nicolai Stange
On Fri, Feb 24 2017, Abel Vesa wrote:
Wouldn't it be better (and more consistent with other archs) to have
pt_regs->ARM_lr = original lr
pt_refs->ARM_pc = current lr
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
instead?
The stack would look like this then
@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |

Quoted 8 lines

Just to make sure we're on the same page. If we are replacing the LR with the original_LR is it worth keeping around the one pushed before the ftrace_regs_caller is called?
Another thing, PC needs to be new_LR and then we can restore all regs r0 through r15 like this:
	ldmia   sp, {r0-r15}

That's the intention - the point is to save the state as it was at the point that the function was entered, not at the point when the ftrace code was entered. What we don't want is the implementation details of GCC's mcount or ftrace's adaption of mcount being exposed via ftrace.

Signature · 3 lines

RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.