From: Miroslav Benes <mbenes@suse.cz>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
X86 ML <x86@kernel.org>, Nadav Amit <nadav.amit@gmail.com>,
Andy Lutomirski <luto@kernel.org>,
Dave Hansen <dave.hansen@linux.intel.com>,
Song Liu <songliubraving@fb.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom trampolines
Date: Fri, 15 Nov 2019 10:32:13 +0100 (CET) [thread overview]
Message-ID: <alpine.LSU.2.21.1911151024390.28642@pobox.suse.cz> (raw)
In-Reply-To: <20191114111952.3eb1a011@gandalf.local.home>
On Thu, 14 Nov 2019, Steven Rostedt wrote:
> On Thu, 14 Nov 2019 16:34:58 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
>
> > On Fri, 8 Nov 2019, Steven Rostedt wrote:
> >
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > >
> > > Enable x86 to allow for register_ftrace_direct(), where a custom trampoline
> > > may be called directly from an ftrace mcount/fentry location.
> > >
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >
> > [...]
> >
> > > +++ b/arch/x86/kernel/ftrace_64.S
> > > @@ -88,6 +88,7 @@ EXPORT_SYMBOL(__fentry__)
> > > movq %rdi, RDI(%rsp)
> > > movq %r8, R8(%rsp)
> > > movq %r9, R9(%rsp)
> > > + movq $0, ORIG_RAX(%rsp)
> > > /*
> > > * Save the original RBP. Even though the mcount ABI does not
> > > * require this, it helps out callers.
> > > @@ -114,7 +115,8 @@ EXPORT_SYMBOL(__fentry__)
> > > subq $MCOUNT_INSN_SIZE, %rdi
> > > .endm
> > >
> > > -.macro restore_mcount_regs
> > > +.macro restore_mcount_regs save=0
> > > +
> > > movq R9(%rsp), %r9
> > > movq R8(%rsp), %r8
> > > movq RDI(%rsp), %rdi
> > > @@ -123,10 +125,7 @@ EXPORT_SYMBOL(__fentry__)
> > > movq RCX(%rsp), %rcx
> > > movq RAX(%rsp), %rax
> > >
> > > - /* ftrace_regs_caller can modify %rbp */
> > > - movq RBP(%rsp), %rbp
> > > -
> > > - addq $MCOUNT_REG_SIZE, %rsp
> > > + addq $MCOUNT_REG_SIZE-\save, %rsp
> > >
> > > .endm
> > >
> > > @@ -228,10 +227,30 @@ GLOBAL(ftrace_regs_call)
> > > movq R10(%rsp), %r10
> > > movq RBX(%rsp), %rbx
> > >
> > > - restore_mcount_regs
> > > + movq RBP(%rsp), %rbp
> > > +
> > > + movq ORIG_RAX(%rsp), %rax
> > > + movq %rax, MCOUNT_REG_SIZE-8(%rsp)
> > > +
> > > + /* If ORIG_RAX is anything but zero, make this a call to that */
> > > + movq ORIG_RAX(%rsp), %rax
> > > + cmpq $0, %rax
> > > + je 1f
> > > +
> > > + /* Swap the flags with orig_rax */
> > > + movq MCOUNT_REG_SIZE(%rsp), %rdi
> > > + movq %rdi, MCOUNT_REG_SIZE-8(%rsp)
> > > + movq %rax, MCOUNT_REG_SIZE(%rsp)
> > > +
> > > + restore_mcount_regs 8
> > > +
> > > + jmp 2f
> > > +
> > > +1: restore_mcount_regs
> > > +
> > >
> > > /* Restore flags */
> > > - popfq
> > > +2: popfq
> >
> > If I am reading the code correctly (and I was confused couple of times, so
> > maybe I am not), this is what makes the direct fops incompatible with
> > ipmodify and livepatching for now. Is that correct?
>
> Actually, it's the fact that the return goes to some unknown trampoline
> that may do something else as well.
Right.
> >
> > What are your plans regarding this?
>
> I wanted to see what the eBPF folks were doing, and then perhaps allow
> the ip modify occur too. I could let it happen as well now, and then we
> can see what the fallout is later ;-)
Waiting for eBPF using this first seems to be a good plan. Your call.
> > Moreover, we could replace ftrace_regs_caller with direct fops for live
> > patching when this is merged with all arch support we need. After all, all
>
> Note, direct call is currently only available for x86_64.
Yes, I was speculating over a possibility in the future.
> > we need is to change the rip, which we could do easily in the direct
> > trampoline. On the other hand, it would exclude coexistence of a live
> > patch and a BPF filter (both direct now) on one function.
>
> It may also end up being more complex, and not much of a performance
> benefit. I believe the BPF is injecting programs into the start of
> functions, but your trampoline for live patching may be not much
> different than what ftrace gives you today.
Ok, I made a note in my TODO list and let's see how it will evolve. It is
definitely not something urgent.
Thanks
Miroslav
next prev parent reply other threads:[~2019-11-15 9:32 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-08 21:28 [PATCH 00/10] ftrace: Add register_ftrace_direct() Steven Rostedt
2019-11-08 21:28 ` [PATCH 01/10] ftrace: Separate out the copying of a ftrace_hash from __ftrace_hash_move() Steven Rostedt
2019-11-08 21:28 ` [PATCH 02/10] ftrace: Separate out functionality from ftrace_location_range() Steven Rostedt
2019-11-08 21:28 ` [PATCH 03/10] ftrace: Add register_ftrace_direct() Steven Rostedt
2019-11-09 2:29 ` Alexei Starovoitov
2019-11-09 12:33 ` Steven Rostedt
2019-11-14 18:29 ` Steven Rostedt
2019-11-14 18:34 ` Alexei Starovoitov
2019-11-14 18:48 ` Steven Rostedt
2019-11-14 19:05 ` Steven Rostedt
2019-11-13 14:13 ` Miroslav Benes
2019-11-13 14:34 ` Steven Rostedt
2019-11-08 21:28 ` [PATCH 04/10] ftrace: Add ftrace_find_direct_func() Steven Rostedt
2019-11-08 21:28 ` [PATCH 05/10] ftrace: Add sample module that uses register_ftrace_direct() Steven Rostedt
2019-11-08 21:28 ` [PATCH 06/10] ftrace/selftest: Add tests to test register_ftrace_direct() Steven Rostedt
2019-11-08 21:28 ` [PATCH 07/10] ftrace: Add another example of register_ftrace_direct() use case Steven Rostedt
2019-11-08 21:28 ` [PATCH 08/10] ftrace/selftests: Update the direct call selftests to test two direct calls Steven Rostedt
2019-11-08 21:28 ` [PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom trampolines Steven Rostedt
2019-11-14 15:34 ` Miroslav Benes
2019-11-14 16:19 ` Steven Rostedt
2019-11-15 9:32 ` Miroslav Benes [this message]
2019-11-08 21:28 ` [PATCH 10/10] ftrace/x86: Add a counter to test function_graph with direct Steven Rostedt
2019-11-08 22:51 ` [PATCH 00/10] ftrace: Add register_ftrace_direct() Josh Poimboeuf
2019-11-09 1:00 ` Steven Rostedt
2019-11-11 8:47 ` Peter Zijlstra
2019-11-11 14:15 ` Josh Poimboeuf
2019-11-13 15:10 ` Miroslav Benes
2019-11-13 16:31 ` Steven Rostedt
2019-11-14 9:05 ` Miroslav Benes
2019-11-14 14:36 ` Steven Rostedt
2019-11-14 15:42 ` Miroslav Benes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LSU.2.21.1911151024390.28642@pobox.suse.cz \
--to=mbenes@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=alexei.starovoitov@gmail.com \
--cc=bristot@redhat.com \
--cc=dave.hansen@linux.intel.com \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@kernel.org \
--cc=nadav.amit@gmail.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=songliubraving@fb.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).