From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752074AbdCPRzh (ORCPT ); Thu, 16 Mar 2017 13:55:37 -0400 Received: from mail.kernel.org ([198.145.29.136]:54578 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751628AbdCPRzg (ORCPT ); Thu, 16 Mar 2017 13:55:36 -0400 Date: Thu, 16 Mar 2017 13:55:01 -0400 From: Steven Rostedt To: Linus Torvalds Cc: Linux Kernel Mailing List , Ingo Molnar , Andrew Morton , Thomas Gleixner , Peter Zijlstra , Masami Hiramatsu , "H. Peter Anvin" , Andy Lutomirski , Josh Poimboeuf Subject: Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Message-ID: <20170316135501.00523b78@gandalf.local.home> In-Reply-To: References: <20170316172008.086705006@goodmis.org> <20170316172056.576095199@goodmis.org> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [ Resending again with a "reply-all" instead of just "reply" ] On Thu, 16 Mar 2017 10:40:24 -0700 Linus Torvalds wrote: > On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt wrote: > > > > When ftrace_regs_caller was created, it was designed to preserve flags as > > much as possible as it needed to act just like a breakpoint triggered on the > > same location. But the design is over complicated as it treated all > > operations as modifying flags. But push, mov and lea do not modify flags. > > This means the code can become more simplified by allowing flags to be > > stored further down. > > It still looks overly complicated to me. > > The snippet below is the patch without the "-" lines, so it's the end result: > > > ENTRY(ftrace_regs_caller) > > /* > > * i386 does not save SS and ESP when coming from kernel. > > * Instead, to get sp, ®s->sp is used (see ptrace.h). > > * Unfortunately, that means eflags must be at the same location > > * as the current return ip is. We move the return ip into the > > + * regs->ip location, and move flags into the return ip location. > > + */ > > + pushl $__KERNEL_CS > > + pushl 4(%esp) /* Save the return ip */ > > + > > + /* temporarily save flags in the orig_ax location */ > > + pushf > > > > pushl %gs > > pushl %fs > > pushl %es > > pushl %ds > > pushl %eax > > + > > + /* move flags into the location of where the return ip was */ > > + movl 5*4(%esp), %eax > > + movl $0, 5*4(%esp) /* Load 0 into orig_ax */ > > + movl %eax, 8*4(%esp) /* Load flags in return ip */ > > Why do you do that silly "temporarily save flags" thing? > > Why not just push $0 there? > > Afaik, the sequence could/should be: > > pushl $__KERNEL_CS > pushl 4(%esp) /* Save the return ip */ > pushl $0 > pushl %gs > pushl %fs > pushl %es > pushl %ds > pushl %eax > > /* Fix up eflags now that we have a scratch register */ > pushfl > popl %eax > movl %eax,8(%rsp) > > Or something. There's no point in the unnecessary "shuffle values back > and forth with odd stack offsets". Sure, I can do this. This is the issue of trying to do too much at first and then eliminating what you don't need. :-p I think previous versions (where I was trying to horribly add a stack frame here) had some more logic. -- Steve