From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757223AbdLWCnl (ORCPT ); Fri, 22 Dec 2017 21:43:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40966 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757179AbdLWCnk (ORCPT ); Fri, 22 Dec 2017 21:43:40 -0500 Date: Fri, 22 Dec 2017 20:43:37 -0600 From: Josh Poimboeuf To: Steven Rostedt Cc: LKML , Ingo Molnar , Thomas Gleixner , Andrew Morton , x86@kernel.org, Linus Torvalds Subject: Re: [RFC][PATCH][REGRESSION FIX] x86/ftrace: Add ORC annotation to 64 bit ftrace assembly Message-ID: <20171223024337.e6ad3gqldiju7ndl@treble> References: <20171222131841.4564c975@gandalf.local.home> <20171222183024.47yadfkznhwhrlgf@treble> <20171222133801.3c9b1078@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20171222133801.3c9b1078@gandalf.local.home> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Sat, 23 Dec 2017 02:43:40 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 22, 2017 at 01:38:01PM -0500, Steven Rostedt wrote: > On Fri, 22 Dec 2017 12:30:24 -0600 > Josh Poimboeuf wrote: > > > I'm officially on vacation but I got a chance to look at this myself a > > few minutes ago. This looks mostly right. In theory, you should able > > to mark those as functions by changing END to ENDPROC. Then you won't > > need all the UNWIND_HINT_FUNCs. > > Yep, that was my first solution, but as you found, objtool complained > about it. > > > > > I tried making that change, but objtool thinks the stack frame is still > > modified when the functions return. I didn't see anything obvious in > > save_mcount_regs or restore_mcount_regs that should cause it to think > > that. I'll need to look into it a little more. > > Thanks! > > Worse comes to worse, we can keep this change, and you can enjoy your > holidays. I just need this fixed before 4.15 is released. > > I'll be jumping into objtool shortly, to see if I can merge the code > from recordmcount.c with it too. I'm going to need to learn ORC and > DWARF to start on extending the function tracer to act more like > tracepoints (like I discussed with Linus in Prague). Objtool doesn't like the fact that save_mcount_regs pushes rbp at the beginning, but it's never restored (directly, at least). How about something like this instead, where it skips the original rbp push? I didn't test it, but I think it should work. It at least gets rid of the warnings and doesn't need any unwind hints other than the UNWIND_HINT_EMPTY in return_to_handler. diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 81bb565f4497..7e2baf7304ae 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -29,10 +29,13 @@ KASAN_SANITIZE_stacktrace.o := n KASAN_SANITIZE_paravirt.o := n OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o := y -OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y OBJECT_FILES_NON_STANDARD_test_nx.o := y OBJECT_FILES_NON_STANDARD_paravirt_patch_$(BITS).o := y +ifdef CONFIG_FRAME_POINTER +OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o := y +endif + # If instrumentation of this dir is enabled, boot hangs during first second. # Probably could be more selective here, but note that files related to irqs, # boot, dumpstack/stacktrace, etc are either non-interesting or can lead to diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index c832291d948a..38b079626018 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -7,6 +7,7 @@ #include #include #include +#include .code64 @@ -20,7 +21,6 @@ EXPORT_SYMBOL(__fentry__) EXPORT_SYMBOL(mcount) #endif -/* All cases save the original rbp (8 bytes) */ #ifdef CONFIG_FRAME_POINTER # ifdef CC_USING_FENTRY /* Save parent and function stack frames (rip and rbp) */ @@ -31,7 +31,7 @@ EXPORT_SYMBOL(mcount) # endif #else /* No need to save a stack frame */ -# define MCOUNT_FRAME_SIZE 8 +# define MCOUNT_FRAME_SIZE 0 #endif /* CONFIG_FRAME_POINTER */ /* Size of stack used to save mcount regs in save_mcount_regs */ @@ -64,10 +64,10 @@ EXPORT_SYMBOL(mcount) */ .macro save_mcount_regs added=0 - /* Always save the original rbp */ +#ifdef CONFIG_FRAME_POINTER + /* Save the original rbp */ pushq %rbp -#ifdef CONFIG_FRAME_POINTER /* * Stack traces will stop at the ftrace trampoline if the frame pointer * is not set up properly. If fentry is used, we need to save a frame @@ -105,7 +105,11 @@ EXPORT_SYMBOL(mcount) * Save the original RBP. Even though the mcount ABI does not * require this, it helps out callers. */ +#ifdef CONFIG_FRAME_POINTER movq MCOUNT_REG_SIZE-8(%rsp), %rdx +#else + movq %rbp, %rdx +#endif movq %rdx, RBP(%rsp) /* Copy the parent address into %rsi (second parameter) */ @@ -148,7 +152,7 @@ EXPORT_SYMBOL(mcount) ENTRY(function_hook) retq -END(function_hook) +ENDPROC(function_hook) ENTRY(ftrace_caller) /* save_mcount_regs fills in first two parameters */ @@ -184,7 +188,7 @@ GLOBAL(ftrace_graph_call) /* This is weak to keep gas from relaxing the jumps */ WEAK(ftrace_stub) retq -END(ftrace_caller) +ENDPROC(ftrace_caller) ENTRY(ftrace_regs_caller) /* Save the current flags before any operations that can change them */ @@ -255,7 +259,7 @@ GLOBAL(ftrace_regs_caller_end) jmp ftrace_epilogue -END(ftrace_regs_caller) +ENDPROC(ftrace_regs_caller) #else /* ! CONFIG_DYNAMIC_FTRACE */ @@ -313,9 +317,10 @@ ENTRY(ftrace_graph_caller) restore_mcount_regs retq -END(ftrace_graph_caller) +ENDPROC(ftrace_graph_caller) -GLOBAL(return_to_handler) +ENTRY(return_to_handler) + UNWIND_HINT_EMPTY subq $24, %rsp /* Save the return values */ @@ -330,4 +335,5 @@ GLOBAL(return_to_handler) movq (%rsp), %rax addq $24, %rsp jmp *%rdi +END(return_to_handler) #endif