Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller
- Date: Thu, 16 Mar 2017 10:40:24 -0700
- From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
- Subject: Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller
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:
> * 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 4(%esp) /* Save the return ip */
/* Fix up eflags now that we have a scratch register */
Or something. There's no point in the unnecessary "shuffle values back
and forth with odd stack offsets".