Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller
- Date: Thu, 16 Mar 2017 13:55:01 -0400
- From: Steven Rostedt <rostedt@xxxxxxxxxxx>
- Subject: Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller
[ Resending again with a "reply-all" instead of just "reply" ]
On Thu, 16 Mar 2017 10:40:24 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> 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 */
> 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.