| Message ID | 1487977101-29027-1-git-send-email-abelvesa@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Abel, On Fri, Feb 24 2017, Abel Vesa wrote: <snip> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index fda6a46..877df5b 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -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. > 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> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > index c73c403..3916dd6 100644 > --- a/arch/arm/kernel/entry-ftrace.S > +++ b/arch/arm/kernel/entry-ftrace.S > @@ -92,12 +92,78 @@ > 2: mcount_exit > .endm > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + > +.macro __ftrace_regs_caller > + > + 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. > + 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 > + > +#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? > + 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
On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > Hi Abel, > > On Fri, Feb 24 2017, Abel Vesa wrote: > > <snip> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index fda6a46..877df5b 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -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? 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). > > Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL. > > > > 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> > > > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > > index c73c403..3916dd6 100644 > > --- a/arch/arm/kernel/entry-ftrace.S > > +++ b/arch/arm/kernel/entry-ftrace.S > > @@ -92,12 +92,78 @@ > > 2: mcount_exit > > .endm > > > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > > + > > +.macro __ftrace_regs_caller > > + > > + 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? > > 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. 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? > > In addition, the original lr register would be made available to ftrace > handlers this way. > > > > + 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 > > + > > > > +#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? > You are right. - sub r2, r0, #4 @ frame pointer + mov r2, fp @ frame pointer > > + 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 Thanks
Hi Abel, On Tue, Feb 28 2017, Abel Vesa wrote: > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: >> On Fri, Feb 24 2017, Abel Vesa wrote: >> >> <snip> >> >> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> > index fda6a46..877df5b 100644 >> > --- a/arch/arm/Kconfig >> > +++ b/arch/arm/Kconfig >> > @@ -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? > 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... >> Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL. >> >> >> > 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> >> >> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S >> > index c73c403..3916dd6 100644 >> > --- a/arch/arm/kernel/entry-ftrace.S >> > +++ b/arch/arm/kernel/entry-ftrace.S >> > @@ -92,12 +92,78 @@ >> > 2: mcount_exit >> > .endm >> > >> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >> > + >> > +.macro __ftrace_regs_caller >> > + >> > + 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? 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: >> >> 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
On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote: > Hi Abel, > > On Tue, Feb 28 2017, Abel Vesa wrote: > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > >> On Fri, Feb 24 2017, Abel Vesa wrote: > >> > >> <snip> > >> > >> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > >> > index fda6a46..877df5b 100644 > >> > --- a/arch/arm/Kconfig > >> > +++ b/arch/arm/Kconfig > >> > @@ -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? > > 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. > That is correct. The DYNAMIC_FTRACE w/o OLD_MCOUNT doesn't work. > 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. > >> Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL. > >> > >> > >> > 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> > >> > >> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > >> > index c73c403..3916dd6 100644 > >> > --- a/arch/arm/kernel/entry-ftrace.S > >> > +++ b/arch/arm/kernel/entry-ftrace.S > >> > @@ -92,12 +92,78 @@ > >> > 2: mcount_exit > >> > .endm > >> > > >> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > >> > + > >> > +.macro __ftrace_regs_caller > >> > + > >> > + 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? > > 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: > > >> > >> 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). > 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. > Thanks, > > Nicolai Thanks
On Tue, Feb 28, 2017 at 11:22:27AM +0000, Abel Vesa wrote: > On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote: > > Hi Abel, > > > > On Tue, Feb 28 2017, Abel Vesa wrote: > > > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > > >> 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? > > > > 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. Which is exactly what I proposed, with code, on one of the previous iterations of this patch...
On Tue, Feb 28, 2017 at 11:46:38AM +0000, Russell King - ARM Linux wrote: > On Tue, Feb 28, 2017 at 11:22:27AM +0000, Abel Vesa wrote: > > On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote: > > > Hi Abel, > > > > > > On Tue, Feb 28 2017, Abel Vesa wrote: > > > > > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > > > >> 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? > > > > > > 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. > > 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. > > -- > 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.
On Tue, Feb 28, 2017 at 11:54:29AM +0000, Abel Vesa wrote: > On Tue, Feb 28, 2017 at 11:46:38AM +0000, Russell King - ARM Linux wrote: > > On Tue, Feb 28, 2017 at 11:22:27AM +0000, Abel Vesa wrote: > > > On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote: > > > > Hi Abel, > > > > > > > > On Tue, Feb 28 2017, Abel Vesa wrote: > > > > > > > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > > > > >> 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? > > > > > > > > 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} > > > > > > > > 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. > > > > 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. > > > > -- > > 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.
On Thu, Mar 02, 2017 at 09:01:24PM +0000, Abel Vesa wrote: > On Tue, Feb 28, 2017 at 11:54:29AM +0000, Abel Vesa wrote: > > On Tue, Feb 28, 2017 at 11:46:38AM +0000, Russell King - ARM Linux wrote: > > > On Tue, Feb 28, 2017 at 11:22:27AM +0000, Abel Vesa wrote: > > > > On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote: > > > > > Hi Abel, > > > > > > > > > > On Tue, Feb 28 2017, Abel Vesa wrote: > > > > > > > > > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > > > > > >> 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? > > > > > > > > > > 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} 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.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index fda6a46..877df5b 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -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) diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h index 22b7311..f379881 100644 --- a/arch/arm/include/asm/ftrace.h +++ b/arch/arm/include/asm/ftrace.h @@ -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 */ diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S index c73c403..3916dd6 100644 --- a/arch/arm/kernel/entry-ftrace.S +++ b/arch/arm/kernel/entry-ftrace.S @@ -92,12 +92,78 @@ 2: mcount_exit .endm +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + +.macro __ftrace_regs_caller + + 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 | + + 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 + +#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 + 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 + .macro __ftrace_caller suffix mcount_enter mcount_get_lr r1 @ lr of instrumented func mcount_adjust_addr r0, lr @ instrumented function +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + ldr r2, =function_trace_op + ldr r2, [r2] @ pointer to the current + @ function tracing op + mov r3, #0 @ regs is NULL +#endif + .globl ftrace_call\suffix ftrace_call\suffix: bl ftrace_stub @@ -212,6 +278,15 @@ UNWIND(.fnstart) __ftrace_caller UNWIND(.fnend) ENDPROC(ftrace_caller) + +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS +ENTRY(ftrace_regs_caller) +UNWIND(.fnstart) + __ftrace_regs_caller +UNWIND(.fnend) +ENDPROC(ftrace_regs_caller) +#endif + #endif #ifdef CONFIG_FUNCTION_GRAPH_TRACER @@ -220,6 +295,14 @@ UNWIND(.fnstart) __ftrace_graph_caller UNWIND(.fnend) ENDPROC(ftrace_graph_caller) + +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS +ENTRY(ftrace_graph_regs_caller) +UNWIND(.fnstart) + __ftrace_graph_regs_caller +UNWIND(.fnend) +ENDPROC(ftrace_graph_regs_caller) +#endif #endif .purgem mcount_enter diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c index 3f17594..cb0358c 100644 --- a/arch/arm/kernel/ftrace.c +++ b/arch/arm/kernel/ftrace.c @@ -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,
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 <abelvesa@linux.com> --- 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(+)