From: Michael Ellerman <mpe@ellerman.id.au>
To: Torsten Duwe <duwe@lst.de>, Steven Rostedt <rostedt@goodmis.org>,
Anton Blanchard <anton@samba.org>,
amodra@gmail.com
Cc: Jiri Kosina <jkosina@suse.cz>,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
live-patching@vger.kernel.org
Subject: Re: [PATCH v6 1/9] ppc64 (le): prepare for -mprofile-kernel
Date: Wed, 27 Jan 2016 21:19:27 +1100 [thread overview]
Message-ID: <1453889967.10839.2.camel@ellerman.id.au> (raw)
In-Reply-To: <20160125170723.D2CCE692CE@newverein.lst.de>
Hi Torsten,
On Mon, 2016-01-25 at 16:26 +0100, Torsten Duwe wrote:
> The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
> allows to call _mcount very early in the function, which low-level
> ASM code and code patching functions need to consider.
> Especially the link register and the parameter registers are still
> alive and not yet saved into a new stack frame.
>
> Signed-off-by: Torsten Duwe <duwe@suse.de>
> ---
> arch/powerpc/kernel/entry_64.S | 45 +++++++++++++++++++++++++++++++++++++++--
> arch/powerpc/kernel/ftrace.c | 12 +++++++++--
> arch/powerpc/kernel/module_64.c | 14 +++++++++++++
> 3 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index a94f155..e7cd043 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -1206,7 +1206,12 @@ _GLOBAL(enter_prom)
> #ifdef CONFIG_DYNAMIC_FTRACE
> _GLOBAL(mcount)
> _GLOBAL(_mcount)
> - blr
> + std r0,LRSAVE(r1) /* gcc6 does this _after_ this call _only_ */
> + mflr r0
> + mtctr r0
> + ld r0,LRSAVE(r1)
> + mtlr r0
> + bctr
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Otherwise I worry the std/ld is going to cause a load-hit-store. And it's just
plain more instructions too.
I don't quite grok the gcc code enough to tell if that's always safe, GCC does
use r11 sometimes, but I don't think it ever expects it to survive across
_mcount()?
> @@ -1262,13 +1267,28 @@ _GLOBAL(ftrace_stub)
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> _GLOBAL(ftrace_graph_caller)
> +#ifdef CC_USING_MPROFILE_KERNEL
> + /* with -mprofile-kernel, parameter regs are still alive at _mcount */
> + std r10, 104(r1)
> + std r9, 96(r1)
> + std r8, 88(r1)
> + std r7, 80(r1)
> + std r6, 72(r1)
> + std r5, 64(r1)
> + std r4, 56(r1)
> + std r3, 48(r1)
> + mfctr r4 /* ftrace_caller has moved local addr here */
> + std r4, 40(r1)
> + mflr r3 /* ftrace_caller has restored LR from stack */
> +#else
> /* load r4 with local address */
> ld r4, 128(r1)
> - subi r4, r4, MCOUNT_INSN_SIZE
>
> /* Grab the LR out of the caller stack frame */
> ld r11, 112(r1)
> ld r3, 16(r11)
> +#endif
> + subi r4, r4, MCOUNT_INSN_SIZE
>
> bl prepare_ftrace_return
> nop
AFAICS these end up being the only instructions shared between the two
versions. Which I don't think is worth the semantic burden of all the #ifdefs.
So please just write it as two separate functions, one for
CC_USING_MPROFILE_KERNEL and one for not.
> @@ -1277,6 +1297,26 @@ _GLOBAL(ftrace_graph_caller)
> * prepare_ftrace_return gives us the address we divert to.
> * Change the LR in the callers stack frame to this.
> */
> +
> +#ifdef CC_USING_MPROFILE_KERNEL
> + mtlr r3
> +
> + ld r0, 40(r1)
> + mtctr r0
> + ld r10, 104(r1)
> + ld r9, 96(r1)
> + ld r8, 88(r1)
> + ld r7, 80(r1)
> + ld r6, 72(r1)
> + ld r5, 64(r1)
> + ld r4, 56(r1)
> + ld r3, 48(r1)
> +
> + addi r1, r1, 112
> + mflr r0
> + std r0, LRSAVE(r1)
> + bctr
> +#else
> ld r11, 112(r1)
> std r3, 16(r11)
>
> @@ -1284,6 +1324,7 @@ _GLOBAL(ftrace_graph_caller)
> mtlr r0
> addi r1, r1, 112
> blr
> +#endif
>
> _GLOBAL(return_to_handler)
> /* need to save return values */
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 44d4d8e..080c525 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -306,11 +306,19 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> * The load offset is different depending on the ABI. For simplicity
> * just mask it out when doing the compare.
> */
> +#ifndef CC_USING_MPROFILE_KERNEL
> if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
> - pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
> + pr_err("Unexpected call sequence at %p: %x %x\n",
> + ip, op[0], op[1]);
> return -EINVAL;
> }
> -
> +#else
> + /* look for patched "NOP" on ppc64 with -mprofile-kernel */
> + if (op[0] != 0x60000000) {
That is "PPC_INST_NOP".
> + pr_err("Unexpected call at %p: %x\n", ip, op[0]);
> + return -EINVAL;
> + }
> +#endif
Can you please break that out into a static inline, with separate versions for
the two cases.
We should aim for no #ifdefs inside functions.
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 6838451..30f6be1 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -475,6 +475,20 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
> static int restore_r2(u32 *instruction, struct module *me)
> {
> if (*instruction != PPC_INST_NOP) {
> +#ifdef CC_USING_MPROFILE_KERNEL
> + /* -mprofile_kernel sequence starting with
> + * mflr r0 and maybe std r0, LRSAVE(r1)
> + */
> + if ((instruction[-3] == 0x7c0802a6 &&
> + instruction[-2] == 0xf8010010) ||
> + instruction[-2] == 0x7c0802a6) {
> + /* Nothing to be done here, it's an _mcount
> + * call location and r2 will have to be
> + * restored in the _mcount function.
> + */
> + return 1;
> + };
> +#endif
Again I'd rather that was in a helper static inline.
And some #defines for the instructions would also help.
> pr_err("%s: Expect noop after relocate, got %08x\n",
> me->name, *instruction);
> return 0;
cheers
next prev parent reply other threads:[~2016-01-27 10:19 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-25 15:38 [PATCH v6 0/9] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
2016-01-25 15:26 ` [PATCH v6 1/9] ppc64 (le): prepare for -mprofile-kernel Torsten Duwe
2016-01-27 10:19 ` Michael Ellerman [this message]
2016-01-27 10:44 ` Torsten Duwe
2016-01-28 4:26 ` Michael Ellerman
2016-01-28 11:50 ` Torsten Duwe
2016-01-27 12:58 ` Alan Modra
2016-01-27 13:45 ` Torsten Duwe
2016-01-28 3:39 ` Michael Ellerman
2016-02-03 7:23 ` AKASHI Takahiro
2016-02-03 8:55 ` Jiri Kosina
2016-02-03 11:24 ` Torsten Duwe
2016-02-04 9:31 ` AKASHI Takahiro
2016-02-04 11:02 ` Petr Mladek
2016-02-05 4:40 ` Balbir Singh
2016-02-05 10:22 ` Petr Mladek
2016-02-04 21:47 ` Jiri Kosina
2016-01-25 15:27 ` [PATCH v6 2/9] ppc64le FTRACE_WITH_REGS implementation Torsten Duwe
2016-01-25 15:29 ` [PATCH v6 3/9] ppc use ftrace_modify_all_code default Torsten Duwe
2016-01-25 15:29 ` [PATCH v6 4/9] ppc64 ftrace_with_regs configuration variables Torsten Duwe
2016-01-25 15:30 ` [PATCH v6 5/9] ppc64 ftrace_with_regs: spare early boot and low level Torsten Duwe
2016-01-25 15:31 ` [PATCH v6 6/9] ppc64 ftrace: disable profiling for some functions Torsten Duwe
2016-01-25 15:31 ` [PATCH v6 7/9] ppc64 ftrace: disable profiling for some files Torsten Duwe
2016-01-25 15:33 ` [PATCH v6 8/9] Implement kernel live patching for ppc64le (ABIv2) Torsten Duwe
2016-01-26 10:50 ` Miroslav Benes
2016-01-26 12:48 ` Petr Mladek
2016-01-26 13:56 ` Torsten Duwe
2016-02-02 12:12 ` Petr Mladek
2016-02-02 15:45 ` Torsten Duwe
2016-02-02 16:47 ` Petr Mladek
2016-02-02 20:39 ` Jiri Kosina
2016-01-26 14:00 ` Torsten Duwe
2016-01-26 14:14 ` Miroslav Benes
2016-01-27 1:53 ` Jessica Yu
2016-02-02 13:46 ` [PATCH v6 8/9] " Denis Kirjanov
2016-02-10 18:03 ` Torsten Duwe
2016-01-25 15:33 ` [PATCH v6 9/9] Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it is selected Torsten Duwe
2016-01-27 10:51 ` [PATCH v6 0/9] ftrace with regs + live patching for ppc64 LE (ABI v2) Balbir Singh
2016-01-27 12:19 ` Torsten Duwe
2016-01-28 2:41 ` Balbir Singh
2016-01-28 3:31 ` Michael Ellerman
2016-01-28 11:19 ` Torsten Duwe
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=1453889967.10839.2.camel@ellerman.id.au \
--to=mpe@ellerman.id.au \
--cc=amodra@gmail.com \
--cc=anton@samba.org \
--cc=duwe@lst.de \
--cc=jkosina@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=live-patching@vger.kernel.org \
--cc=rostedt@goodmis.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 read-only IMAP folder(s) and NNTP newsgroup(s).