Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller

[Steven Rostedt <rostedt@goodmis.org>]

[ Resending again with a "reply-all" instead of just "reply" ]

On Thu, 16 Mar 2017 10:40:24 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt <rostedt@goodmis.org> 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, &regs->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