Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, Oct 18, 2019 at 04:31:25PM -0400, Steven Rostedt wrote: > Still looks ugly ;-) See below. I think it's not so bad. It is only built-tested on 64-bit and objtool complains about something again: arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x0: unreachable instruction but I think it is the better thing to do. > I do the talks hoping someone else will finally sit down and write the > documentation! Well, you can put down the outline of the doc and flesh out section by section gradually. Besides, you have all the text in your slides so it is actually more or less a copy+paste. --- diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst index 29ccd6e61fe5..f55c2bb74d00 100644 --- a/Documentation/asm-annotations.rst +++ b/Documentation/asm-annotations.rst @@ -117,9 +117,9 @@ This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above. So in most cases, developers should write something like in the following example, having some asm instructions in between the macros, of course:: -SYM_FUNC_START(function_hook) +SYM_FUNC_START(memset) ... asm insns ... -SYM_FUNC_END(function_hook) +SYM_FUNC_END(memset) In fact, this kind of annotation corresponds to the now deprecated ``ENTRY`` and ``ENDPROC`` macros. diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S index 8ed1f5d371f0..77be7e7e5e59 100644 --- a/arch/x86/kernel/ftrace_32.S +++ b/arch/x86/kernel/ftrace_32.S @@ -12,18 +12,16 @@ #include #include -# define function_hook __fentry__ -EXPORT_SYMBOL(__fentry__) - #ifdef CONFIG_FRAME_POINTER # define MCOUNT_FRAME 1 /* using frame = true */ #else # define MCOUNT_FRAME 0 /* using frame = false */ #endif -SYM_FUNC_START(function_hook) +SYM_CODE_START(__fentry__) ret -SYM_FUNC_END(function_hook) +SYM_CODE_END(__fentry__) +EXPORT_SYMBOL(__fentry__) SYM_CODE_START(ftrace_caller) diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index 69c8d1b9119e..3029fe4f8547 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -14,9 +14,6 @@ .code64 .section .entry.text, "ax" -# define function_hook __fentry__ -EXPORT_SYMBOL(__fentry__) - #ifdef CONFIG_FRAME_POINTER /* Save parent and function stack frames (rip and rbp) */ # define MCOUNT_FRAME_SIZE(8+16*2) @@ -132,9 +129,10 @@ EXPORT_SYMBOL(__fentry__) #ifdef CONFIG_DYNAMIC_FTRACE -SYM_FUNC_START(function_hook) +SYM_CODE_START(__fentry__) retq -SYM_FUNC_END(function_hook) +SYM_CODE_END(__fentry__) +EXPORT_SYMBOL(__fentry__) SYM_FUNC_START(ftrace_caller) /* save_mcount_regs fills in first two parameters */ @@ -248,7 +246,7 @@ SYM_FUNC_END(ftrace_regs_caller) #else /* ! CONFIG_DYNAMIC_FTRACE */ -SYM_FUNC_START(function_hook) +SYM_CODE_START(__fentry__) cmpq $ftrace_stub, ftrace_trace_function jnz trace @@ -279,7 +277,8 @@ trace: restore_mcount_regs jmp fgraph_trace -SYM_FUNC_END(function_hook) +SYM_CODE_END(__fentry__) +EXPORT_SYMBOL(__fentry__) #endif /* CONFIG_DYNAMIC_FTRACE */ #ifdef CONFIG_FUNCTION_GRAPH_TRACER -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, 18 Oct 2019 21:48:56 +0200 Borislav Petkov wrote: > > The #define was because we use to support mcount or __fentry__, now we > > just support __fentry__, and function_hook describes it better ;-) > > Well sorry but gcc documentation talks about __fentry__. I'd keep the > same name to avoid confusion and explain above it what it is. Much > better. Still looks ugly ;-) > > > Heh, I guess we could, which would probably be quite a long comment as > > it's the key behind ftrace itself. > > Well, you can explain with a couple of sentences what it is and > then point at the bigger document explaining ftrace. Provided, Mr. > Rostedt, you'll stop doing talks and finally sit down and write that > documentation! I do the talks hoping someone else will finally sit down and write the documentation! -- Steve
Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, Oct 18, 2019 at 01:37:35PM -0400, Steven Rostedt wrote: > It just needs to be visible by modules and what not, otherwise linking > will fail. And I assume all of them? > The #define was because we use to support mcount or __fentry__, now we > just support __fentry__, and function_hook describes it better ;-) Well sorry but gcc documentation talks about __fentry__. I'd keep the same name to avoid confusion and explain above it what it is. Much better. > Heh, I guess we could, which would probably be quite a long comment as > it's the key behind ftrace itself. Well, you can explain with a couple of sentences what it is and then point at the bigger document explaining ftrace. Provided, Mr. Rostedt, you'll stop doing talks and finally sit down and write that documentation! :-P -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, 18 Oct 2019 19:13:54 +0200 Borislav Petkov wrote: > On Fri, Oct 18, 2019 at 12:49:56PM -0400, Steven Rostedt wrote: > > On Fri, 18 Oct 2019 12:48:00 -0400 > > Steven Rostedt wrote: > > > > > > Relabel function_hook to be marked really as a function. It is called > > > > from C and has the same expectations towards the stack etc. > > > > > > > And to go even further, it *does not* have the same expectations > > towards the stack. > > > > I think this patch should not be applied. > > There are a couple more markings like that now: > > $ git grep function_hook > Documentation/asm-annotations.rst:120:SYM_FUNC_START(function_hook) > Documentation/asm-annotations.rst:122:SYM_FUNC_END(function_hook) > arch/x86/kernel/ftrace_32.S:15:# define function_hook __fentry__ > arch/x86/kernel/ftrace_32.S:24:SYM_FUNC_START(function_hook) > arch/x86/kernel/ftrace_32.S:26:SYM_FUNC_END(function_hook) > arch/x86/kernel/ftrace_64.S:17:# define function_hook __fentry__ > arch/x86/kernel/ftrace_64.S:135:SYM_FUNC_START(function_hook) > arch/x86/kernel/ftrace_64.S:137:SYM_FUNC_END(function_hook) > arch/x86/kernel/ftrace_64.S:251:SYM_FUNC_START(function_hook) > arch/x86/kernel/ftrace_64.S:282:SYM_FUNC_END(function_hook) > > Frankly, I wouldn't mark this function at all as it is special and I see > a little sense to have it in stack traces but maybe Jiri has another > angle here. I'll let him comment. It just needs to be visible by modules and what not, otherwise linking will fail. > > I guess with the new nomenclature that can be SYM_CODE_* now... > > Then, this magic "function" or a global symbol with an address or > whatever that is (oh, there's #define trickery too) definitely deserves > a comment above it to explain what it is. I even have to build the .s > file to see what it turns into: The #define was because we use to support mcount or __fentry__, now we just support __fentry__, and function_hook describes it better ;-) > > .globl __fentry__ ; .p2align 4, 0x90 ; __fentry__: > retq > .type __fentry__, @function ; .size __fentry__, .-__fentry__ > > Yeah, it is called on every function entry: > > callq 81a01760 <__fentry__> > > but can we please explain with a comment above it what it is? Heh, I guess we could, which would probably be quite a long comment as it's the key behind ftrace itself. -- Steve
Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, Oct 18, 2019 at 12:49:56PM -0400, Steven Rostedt wrote: > On Fri, 18 Oct 2019 12:48:00 -0400 > Steven Rostedt wrote: > > > > Relabel function_hook to be marked really as a function. It is called > > > from C and has the same expectations towards the stack etc. > > > > And to go even further, it *does not* have the same expectations > towards the stack. > > I think this patch should not be applied. There are a couple more markings like that now: $ git grep function_hook Documentation/asm-annotations.rst:120:SYM_FUNC_START(function_hook) Documentation/asm-annotations.rst:122:SYM_FUNC_END(function_hook) arch/x86/kernel/ftrace_32.S:15:# define function_hook __fentry__ arch/x86/kernel/ftrace_32.S:24:SYM_FUNC_START(function_hook) arch/x86/kernel/ftrace_32.S:26:SYM_FUNC_END(function_hook) arch/x86/kernel/ftrace_64.S:17:# define function_hook __fentry__ arch/x86/kernel/ftrace_64.S:135:SYM_FUNC_START(function_hook) arch/x86/kernel/ftrace_64.S:137:SYM_FUNC_END(function_hook) arch/x86/kernel/ftrace_64.S:251:SYM_FUNC_START(function_hook) arch/x86/kernel/ftrace_64.S:282:SYM_FUNC_END(function_hook) Frankly, I wouldn't mark this function at all as it is special and I see a little sense to have it in stack traces but maybe Jiri has another angle here. I'll let him comment. I guess with the new nomenclature that can be SYM_CODE_* now... Then, this magic "function" or a global symbol with an address or whatever that is (oh, there's #define trickery too) definitely deserves a comment above it to explain what it is. I even have to build the .s file to see what it turns into: .globl __fentry__ ; .p2align 4, 0x90 ; __fentry__: retq .type __fentry__, @function ; .size __fentry__, .-__fentry__ Yeah, it is called on every function entry: callq 81a01760 <__fentry__> but can we please explain with a comment above it what it is? Thx. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, 18 Oct 2019 12:48:00 -0400 Steven Rostedt wrote: > > Relabel function_hook to be marked really as a function. It is called > > from C and has the same expectations towards the stack etc. > And to go even further, it *does not* have the same expectations towards the stack. I think this patch should not be applied. -- Steve
Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
On Fri, 18 Oct 2019 16:30:27 - "tip-bot2 for Jiri Slaby" wrote: > The following commit has been merged into the x86/asm branch of tip: > > Commit-ID: f13ad88a984e8090226a8f62d75e87b770eefdf4 > Gitweb: > https://git.kernel.org/tip/f13ad88a984e8090226a8f62d75e87b770eefdf4 > Author:Jiri Slaby > AuthorDate:Fri, 11 Oct 2019 13:51:01 +02:00 > Committer: Borislav Petkov > CommitterDate: Fri, 18 Oct 2019 11:35:41 +02:00 I just noticed this (sorry missed the original patch). > > x86/asm/ftrace: Mark function_hook as function > > Relabel function_hook to be marked really as a function. It is called > from C and has the same expectations towards the stack etc. This is wrong, function_hook is never called from C. It's called via fentry (use to be mcount), and does not have the same semantics as a C function. In fact, that's why it exists in assembly. Because it has to save and restore registers to make it possible to call a C function! -- Steve > > Signed-off-by: Jiri Slaby > Signed-off-by: Borislav Petkov > Cc: "H. Peter Anvin" > Cc: Ingo Molnar > Cc: Josh Poimboeuf > Cc: linux-a...@vger.kernel.org > Cc: Masami Hiramatsu > Cc: Peter Zijlstra > Cc: "Steven Rostedt (VMware)" > Cc: Thomas Gleixner > Cc: x86-ml > Link: https://lkml.kernel.org/r/2019105108.12392-22-jsl...@suse.cz > --- > arch/x86/kernel/ftrace_32.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S > index e0061dc..219be13 100644 > --- a/arch/x86/kernel/ftrace_32.S > +++ b/arch/x86/kernel/ftrace_32.S > @@ -21,9 +21,9 @@ EXPORT_SYMBOL(__fentry__) > # define MCOUNT_FRAME0 /* using frame = false > */ > #endif > > -ENTRY(function_hook) > +SYM_FUNC_START(function_hook) > ret > -END(function_hook) > +SYM_FUNC_END(function_hook) > > ENTRY(ftrace_caller) >
[tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
The following commit has been merged into the x86/asm branch of tip: Commit-ID: f13ad88a984e8090226a8f62d75e87b770eefdf4 Gitweb: https://git.kernel.org/tip/f13ad88a984e8090226a8f62d75e87b770eefdf4 Author:Jiri Slaby AuthorDate:Fri, 11 Oct 2019 13:51:01 +02:00 Committer: Borislav Petkov CommitterDate: Fri, 18 Oct 2019 11:35:41 +02:00 x86/asm/ftrace: Mark function_hook as function Relabel function_hook to be marked really as a function. It is called from C and has the same expectations towards the stack etc. Signed-off-by: Jiri Slaby Signed-off-by: Borislav Petkov Cc: "H. Peter Anvin" Cc: Ingo Molnar Cc: Josh Poimboeuf Cc: linux-a...@vger.kernel.org Cc: Masami Hiramatsu Cc: Peter Zijlstra Cc: "Steven Rostedt (VMware)" Cc: Thomas Gleixner Cc: x86-ml Link: https://lkml.kernel.org/r/2019105108.12392-22-jsl...@suse.cz --- arch/x86/kernel/ftrace_32.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S index e0061dc..219be13 100644 --- a/arch/x86/kernel/ftrace_32.S +++ b/arch/x86/kernel/ftrace_32.S @@ -21,9 +21,9 @@ EXPORT_SYMBOL(__fentry__) # define MCOUNT_FRAME 0 /* using frame = false */ #endif -ENTRY(function_hook) +SYM_FUNC_START(function_hook) ret -END(function_hook) +SYM_FUNC_END(function_hook) ENTRY(ftrace_caller)
