All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>,
	Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
	Petr Mladek <pmladek@suse.com>,
	Joe Lawrence <joe.lawrence@redhat.com>,
	Ingo Molnar <mingo@redhat.com>,
	"Naveen N . Rao" <naveen.n.rao@linux.vnet.ibm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"live-patching@vger.kernel.org" <live-patching@vger.kernel.org>
Subject: Re: [PATCH v1 0/5] Implement livepatch on PPC32
Date: Mon, 13 Dec 2021 12:15:36 -0500	[thread overview]
Message-ID: <20211213121536.25e5488d@gandalf.local.home> (raw)
In-Reply-To: <6209682d-0caa-b779-8763-376a984d8ed8@csgroup.eu>

On Mon, 13 Dec 2021 14:39:15 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > Note, you can implement this first, (I looked over the patches and they
> > seem fine) and then update both ppc64 and ppc32 to implement
> > DYNAMIC_FTRACE_WITH_ARGS.
> >   
> 
> I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
> 
> I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add 
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> 
> Ftrace selftests tell "Testing tracer function_graph: FAILED!".
> 
> Is there anything else to do ?

Yes. Because BPF is now hooking into the function callbacks, it causes
issues with function graph tracer. So what we did was to have function
graph tracing to now use the function tracer callback as well (this allows
both the BPF direct trampolines to work with function graph tracer).

As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
support that for now, I decided to make all the archs change function graph
tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
pain to have too many variants of function tracing between the archs).

The change that did this for x86 was:

0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")

This actually simplifies the function graph tracer, as you no longer need
it's own entry trampoline (still need the trampoline for the return of the
function).

What you need to do is:

In your arch/*/include/asm/ftrace.h add:

struct ftrace_ops;

#define ftrace_graph_func ftrace_graph_func
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
                      struct ftrace_ops *op, struct ftrace_regs *fregs);


Where ftrace_graph_func() is now what is called for the function graph
tracer, directly from the ftrace callbacks (no longer a secondary
trampoline).

Define the ftrace_graph_func() to be something like:

void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
                      struct ftrace_ops *op, struct ftrace_regs *fregs)
{
       struct pt_regs *regs = &fregs->regs;
       unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);

       prepare_ftrace_return(ip, (unsigned long *)stack, 0);
}

This is called by the function tracer code. But because with
DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
also have access to the link register and the stack. Then you can use that
to modify the stack and or link register to jump to the the return
trampoline.

This should all work with powerpc (both 64 and 32) but if it does not, let
me know. I'm happy to help out.

-- Steve

WARNING: multiple messages have this Message-ID (diff)
From: Steven Rostedt <rostedt@goodmis.org>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Petr Mladek <pmladek@suse.com>,
	Joe Lawrence <joe.lawrence@redhat.com>,
	Jiri Kosina <jikos@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@redhat.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	"live-patching@vger.kernel.org" <live-patching@vger.kernel.org>,
	"Naveen N . Rao" <naveen.n.rao@linux.vnet.ibm.com>,
	Miroslav Benes <mbenes@suse.cz>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v1 0/5] Implement livepatch on PPC32
Date: Mon, 13 Dec 2021 12:15:36 -0500	[thread overview]
Message-ID: <20211213121536.25e5488d@gandalf.local.home> (raw)
In-Reply-To: <6209682d-0caa-b779-8763-376a984d8ed8@csgroup.eu>

On Mon, 13 Dec 2021 14:39:15 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > Note, you can implement this first, (I looked over the patches and they
> > seem fine) and then update both ppc64 and ppc32 to implement
> > DYNAMIC_FTRACE_WITH_ARGS.
> >   
> 
> I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
> 
> I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add 
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> 
> Ftrace selftests tell "Testing tracer function_graph: FAILED!".
> 
> Is there anything else to do ?

Yes. Because BPF is now hooking into the function callbacks, it causes
issues with function graph tracer. So what we did was to have function
graph tracing to now use the function tracer callback as well (this allows
both the BPF direct trampolines to work with function graph tracer).

As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
support that for now, I decided to make all the archs change function graph
tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
pain to have too many variants of function tracing between the archs).

The change that did this for x86 was:

0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")

This actually simplifies the function graph tracer, as you no longer need
it's own entry trampoline (still need the trampoline for the return of the
function).

What you need to do is:

In your arch/*/include/asm/ftrace.h add:

struct ftrace_ops;

#define ftrace_graph_func ftrace_graph_func
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
                      struct ftrace_ops *op, struct ftrace_regs *fregs);


Where ftrace_graph_func() is now what is called for the function graph
tracer, directly from the ftrace callbacks (no longer a secondary
trampoline).

Define the ftrace_graph_func() to be something like:

void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
                      struct ftrace_ops *op, struct ftrace_regs *fregs)
{
       struct pt_regs *regs = &fregs->regs;
       unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);

       prepare_ftrace_return(ip, (unsigned long *)stack, 0);
}

This is called by the function tracer code. But because with
DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
also have access to the link register and the stack. Then you can use that
to modify the stack and or link register to jump to the the return
trampoline.

This should all work with powerpc (both 64 and 32) but if it does not, let
me know. I'm happy to help out.

-- Steve

  reply	other threads:[~2021-12-13 17:15 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-28 12:24 [PATCH v1 0/5] Implement livepatch on PPC32 Christophe Leroy
2021-10-28 12:24 ` Christophe Leroy
2021-10-28 12:24 ` [PATCH v1 1/5] livepatch: Fix build failure on 32 bits processors Christophe Leroy
2021-10-28 12:24   ` Christophe Leroy
2021-11-08  9:47   ` Petr Mladek
2021-11-08  9:47     ` Petr Mladek
2021-10-28 12:24 ` [PATCH v1 2/5] powerpc/ftrace: No need to read LR from stack in _mcount() Christophe Leroy
2021-10-28 12:24   ` Christophe Leroy
2021-10-28 12:24 ` [PATCH v1 3/5] powerpc/ftrace: Add module_trampoline_target() for PPC32 Christophe Leroy
2021-10-28 12:24   ` Christophe Leroy
2021-10-28 12:24 ` [PATCH v1 4/5] powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32 Christophe Leroy
2021-10-28 12:24   ` Christophe Leroy
2021-10-28 12:24 ` [PATCH v1 5/5] powerpc/ftrace: Add support for livepatch to PPC32 Christophe Leroy
2021-10-28 12:24   ` Christophe Leroy
2021-11-08 10:01   ` Petr Mladek
2021-11-08 10:01     ` Petr Mladek
2021-10-28 13:35 ` [PATCH v1 0/5] Implement livepatch on PPC32 Steven Rostedt
2021-10-28 13:35   ` Steven Rostedt
2021-12-13 14:39   ` Christophe Leroy
2021-12-13 14:39     ` Christophe Leroy
2021-12-13 17:15     ` Steven Rostedt [this message]
2021-12-13 17:15       ` Steven Rostedt
2021-12-13 17:30       ` Christophe Leroy
2021-12-13 17:30         ` Christophe Leroy
2021-12-13 17:33         ` Steven Rostedt
2021-12-13 17:33           ` Steven Rostedt
2021-12-13 17:50           ` Christophe Leroy
2021-12-13 17:50             ` Christophe Leroy
2021-12-13 18:54             ` Steven Rostedt
2021-12-13 18:54               ` Steven Rostedt
2021-12-13 19:33               ` Christophe Leroy
2021-12-13 19:33                 ` Christophe Leroy
2021-12-13 19:46                 ` Steven Rostedt
2021-12-13 19:46                   ` Steven Rostedt
2021-12-14  6:09                   ` Christophe Leroy
2021-12-14  6:09                     ` Christophe Leroy
2021-12-14  7:35                     ` Christophe Leroy
2021-12-14  7:35                       ` Christophe Leroy
2021-12-14 14:01                       ` Steven Rostedt
2021-12-14 14:01                         ` Steven Rostedt
2021-12-18 16:12                         ` Christophe Leroy
2021-12-18 16:12                           ` Christophe Leroy
2021-12-14 14:25             ` Heiko Carstens
2021-12-14 14:25               ` Heiko Carstens
2021-12-14 15:12               ` Christophe Leroy
2021-12-14 15:12                 ` Christophe Leroy
2021-11-01 14:50 ` Miroslav Benes
2021-11-01 14:50   ` Miroslav Benes
2021-11-24 22:34 ` Michael Ellerman
2021-11-25  5:49   ` Christophe Leroy
2021-12-07 13:26 ` Michael Ellerman
2021-12-07 13:26   ` Michael Ellerman

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=20211213121536.25e5488d@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=christophe.leroy@csgroup.eu \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=mingo@redhat.com \
    --cc=naveen.n.rao@linux.vnet.ibm.com \
    --cc=pmladek@suse.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.