Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1602708

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

Show all headers | View raw

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".

           Linus

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 18:30 +0100
  Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 18:50 +0100
    Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 19:00 +0100
    [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 19:10 +0100
      Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 19:30 +0100
        Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 20:30 +0100
          Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:50 +0100
        Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:30 +0100
          Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-16 20:40 +0100
            Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 21:00 +0100
        Re: [PATCH 4/5 v3] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 20:30 +0100
        [PATCH 4/5 v3.1] ftrace/x86_32: Clean up ftrace_regs_caller Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 21:20 +0100

csiph-web