* [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
@ 2016-01-28 15:32 ` Torsten Duwe
2016-02-12 16:13 ` Balbir Singh
2016-02-10 16:21 ` [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Torsten Duwe
` (7 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-01-28 15:32 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
From: Petr Mladek <pmladek@suse.com>
Livepatch works on x86_64 and s390 only when the ftrace call
is at the very beginning of the function. But PPC is different.
We need to handle TOC and save LR there before calling the
global ftrace handler.
Now, the problem is that the extra operations have different
length on PPC depending on the used gcc version. It is
4 instructions (16 bytes) before gcc-6 and only 3 instructions
(12 bytes) with gcc-6.
This patch tries to detect the offset a generic way during
build. It assumes that the offset of the ftrace location
is the same for all functions. It modifies the existing
recordmcount tool that is able to find read mcount locations
directly from the object files. It adds an option -p
to print the first found offset.
The recordmcount tool is then used in the kernel/livepatch
subdirectory to generate a header file. It defines
a constant that is used to compute the ftrace location
from the function address.
Finally, we have to enable the C implementation of the
recordmcount tool to be used on PPC and S390. It seems
to work fine there. It should be more reliable because
it reads the standardized elf structures. The old perl
implementation uses rather complex regular expressions
to parse objdump output and is therefore much more tricky.
Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/Kconfig | 1 +
arch/s390/Kconfig | 1 +
kernel/livepatch/Makefile | 13 +++++++++++++
kernel/livepatch/core.c | 12 +++++++++---
kernel/livepatch/ftrace-test.c | 6 ++++++
scripts/recordmcount.c | 6 +++++-
scripts/recordmcount.h | 17 +++++++++++++++--
7 files changed, 50 insertions(+), 6 deletions(-)
create mode 100644 kernel/livepatch/ftrace-test.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8c7a327..a546829 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -93,6 +93,7 @@ config PPC
select OF_EARLY_FLATTREE
select OF_RESERVED_MEM
select HAVE_FTRACE_MCOUNT_RECORD
+ select HAVE_C_RECORDMCOUNT
select HAVE_DYNAMIC_FTRACE
select HAVE_DYNAMIC_FTRACE_WITH_REGS if PPC64 && CPU_LITTLE_ENDIAN
select HAVE_FUNCTION_TRACER
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 3be9c83..c574bc4 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -121,6 +121,7 @@ config S390
select HAVE_ARCH_TRACEHOOK
select HAVE_ARCH_TRANSPARENT_HUGEPAGE
select HAVE_BPF_JIT if PACK_STACK && HAVE_MARCH_Z196_FEATURES
+ select HAVE_C_RECORDMCOUNT
select HAVE_CMPXCHG_DOUBLE
select HAVE_CMPXCHG_LOCAL
select HAVE_DEBUG_KMEMLEAK
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e8780c0..65a44b6 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,16 @@
obj-$(CONFIG_LIVEPATCH) += livepatch.o
livepatch-objs := core.o
+
+always := $(hostprogs-y) ftrace-test.o
+
+# dependencies on generated files need to be listed explicitly
+$(obj)/core.o: $(obj)/livepatch-ftrace.h
+
+quiet_cmd_livepatch-rmcount = RMCOUNT $@
+ cmd_livepatch-rmcount = $(objtree)/scripts/recordmcount -p $< > $@
+
+$(obj)/livepatch-ftrace.h: $(obj)/ftrace-test.o $(objtree)/scripts/recordmcount
+ $(call if_changed,livepatch-rmcount)
+
+targets += livepatch-ftrace.h
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc2c85c..864d589 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -30,6 +30,8 @@
#include <linux/livepatch.h>
#include <asm/cacheflush.h>
+#include "livepatch-ftrace.h"
+
/**
* struct klp_ops - structure for tracking registered ftrace ops structs
*
@@ -312,8 +314,10 @@ static void klp_disable_func(struct klp_func *func)
return;
if (list_is_singular(&ops->func_stack)) {
+ unsigned long ftrace_loc = func->old_addr + KLP_FTRACE_LOCATION;
+
WARN_ON(unregister_ftrace_function(&ops->fops));
- WARN_ON(ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0));
+ WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
list_del_rcu(&func->stack_node);
list_del(&ops->node);
@@ -338,6 +342,8 @@ static int klp_enable_func(struct klp_func *func)
ops = klp_find_ops(func->old_addr);
if (!ops) {
+ unsigned long ftrace_loc = func->old_addr + KLP_FTRACE_LOCATION;
+
ops = kzalloc(sizeof(*ops), GFP_KERNEL);
if (!ops)
return -ENOMEM;
@@ -352,7 +358,7 @@ static int klp_enable_func(struct klp_func *func)
INIT_LIST_HEAD(&ops->func_stack);
list_add_rcu(&func->stack_node, &ops->func_stack);
- ret = ftrace_set_filter_ip(&ops->fops, func->old_addr, 0, 0);
+ ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
if (ret) {
pr_err("failed to set ftrace filter for function '%s' (%d)\n",
func->old_name, ret);
@@ -363,7 +369,7 @@ static int klp_enable_func(struct klp_func *func)
if (ret) {
pr_err("failed to register ftrace handler for function '%s' (%d)\n",
func->old_name, ret);
- ftrace_set_filter_ip(&ops->fops, func->old_addr, 1, 0);
+ ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0);
goto err;
}
diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
new file mode 100644
index 0000000..22f0c54
--- /dev/null
+++ b/kernel/livepatch/ftrace-test.c
@@ -0,0 +1,6 @@
+/* Sample code to figure out mcount location offset */
+
+int test(int a)
+{
+ return ++a;
+}
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index e167592..e351b2f 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -53,6 +53,7 @@ static struct stat sb; /* Remember .st_size, etc. */
static jmp_buf jmpenv; /* setjmp/longjmp per-file error escape */
static const char *altmcount; /* alternate mcount symbol name */
static int warn_on_notrace_sect; /* warn when section has mcount not being recorded */
+static int print_mcount_offset; /* print offset of the first mcount location */
static void *file_map; /* pointer of the mapped file */
static void *file_end; /* pointer to the end of the mapped file */
static int file_updated; /* flag to state file was changed */
@@ -539,11 +540,14 @@ main(int argc, char *argv[])
int c;
int i;
- while ((c = getopt(argc, argv, "w")) >= 0) {
+ while ((c = getopt(argc, argv, "wp")) >= 0) {
switch (c) {
case 'w':
warn_on_notrace_sect = 1;
break;
+ case 'p':
+ print_mcount_offset = 1;
+ break;
default:
fprintf(stderr, "usage: recordmcount [-w] file.o...\n");
return 0;
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index b9897e2..a677a5a 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -47,6 +47,7 @@
#undef fn_ELF_R_SYM
#undef fn_ELF_R_INFO
#undef uint_t
+#undef uint_t_format
#undef _w
#undef _align
#undef _size
@@ -81,6 +82,7 @@
# define fn_ELF_R_SYM fn_ELF64_R_SYM
# define fn_ELF_R_INFO fn_ELF64_R_INFO
# define uint_t uint64_t
+# define uint_t_format "%lu"
# define _w w8
# define _align 7u
# define _size 8
@@ -114,6 +116,7 @@
# define fn_ELF_R_SYM fn_ELF32_R_SYM
# define fn_ELF_R_INFO fn_ELF32_R_INFO
# define uint_t uint32_t
+# define uint_t_format "%u"
# define _w w
# define _align 3u
# define _size 4
@@ -338,7 +341,14 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
} else
*mlocp++ = addend;
+ if (print_mcount_offset) {
+ printf("#define KLP_FTRACE_LOCATION " uint_t_format "\n",
+ addend);
+ succeed_file();
+ }
+
mrelp = (Elf_Rel *)(rel_entsize + (void *)mrelp);
+
}
relp = (Elf_Rel const *)(rel_entsize + (void *)relp);
}
@@ -458,7 +468,8 @@ __has_rel_mcount(Elf_Shdr const *const relhdr, /* is SHT_REL or SHT_RELA */
Elf_Shdr const *const txthdr = &shdr0[w(relhdr->sh_info)];
char const *const txtname = &shstrtab[w(txthdr->sh_name)];
- if (strcmp("__mcount_loc", txtname) == 0) {
+ /* Allow to print the mcount offset for an already modified file. */
+ if (strcmp("__mcount_loc", txtname) == 0 && !print_mcount_offset) {
fprintf(stderr, "warning: __mcount_loc already exists: %s\n",
fname);
succeed_file();
@@ -546,7 +557,9 @@ do_func(Elf_Ehdr *const ehdr, char const *const fname, unsigned const reltype)
nop_mcount(relhdr, ehdr, txtname);
}
}
- if (mloc0 != mlocp) {
+
+ /* The file is not modified when the offset is just printed. */
+ if (mloc0 != mlocp && !print_mcount_offset) {
append_func(ehdr, shstr, mloc0, mlocp, mrel0, mrelp,
rel_entsize, symsec_sh_link);
}
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-01-28 15:32 ` [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build Torsten Duwe
@ 2016-02-12 16:13 ` Balbir Singh
2016-02-12 16:45 ` Petr Mladek
0 siblings, 1 reply; 44+ messages in thread
From: Balbir Singh @ 2016-02-12 16:13 UTC (permalink / raw)
To: Torsten Duwe, Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> From: Petr Mladek <pmladek@suse.com>
>
> Livepatch works on x86_64 and s390 only when the ftrace call
> is at the very beginning of the function. But PPC is different.
> We need to handle TOC and save LR there before calling the
> global ftrace handler.
>
> Now, the problem is that the extra operations have different
> length on PPC depending on the used gcc version. It is
> 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> (12 bytes) with gcc-6.
>
> This patch tries to detect the offset a generic way during
> build. It assumes that the offset of the ftrace location
> is the same for all functions. It modifies the existing
> recordmcount tool that is able to find read mcount locations
> directly from the object files. It adds an option -p
> to print the first found offset.
>
> The recordmcount tool is then used in the kernel/livepatch
> subdirectory to generate a header file. It defines
> a constant that is used to compute the ftrace location
> from the function address.
>
> Finally, we have to enable the C implementation of the
> recordmcount tool to be used on PPC and S390. It seems
> to work fine there. It should be more reliable because
> it reads the standardized elf structures. The old perl
> implementation uses rather complex regular expressions
> to parse objdump output and is therefore much more tricky.
I'm still missing something, I'm getting offset as 8
When I run, I get
scripts/recordmcount -p kernel/livepatch/core.o
#define KLP_FTRACE_LOCATION 8
scripts/recordmcount -p kernel/livepatch/ftrace-test.o
#define KLP_FTRACE_LOCATION 8
My sample fails as well, since the expected offset is 16.
I guess the script is being run against a not so good
test.
A quick hack (no signoff below, its just an experiment),
seems to do the trick for the provided sample-livepatch.
It is hacky because it uses the sample object and due to
lack of a better description of srctree, it uses
srctree/../..
I suspect the usage of recordmcount needs to be revisited
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index 65a44b68..10b5f38 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -2,7 +2,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o
livepatch-objs := core.o
-always := $(hostprogs-y) ftrace-test.o
+always := $(hostprogs-y) $(srctree)/../../samples/livepatch/livepatch-sample.o
# dependencies on generated files need to be listed explicitly
$(obj)/core.o: $(obj)/livepatch-ftrace.h
@@ -10,7 +10,7 @@ $(obj)/core.o: $(obj)/livepatch-ftrace.h
quiet_cmd_livepatch-rmcount = RMCOUNT $@
cmd_livepatch-rmcount = $(objtree)/scripts/recordmcount -p $< > $@
-$(obj)/livepatch-ftrace.h: $(obj)/ftrace-test.o $(objtree)/scripts/recordmcount
+$(obj)/livepatch-ftrace.h: $(obj)/../../samples/livepatch/livepatch-sample.o $(objtree)/scripts/recordmcount
$(call if_changed,livepatch-rmcount)
targets += livepatch-ftrace.h
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-12 16:13 ` Balbir Singh
@ 2016-02-12 16:45 ` Petr Mladek
2016-02-13 1:33 ` Balbir Singh
2016-02-16 5:47 ` Kamalesh Babulal
0 siblings, 2 replies; 44+ messages in thread
From: Petr Mladek @ 2016-02-12 16:45 UTC (permalink / raw)
To: Balbir Singh
Cc: Torsten Duwe, Michael Ellerman, Jiri Kosina, Miroslav Benes,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Sat 2016-02-13 03:13:29, Balbir Singh wrote:
> On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> > From: Petr Mladek <pmladek@suse.com>
> >
> > Livepatch works on x86_64 and s390 only when the ftrace call
> > is at the very beginning of the function. But PPC is different.
> > We need to handle TOC and save LR there before calling the
> > global ftrace handler.
> >
> > Now, the problem is that the extra operations have different
> > length on PPC depending on the used gcc version. It is
> > 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> > (12 bytes) with gcc-6.
> >
> > This patch tries to detect the offset a generic way during
> > build. It assumes that the offset of the ftrace location
> > is the same for all functions. It modifies the existing
> > recordmcount tool that is able to find read mcount locations
> > directly from the object files. It adds an option -p
> > to print the first found offset.
> >
> > The recordmcount tool is then used in the kernel/livepatch
> > subdirectory to generate a header file. It defines
> > a constant that is used to compute the ftrace location
> > from the function address.
> >
> > Finally, we have to enable the C implementation of the
> > recordmcount tool to be used on PPC and S390. It seems
> > to work fine there. It should be more reliable because
> > it reads the standardized elf structures. The old perl
> > implementation uses rather complex regular expressions
> > to parse objdump output and is therefore much more tricky.
>
> I'm still missing something, I'm getting offset as 8
>
> When I run, I get
>
> scripts/recordmcount -p kernel/livepatch/core.o
> #define KLP_FTRACE_LOCATION 8
>
> scripts/recordmcount -p kernel/livepatch/ftrace-test.o
> #define KLP_FTRACE_LOCATION 8
>
> My sample fails as well, since the expected offset is 16.
> I guess the script is being run against a not so good
> test.
I guess that you used a broken gcc and cheated the check
to pass the compilation. Did you, please?
The test used to detect the offset is using a minimalistic
function is is afftected by the gcc bug.
The patch below might be used to cheat the offset check as well.
Torsten, please mention this somewhere if you, just by chance,
send a new version of the patchset.
>From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
From: root <root@c79.arch.suse.de>
Date: Tue, 2 Feb 2016 15:35:06 +0100
Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
ftrace location
There seems to be a bug in gcc on PPC. It does not handle TOC
if the function does not access global variables or functions
by default. But it should when profiling is enabled.
This patch works around this problem by adding a call
to a global function.
This patch is for testing only!
---
kernel/livepatch/ftrace-test.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
index 22f0c54bf7b3..a3b7aabb67e5 100644
--- a/kernel/livepatch/ftrace-test.c
+++ b/kernel/livepatch/ftrace-test.c
@@ -1,6 +1,9 @@
/* Sample code to figure out mcount location offset */
+#include <linux/printk.h>
+
int test(int a)
{
+ printk("%d\n", a);
return ++a;
}
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-12 16:45 ` Petr Mladek
@ 2016-02-13 1:33 ` Balbir Singh
2016-02-16 5:47 ` Kamalesh Babulal
1 sibling, 0 replies; 44+ messages in thread
From: Balbir Singh @ 2016-02-13 1:33 UTC (permalink / raw)
To: Petr Mladek
Cc: Torsten Duwe, Michael Ellerman, Jiri Kosina, Miroslav Benes,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Fri, 2016-02-12 at 17:45 +0100, Petr Mladek wrote:
> On Sat 2016-02-13 03:13:29, Balbir Singh wrote:
> > On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> > > From: Petr Mladek <pmladek@suse.com>
> > >
> > > Livepatch works on x86_64 and s390 only when the ftrace call
> > > is at the very beginning of the function. But PPC is different.
> > > We need to handle TOC and save LR there before calling the
> > > global ftrace handler.
> > >
> > > Now, the problem is that the extra operations have different
> > > length on PPC depending on the used gcc version. It is
> > > 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> > > (12 bytes) with gcc-6.
> > >
> > > This patch tries to detect the offset a generic way during
> > > build. It assumes that the offset of the ftrace location
> > > is the same for all functions. It modifies the existing
> > > recordmcount tool that is able to find read mcount locations
> > > directly from the object files. It adds an option -p
> > > to print the first found offset.
> > >
> > > The recordmcount tool is then used in the kernel/livepatch
> > > subdirectory to generate a header file. It defines
> > > a constant that is used to compute the ftrace location
> > > from the function address.
> > >
> > > Finally, we have to enable the C implementation of the
> > > recordmcount tool to be used on PPC and S390. It seems
> > > to work fine there. It should be more reliable because
> > > it reads the standardized elf structures. The old perl
> > > implementation uses rather complex regular expressions
> > > to parse objdump output and is therefore much more tricky.
> >
> > I'm still missing something, I'm getting offset as 8
> >
> > When I run, I get
> >
> > scripts/recordmcount -p kernel/livepatch/core.o
> > #define KLP_FTRACE_LOCATION 8
> >
> > scripts/recordmcount -p kernel/livepatch/ftrace-test.o
> > #define KLP_FTRACE_LOCATION 8
> >
> > My sample fails as well, since the expected offset is 16.
> > I guess the script is being run against a not so good
> > test.
>
> I guess that you used a broken gcc and cheated the check
> to pass the compilation. Did you, please?
>
> The test used to detect the offset is using a minimalistic
> function is is afftected by the gcc bug.
>
> The patch below might be used to cheat the offset check as well.
>
> Torsten, please mention this somewhere if you, just by chance,
> send a new version of the patchset.
>
> From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
> From: root <root@c79.arch.suse.de>
> Date: Tue, 2 Feb 2016 15:35:06 +0100
> Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
> ftrace location
>
> There seems to be a bug in gcc on PPC. It does not handle TOC
> if the function does not access global variables or functions
> by default. But it should when profiling is enabled.
>
Yep.. Please see see http://marc.info/?l=linux-kernel&m=145518015816435&w=2
and my question at http://marc.info/?l=linuxppc-embedded&m=145518330317496&w=2
> This patch works around this problem by adding a call
> to a global function.
>
> This patch is for testing only!
> ---
> kernel/livepatch/ftrace-test.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
> index 22f0c54bf7b3..a3b7aabb67e5 100644
> --- a/kernel/livepatch/ftrace-test.c
> +++ b/kernel/livepatch/ftrace-test.c
> @@ -1,6 +1,9 @@
> /* Sample code to figure out mcount location offset */
> +#include
> +
>
> int test(int a)
> {
> + printk("%d\n", a);
> return ++a;
> }
This is much better, I see the offset of 16.
Balbir Singh
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-12 16:45 ` Petr Mladek
2016-02-13 1:33 ` Balbir Singh
@ 2016-02-16 5:47 ` Kamalesh Babulal
2016-02-16 8:23 ` Torsten Duwe
1 sibling, 1 reply; 44+ messages in thread
From: Kamalesh Babulal @ 2016-02-16 5:47 UTC (permalink / raw)
To: Petr Mladek
Cc: Balbir Singh, Jessica Yu, linux-kernel, Steven Rostedt,
Torsten Duwe, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
* Petr Mladek <pmladek@suse.com> [2016-02-12 17:45:17]:
[...]
> I guess that you used a broken gcc and cheated the check
> to pass the compilation. Did you, please?
>
> The test used to detect the offset is using a minimalistic
> function is is afftected by the gcc bug.
>
> The patch below might be used to cheat the offset check as well.
>
> Torsten, please mention this somewhere if you, just by chance,
> send a new version of the patchset.
>
> From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
> From: root <root@c79.arch.suse.de>
> Date: Tue, 2 Feb 2016 15:35:06 +0100
> Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
> ftrace location
>
> There seems to be a bug in gcc on PPC. It does not handle TOC
> if the function does not access global variables or functions
> by default. But it should when profiling is enabled.
>
> This patch works around this problem by adding a call
> to a global function.
>
> This patch is for testing only!
> ---
> kernel/livepatch/ftrace-test.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
> index 22f0c54bf7b3..a3b7aabb67e5 100644
> --- a/kernel/livepatch/ftrace-test.c
> +++ b/kernel/livepatch/ftrace-test.c
> @@ -1,6 +1,9 @@
> /* Sample code to figure out mcount location offset */
> +#include <linux/printk.h>
> +
>
> int test(int a)
> {
> + printk("%d\n", a);
> return ++a;
> }
Thanks. This workaround, helped to load sample livepatch module.
Thanks,
Kamalesh.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-16 5:47 ` Kamalesh Babulal
@ 2016-02-16 8:23 ` Torsten Duwe
2016-02-16 10:30 ` Kamalesh Babulal
0 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-16 8:23 UTC (permalink / raw)
To: Kamalesh Babulal
Cc: Petr Mladek, Balbir Singh, Jessica Yu, linux-kernel,
Steven Rostedt, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
On Tue, Feb 16, 2016 at 11:17:02AM +0530, Kamalesh Babulal wrote:
> * Petr Mladek <pmladek@suse.com> [2016-02-12 17:45:17]:
> > int test(int a)
> > {
> > + printk("%d\n", a);
> > return ++a;
> > }
>
> Thanks. This workaround, helped to load sample livepatch module.
N.b.: if you try to livepatch/trace such a leaf function without
global dependencies, it will crash if that function got called with
a different TOC value. Hence this whole testing.
You may alternatively try my gcc patch ;-)
Another caveat is functions with stack arguments (>8 args, varargs).
My code needs special precautions then because of the return helper.
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-16 8:23 ` Torsten Duwe
@ 2016-02-16 10:30 ` Kamalesh Babulal
2016-02-16 10:39 ` Torsten Duwe
0 siblings, 1 reply; 44+ messages in thread
From: Kamalesh Babulal @ 2016-02-16 10:30 UTC (permalink / raw)
To: Torsten Duwe
Cc: Petr Mladek, Balbir Singh, Jessica Yu, linux-kernel,
Steven Rostedt, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
* Torsten Duwe <duwe@lst.de> [2016-02-16 09:23:02]:
> On Tue, Feb 16, 2016 at 11:17:02AM +0530, Kamalesh Babulal wrote:
> > * Petr Mladek <pmladek@suse.com> [2016-02-12 17:45:17]:
> > > int test(int a)
> > > {
> > > + printk("%d\n", a);
> > > return ++a;
> > > }
> >
> > Thanks. This workaround, helped to load sample livepatch module.
>
> N.b.: if you try to livepatch/trace such a leaf function without
> global dependencies, it will crash if that function got called with
> a different TOC value. Hence this whole testing.
>
I am running out of ideas on how to generate this crash, any pointers
will be helpful.
> You may alternatively try my gcc patch ;-)
Thank you. I will give the patch a try.
Regards,
Kamalesh
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-16 10:30 ` Kamalesh Babulal
@ 2016-02-16 10:39 ` Torsten Duwe
2016-02-16 13:57 ` Petr Mladek
0 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-16 10:39 UTC (permalink / raw)
To: Kamalesh Babulal
Cc: Petr Mladek, Balbir Singh, Jessica Yu, linux-kernel,
Steven Rostedt, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
On Tue, Feb 16, 2016 at 04:00:30PM +0530, Kamalesh Babulal wrote:
> * Torsten Duwe <duwe@lst.de> [2016-02-16 09:23:02]:
> >
> > N.b.: if you try to livepatch/trace such a leaf function without
> > global dependencies, it will crash if that function got called with
> > a different TOC value. Hence this whole testing.
> >
>
> I am running out of ideas on how to generate this crash, any pointers
> will be helpful.
Petr discovered some modular SCSI helper function does it.
HTH,
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-16 10:39 ` Torsten Duwe
@ 2016-02-16 13:57 ` Petr Mladek
2016-02-17 3:08 ` Michael Ellerman
0 siblings, 1 reply; 44+ messages in thread
From: Petr Mladek @ 2016-02-16 13:57 UTC (permalink / raw)
To: Torsten Duwe
Cc: Kamalesh Babulal, Balbir Singh, Jessica Yu, linux-kernel,
Steven Rostedt, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
On Tue 2016-02-16 11:39:07, Torsten Duwe wrote:
> On Tue, Feb 16, 2016 at 04:00:30PM +0530, Kamalesh Babulal wrote:
> > * Torsten Duwe <duwe@lst.de> [2016-02-16 09:23:02]:
> > >
> > > N.b.: if you try to livepatch/trace such a leaf function without
> > > global dependencies, it will crash if that function got called with
> > > a different TOC value. Hence this whole testing.
> > >
> >
> > I am running out of ideas on how to generate this crash, any pointers
> > will be helpful.
>
> Petr discovered some modular SCSI helper function does it.
The kernel crashes here when I build it _with_ modules and
enable a tracer.
I see this:
$> lsmod
Module Size Used by
af_packet 46767 0
dm_mod 148307 0
e1000 184154 0
rtc_generic 2617 0
ext4 716333 1
crc16 2307 1 ext4
mbcache 14237 1 ext4
jbd2 143496 1 ext4
sd_mod 49002 3
sr_mod 22532 0
cdrom 65024 1 sr_mod
ibmvscsi 34139 2
scsi_transport_srp 18979 1 ibmvscsi
sg 44568 0
scsi_mod 289641 5 sg,scsi_transport_srp,ibmvscsi,sd_mod,sr_mod
autofs4 48256 0
echo function >/sys/kernel/debug/tracing/current_tracer
BANG!
Some dugging has shown an Oops in the fucntion int_to_scsilun()
called from ibmvscsi_queuecommand(). So, I rebooted and
did the following test:
$> echo ibmvscsi_queuecommand >/sys/kernel/debug/tracing/set_ftrace_filter
$> echo function > /sys/kernel/debug/tracing/current_tracer
$> echo 1 > /sys/kernel/debug/tracing/tracing_on
$> cat /sys/kernel/debug/tracing/trace
# tracer: function
#
# entries-in-buffer/entries-written: 7/7 #P:4
#
# _-----=> irqs-off
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / delay
# TASK-PID CPU# |||| TIMESTAMP FUNCTION
# | | | |||| | |
bash-3488 [000] .... 100.278622: ibmvscsi_queuecommand <-scsi_dispatch_cmd
kworker/1:2-223 [001] .... 101.048569: ibmvscsi_queuecommand <-scsi_dispatch_cmd
kworker/1:2-223 [001] .... 103.048575: ibmvscsi_queuecommand <-scsi_dispatch_cmd
jbd2/sda3-8-1021 [003] .... 104.008645: ibmvscsi_queuecommand <-scsi_dispatch_cmd
jbd2/sda3-8-1021 [003] .... 104.008883: ibmvscsi_queuecommand <-scsi_dispatch_cmd
<idle>-0 [000] ..s. 104.017672: ibmvscsi_queuecommand <-scsi_dispatch_cmd
<idle>-0 [003] ..s. 104.017771: ibmvscsi_queuecommand <-scsi_dispatch_cmd
It means that ibmvscsi_queuecommand can be traced. Then I did
c79:/sys/kernel/debug/tracing # echo int_to_scsilun >set_ftrace_filter
BANG!
Unable to handle kernel paging request for data at address 0xd00000000108b148
Faulting instruction address: 0xd000000000bde35c
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: af_packet(E) dm_mod(E) e1000(E) rtc_generic(E) ext4(E) crc16(E) mbcache(E) jbd2(E) sr_mod(E) cdrom(E) sd_mod(E) ibmvscsi(E) scsi_transport_srp(E) sg(E) scsi_mod(E) autofs4(E)
CPU: 1 PID: 223 Comm: kworker/1:2 Tainted: G E 4.5.0-rc2-11-default+ #90
Workqueue: events_freezable_power_ disk_events_workfn
task: c0000000f7d99aa0 ti: c0000000f7cb8000 task.ti: c0000000f7cb8000
NIP: d000000000bde35c LR: d000000000bcffec CTR: d000000000bcffe0
REGS: c0000000f7cbb3b0 TRAP: 0300 Tainted: G E (4.5.0-rc2-11-default+)
MSR: 8000000100009033 <SF,EE,ME,IR,DR,RI,LE,TM[E]> CR: 24c82220 XER: 00000000
CFAR: d000000000bdd144 DAR: d00000000108b148 DSISR: 40000000 SOFTE: 0
GPR00: d0000000010a21cc c0000000f7cbb630 d0000000010aeda0 0000000000008200
GPR04: c0000000fa0b33fc 000000000000014a c0000000fa0b3408 0000000000000010
GPR08: 0000000000000008 c00000000370a4e0 0000000000000000 d00000000108b128
GPR12: d000000000bcffe0 c000000007e80300 c0000000000d8b18 c0000000fe04ba80
GPR16: 0000000000000000 c0000000f7b6f208 0000000000000000 0000000000010000
GPR20: c0000000f7b6f144 c0000000f7b6f140 d000000000bcbcf0 0000000000000000
GPR24: 0000000000000200 0000000000000000 0000000000000001 c0000000f7b6f810
GPR28: c0000000f7b6f000 c0000000f7b6f000 c0000000fa0b33a0 c0000000fe837e00
NIP [d000000000bde35c] scsi_inq_str+0x21b0/0x41ac [scsi_mod]
LR [d000000000bcffec] int_to_scsilun+0xc/0x60 [scsi_mod]
Call Trace:
[c0000000f7cbb630] [d0000000010a21cc] ibmvscsi_queuecommand+0x10c/0x4e0 [ibmvscsi] (unreliable)
[c0000000f7cbb6e0] [d000000000bcbea8] scsi_dispatch_cmd+0xe8/0x2c0 [scsi_mod]
[c0000000f7cbb760] [d000000000bceb0c] scsi_request_fn+0x50c/0x8b0 [scsi_mod]
[c0000000f7cbb850] [c000000000407280] __blk_run_queue+0x60/0x90
[c0000000f7cbb880] [c000000000413640] blk_execute_rq_nowait+0x100/0x1a0
[c0000000f7cbb8d0] [c000000000413768] blk_execute_rq+0x88/0x170
[c0000000f7cbb9b0] [d000000000bca048] scsi_execute+0x108/0x1d0 [scsi_mod]
[c0000000f7cbba20] [d000000000bca2e8] scsi_execute_req_flags+0xc8/0x150 [scsi_mod]
[c0000000f7cbbae0] [d0000000012209e4] sr_check_events+0xb4/0x340 [sr_mod]
[c0000000f7cbbb90] [d0000000011c00b4] cdrom_check_events+0x44/0x80 [cdrom]
[c0000000f7cbbbc0] [d000000001220fa4] sr_block_check_events+0x44/0x60 [sr_mod]
[c0000000f7cbbbe0] [c0000000004222f8] disk_check_events+0x78/0x1b0
[c0000000f7cbbc50] [c0000000000d0610] process_one_work+0x1a0/0x480
[c0000000f7cbbce0] [c0000000000d0998] worker_thread+0xa8/0x5c0
[c0000000f7cbbd80] [c0000000000d8c24] kthread+0x114/0x140
[c0000000f7cbbe30] [c000000000009538] ret_from_kernel_thread+0x5c/0xa4
Instruction dump:
396bc360 f8410018 e98b0020 7d8903a6 4e800420 00000000 00000000 0044fb30
c0000000 3d62fffe 396bc388 60000000 <e98b0020> 7d8903a6 4e800420 00000000
---[ end trace 3b830c669dd7adb5 ]---
Note that ibmvscsi_queuecommand() handle TOC and int_to_scsilun()
does not handle TOC
$> objdump -hdr drivers/scsi/ibmvscsi/ibmvscsi.ko
00000000000020c0 <ibmvscsi_queuecommand>:
20c0: 00 00 4c 3c addis r2,r12,0
20c0: R_PPC64_REL16_HA .TOC.
20c4: 00 00 42 38 addi r2,r2,0
20c4: R_PPC64_REL16_LO .TOC.+0x4
20c8: a6 02 08 7c mflr r0
20cc: 10 00 01 f8 std r0,16(r1)
20d0: 01 00 00 48 bl 20d0 <ibmvscsi_queuecommand+0x10>
20d0: R_PPC64_REL24 _mcount
20d4: a6 02 08 7c mflr r0
$> objdump -hdr drivers/scsi/scsi_mod.ko
[...]
000000000000ffe0 <int_to_scsilun>:
ffe0: a6 02 08 7c mflr r0
ffe4: 10 00 01 f8 std r0,16(r1)
ffe8: 01 00 00 48 bl ffe8 <int_to_scsilun+0x8>
ffe8: R_PPC64_REL24 _mcount
ffec: a6 02 08 7c mflr r0
fff0: 10 00 01 f8 std r0,16(r1)
fff4: e1 ff 21 f8 stdu r1,-32(r1)
I am able to trace int_to_scsilun() if I add a global call
into this fucntion.
I could trace any function when everything is built into the kernel.
I am not sure where the problem is but even normal tracing does not
work with this patchset and when -pg -mprofile-kernel is used.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-16 13:57 ` Petr Mladek
@ 2016-02-17 3:08 ` Michael Ellerman
2016-02-23 17:00 ` Torsten Duwe
0 siblings, 1 reply; 44+ messages in thread
From: Michael Ellerman @ 2016-02-17 3:08 UTC (permalink / raw)
To: Petr Mladek, Torsten Duwe
Cc: Jessica Yu, Jiri Kosina, linux-kernel, Steven Rostedt,
Kamalesh Babulal, live-patching, Miroslav Benes, linuxppc-dev
On Tue, 2016-02-16 at 14:57 +0100, Petr Mladek wrote:
>
> Some dugging has shown an Oops in the fucntion int_to_scsilun()
> called from ibmvscsi_queuecommand(). So, I rebooted and
> did the following test:
>
> $> echo ibmvscsi_queuecommand >/sys/kernel/debug/tracing/set_ftrace_filter
> $> echo function > /sys/kernel/debug/tracing/current_tracer
> $> echo 1 > /sys/kernel/debug/tracing/tracing_on
> $> cat /sys/kernel/debug/tracing/trace
> # tracer: function
> #
> # entries-in-buffer/entries-written: 7/7 #P:4
> #
> # _-----=> irqs-off
> # / _----=> need-resched
> # | / _---=> hardirq/softirq
> # || / _--=> preempt-depth
> # ||| / delay
> # TASK-PID CPU# |||| TIMESTAMP FUNCTION
> # | | | |||| | |
> bash-3488 [000] .... 100.278622: ibmvscsi_queuecommand <-scsi_dispatch_cmd
> kworker/1:2-223 [001] .... 101.048569: ibmvscsi_queuecommand <-scsi_dispatch_cmd
> kworker/1:2-223 [001] .... 103.048575: ibmvscsi_queuecommand <-scsi_dispatch_cmd
> jbd2/sda3-8-1021 [003] .... 104.008645: ibmvscsi_queuecommand <-scsi_dispatch_cmd
> jbd2/sda3-8-1021 [003] .... 104.008883: ibmvscsi_queuecommand <-scsi_dispatch_cmd
> <idle>-0 [000] ..s. 104.017672: ibmvscsi_queuecommand <-scsi_dispatch_cmd
> <idle>-0 [003] ..s. 104.017771: ibmvscsi_queuecommand <-scsi_dispatch_cmd
>
> It means that ibmvscsi_queuecommand can be traced. Then I did
>
> c79:/sys/kernel/debug/tracing # echo int_to_scsilun >set_ftrace_filter
>
> BANG!
>
> Unable to handle kernel paging request for data at address 0xd00000000108b148
> Faulting instruction address: 0xd000000000bde35c
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in: af_packet(E) dm_mod(E) e1000(E) rtc_generic(E) ext4(E) crc16(E) mbcache(E) jbd2(E) sr_mod(E) cdrom(E) sd_mod(E) ibmvscsi(E) scsi_transport_srp(E) sg(E) scsi_mod(E) autofs4(E)
> CPU: 1 PID: 223 Comm: kworker/1:2 Tainted: G E 4.5.0-rc2-11-default+ #90
> Workqueue: events_freezable_power_ disk_events_workfn
> task: c0000000f7d99aa0 ti: c0000000f7cb8000 task.ti: c0000000f7cb8000
> NIP: d000000000bde35c LR: d000000000bcffec CTR: d000000000bcffe0
> REGS: c0000000f7cbb3b0 TRAP: 0300 Tainted: G E (4.5.0-rc2-11-default+)
> MSR: 8000000100009033 <SF,EE,ME,IR,DR,RI,LE,TM[E]> CR: 24c82220 XER: 00000000
> CFAR: d000000000bdd144 DAR: d00000000108b148 DSISR: 40000000 SOFTE: 0
> GPR00: d0000000010a21cc c0000000f7cbb630 d0000000010aeda0 0000000000008200
> GPR04: c0000000fa0b33fc 000000000000014a c0000000fa0b3408 0000000000000010
> GPR08: 0000000000000008 c00000000370a4e0 0000000000000000 d00000000108b128
> GPR12: d000000000bcffe0 c000000007e80300 c0000000000d8b18 c0000000fe04ba80
> GPR16: 0000000000000000 c0000000f7b6f208 0000000000000000 0000000000010000
> GPR20: c0000000f7b6f144 c0000000f7b6f140 d000000000bcbcf0 0000000000000000
> GPR24: 0000000000000200 0000000000000000 0000000000000001 c0000000f7b6f810
> GPR28: c0000000f7b6f000 c0000000f7b6f000 c0000000fa0b33a0 c0000000fe837e00
> NIP [d000000000bde35c] scsi_inq_str+0x21b0/0x41ac [scsi_mod]
> LR [d000000000bcffec] int_to_scsilun+0xc/0x60 [scsi_mod]
> Call Trace:
> [c0000000f7cbb630] [d0000000010a21cc] ibmvscsi_queuecommand+0x10c/0x4e0 [ibmvscsi] (unreliable)
> [c0000000f7cbb6e0] [d000000000bcbea8] scsi_dispatch_cmd+0xe8/0x2c0 [scsi_mod]
> [c0000000f7cbb760] [d000000000bceb0c] scsi_request_fn+0x50c/0x8b0 [scsi_mod]
> [c0000000f7cbb850] [c000000000407280] __blk_run_queue+0x60/0x90
> [c0000000f7cbb880] [c000000000413640] blk_execute_rq_nowait+0x100/0x1a0
> [c0000000f7cbb8d0] [c000000000413768] blk_execute_rq+0x88/0x170
> [c0000000f7cbb9b0] [d000000000bca048] scsi_execute+0x108/0x1d0 [scsi_mod]
> [c0000000f7cbba20] [d000000000bca2e8] scsi_execute_req_flags+0xc8/0x150 [scsi_mod]
> [c0000000f7cbbae0] [d0000000012209e4] sr_check_events+0xb4/0x340 [sr_mod]
> [c0000000f7cbbb90] [d0000000011c00b4] cdrom_check_events+0x44/0x80 [cdrom]
> [c0000000f7cbbbc0] [d000000001220fa4] sr_block_check_events+0x44/0x60 [sr_mod]
> [c0000000f7cbbbe0] [c0000000004222f8] disk_check_events+0x78/0x1b0
> [c0000000f7cbbc50] [c0000000000d0610] process_one_work+0x1a0/0x480
> [c0000000f7cbbce0] [c0000000000d0998] worker_thread+0xa8/0x5c0
> [c0000000f7cbbd80] [c0000000000d8c24] kthread+0x114/0x140
> [c0000000f7cbbe30] [c000000000009538] ret_from_kernel_thread+0x5c/0xa4
> Instruction dump:
> 396bc360 f8410018 e98b0020 7d8903a6 4e800420 00000000 00000000 0044fb30
> c0000000 3d62fffe 396bc388 60000000 <e98b0020> 7d8903a6 4e800420 00000000
> ---[ end trace 3b830c669dd7adb5 ]---
>
>
> Note that ibmvscsi_queuecommand() handle TOC and int_to_scsilun()
> does not handle TOC
>
> $> objdump -hdr drivers/scsi/ibmvscsi/ibmvscsi.ko
> 00000000000020c0 <ibmvscsi_queuecommand>:
> 20c0: 00 00 4c 3c addis r2,r12,0
> 20c0: R_PPC64_REL16_HA .TOC.
> 20c4: 00 00 42 38 addi r2,r2,0
> 20c4: R_PPC64_REL16_LO .TOC.+0x4
> 20c8: a6 02 08 7c mflr r0
> 20cc: 10 00 01 f8 std r0,16(r1)
> 20d0: 01 00 00 48 bl 20d0 <ibmvscsi_queuecommand+0x10>
> 20d0: R_PPC64_REL24 _mcount
> 20d4: a6 02 08 7c mflr r0
>
>
> $> objdump -hdr drivers/scsi/scsi_mod.ko
> [...]
> 000000000000ffe0 <int_to_scsilun>:
> ffe0: a6 02 08 7c mflr r0
> ffe4: 10 00 01 f8 std r0,16(r1)
> ffe8: 01 00 00 48 bl ffe8 <int_to_scsilun+0x8>
> ffe8: R_PPC64_REL24 _mcount
> ffec: a6 02 08 7c mflr r0
> fff0: 10 00 01 f8 std r0,16(r1)
> fff4: e1 ff 21 f8 stdu r1,-32(r1)
>
>
> I am able to trace int_to_scsilun() if I add a global call
> into this fucntion.
>
> I could trace any function when everything is built into the kernel.
>
>
> I am not sure where the problem is but even normal tracing does not
> work with this patchset and when -pg -mprofile-kernel is used.
When you call into ibmvscsi_queuecommand() it sets up r2 to hold the TOC for
ibmvscsi.ko.
It then calls int_to_scsilun(). int_to_scsilun() does no TOC setup, and tries
to call _mcount. It can't call _mcount() directly (because int_to_scsilun() is
in a module), it has to go via a stub.
That stub uses r2 to find the location of itself, but it only works if r2 holds
the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.
So when the stub calculates the address of itself it gets the wrong value, and
then when it does a load using that address it either faults (as in your case),
or you get some bogus value and jump into a random piece of code.
cheers
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-17 3:08 ` Michael Ellerman
@ 2016-02-23 17:00 ` Torsten Duwe
2016-02-24 6:37 ` Balbir Singh
2016-02-24 7:51 ` Kamalesh Babulal
0 siblings, 2 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-23 17:00 UTC (permalink / raw)
To: Michael Ellerman
Cc: Petr Mladek, Jessica Yu, Jiri Kosina, linux-kernel,
Steven Rostedt, Kamalesh Babulal, live-patching, Miroslav Benes,
linuxppc-dev
On Wed, Feb 17, 2016 at 02:08:41PM +1100, Michael Ellerman wrote:
>
> That stub uses r2 to find the location of itself, but it only works if r2 holds
> the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.
Here's my solution, a bit rough still. This replaces the module_64.c change
from patch 2/8:
I shuffle the trampoline instructions so the R2-save-to-stack comes first.
This allows me to construct a profiling trampoline code that
looks very similar. The first instruction, harmful to -mprofile-kernel
can now be replaced with a load of the *kernel* TOC value via paca.
Arithmetic is done in r11, to keep it bitwise identical where possible.
Likewise the result is "moved" to r12 via an addi.
What do you think?
Torsten
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -27,6 +27,7 @@
#include <linux/bug.h>
#include <linux/uaccess.h>
#include <asm/module.h>
+#include <asm/asm-offsets.h>
#include <asm/firmware.h>
#include <asm/code-patching.h>
#include <linux/sort.h>
@@ -123,10 +124,10 @@ struct ppc64_stub_entry
*/
static u32 ppc64_stub_insns[] = {
- 0x3d620000, /* addis r11,r2, <high> */
- 0x396b0000, /* addi r11,r11, <low> */
/* Save current r2 value in magic place on the stack. */
0xf8410000|R2_STACK_OFFSET, /* std r2,R2_STACK_OFFSET(r1) */
+ 0x3d620000, /* addis r11,r2, <high> */
+ 0x396b0000, /* addi r11,r11, <low> */
0xe98b0020, /* ld r12,32(r11) */
#if !defined(_CALL_ELF) || _CALL_ELF != 2
/* Set up new r2 from function descriptor */
@@ -136,13 +137,30 @@ static u32 ppc64_stub_insns[] = {
0x4e800420 /* bctr */
};
+/* In case of _mcount calls or dynamic ftracing with -mprofile-kernel,
+ * the stack frame already holds the TOC value of the original
+ * caller. And even worse, for a leaf function without global data
+ * references, R2 holds the TOC of the caller's caller, e.g. is
+ * completely undefined. So: do not dare to write r2 anywhere, and use
+ * the kernel's TOC to find _mcount / ftrace_caller. Mcount and
+ * ftrace_caller will then take care of the r2 value themselves.
+ */
+static u32 ppc64_profile_stub_insns[] = {
+ 0xe98d0000|PACATOC, /* ld r12,PACATOC(r13) */
+ 0x3d6c0000, /* addis r11,r12, <high> */
+ 0x396b0000, /* addi r11,r11, <low> */
+ 0x398b0000, /* addi r12,r11,0 */
+ 0x7d8903a6, /* mtctr r12 */
+ 0x4e800420 /* bctr */
+};
+
#ifdef CONFIG_DYNAMIC_FTRACE
static u32 ppc64_stub_mask[] = {
+ 0xee330000,
+ 0xfff10000,
0xffff0000,
- 0xffff0000,
- 0xffffffff,
- 0xffffffff,
+ 0x2fffffdf,
#if !defined(_CALL_ELF) || _CALL_ELF != 2
0xffffffff,
#endif
@@ -168,10 +186,15 @@ bool is_module_trampoline(u32 *p)
if ((insna & mask) != (insnb & mask))
return false;
}
+ if (insns[0] != ppc64_stub_insns[0] &&
+ insns[0] != ppc64_profile_stub_insns[0])
+ return false;
return true;
}
+extern unsigned long __toc_start;
+
int module_trampoline_target(struct module *mod, u32 *trampoline,
unsigned long *target)
{
@@ -180,7 +203,7 @@ int module_trampoline_target(struct modu
long offset;
void *toc_entry;
- if (probe_kernel_read(buf, trampoline, sizeof(buf)))
+ if (probe_kernel_read(buf, trampoline+1, sizeof(buf)))
return -EFAULT;
upper = buf[0] & 0xffff;
@@ -189,6 +212,13 @@ int module_trampoline_target(struct modu
/* perform the addis/addi, both signed */
offset = ((short)upper << 16) + (short)lower;
+ /* profiling trampolines work differently */
+ if ((buf[0] & 0xFFFF0000) == 0x3D6C0000)
+ {
+ *target = offset + (unsigned long)(&__toc_start) + 0x8000UL;
+ return 0;
+ }
+
/*
* Now get the address this trampoline jumps to. This
* is always 32 bytes into our trampoline stub.
@@ -427,14 +457,24 @@ static inline unsigned long my_r2(Elf64_
static inline int create_stub(Elf64_Shdr *sechdrs,
struct ppc64_stub_entry *entry,
unsigned long addr,
- struct module *me)
+ struct module *me,
+ bool prof)
{
long reladdr;
- memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+ if (prof)
+ {
+ memcpy(entry->jump, ppc64_profile_stub_insns,
+ sizeof(ppc64_stub_insns));
- /* Stub uses address relative to r2. */
- reladdr = (unsigned long)entry - my_r2(sechdrs, me);
+ /* Stub uses address relative to kernel TOC. */
+ reladdr = addr - ((unsigned long)(&__toc_start) + 0x8000UL);
+ } else {
+ memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
+
+ /* Stub uses address relative to r2. */
+ reladdr = (unsigned long)entry - my_r2(sechdrs, me);
+ }
if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
pr_err("%s: Address %p of stub out of range of %p.\n",
me->name, (void *)reladdr, (void *)my_r2);
@@ -442,8 +482,8 @@ static inline int create_stub(Elf64_Shdr
}
pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
- entry->jump[0] |= PPC_HA(reladdr);
- entry->jump[1] |= PPC_LO(reladdr);
+ entry->jump[1] |= PPC_HA(reladdr);
+ entry->jump[2] |= PPC_LO(reladdr);
entry->funcdata = func_desc(addr);
return 1;
}
@@ -452,7 +492,8 @@ static inline int create_stub(Elf64_Shdr
stub to set up the TOC ptr (r2) for the function. */
static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
unsigned long addr,
- struct module *me)
+ struct module *me,
+ bool prof)
{
struct ppc64_stub_entry *stubs;
unsigned int i, num_stubs;
@@ -468,44 +509,17 @@ static unsigned long stub_for_addr(Elf64
return (unsigned long)&stubs[i];
}
- if (!create_stub(sechdrs, &stubs[i], addr, me))
+ if (!create_stub(sechdrs, &stubs[i], addr, me, prof))
return 0;
return (unsigned long)&stubs[i];
}
-#ifdef CC_USING_MPROFILE_KERNEL
-static int is_early_mcount_callsite(u32 *instruction)
-{
- /* -mprofile-kernel sequence starting with
- * mflr r0 and maybe std r0, LRSAVE(r1).
- */
- if ((instruction[-3] == PPC_INST_MFLR &&
- instruction[-2] == PPC_INST_STD_LR) ||
- instruction[-2] == PPC_INST_MFLR) {
- /* Nothing to be done here, it's an _mcount
- * call location and r2 will have to be
- * restored in the _mcount function.
- */
- return 1;
- }
- return 0;
-}
-#else
-/* without -mprofile-kernel, mcount calls are never early */
-static int is_early_mcount_callsite(u32 *instruction)
-{
- return 0;
-}
-#endif
-
/* We expect a noop next: if it is, replace it with instruction to
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
if (*instruction != PPC_INST_NOP) {
- if (is_early_mcount_callsite(instruction))
- return 1;
pr_err("%s: Expect noop after relocate, got %08x\n",
me->name, *instruction);
return 0;
@@ -515,6 +529,12 @@ static int restore_r2(u32 *instruction,
return 1;
}
+#ifdef CC_USING_MPROFILE_KERNEL
+#define IS_KERNEL_PROFILING_CALL (!strcmp("_mcount", strtab+sym->st_name))
+#else
+#define IS_KERNEL_PROFILING_CALL 0
+#endif
+
int apply_relocate_add(Elf64_Shdr *sechdrs,
const char *strtab,
unsigned int symindex,
@@ -630,11 +650,15 @@ int apply_relocate_add(Elf64_Shdr *sechd
case R_PPC_REL24:
/* FIXME: Handle weak symbols here --RR */
if (sym->st_shndx == SHN_UNDEF) {
+ bool prof = false;
+ if (IS_KERNEL_PROFILING_CALL)
+ prof = true;
/* External: go via stub */
- value = stub_for_addr(sechdrs, value, me);
+ value = stub_for_addr(sechdrs, value, me, prof);
if (!value)
return -ENOENT;
- if (!restore_r2((u32 *)location + 1, me))
+ if (!prof &&
+ !restore_r2((u32 *)location + 1, me))
return -ENOEXEC;
} else
value += local_entry_offset(sym);
@@ -722,7 +746,7 @@ int apply_relocate_add(Elf64_Shdr *sechd
me->arch.toc = my_r2(sechdrs, me);
me->arch.tramp = stub_for_addr(sechdrs,
(unsigned long)ftrace_caller,
- me);
+ me, true);
#endif
return 0;
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-23 17:00 ` Torsten Duwe
@ 2016-02-24 6:37 ` Balbir Singh
2016-02-24 6:55 ` Balbir Singh
2016-02-24 7:51 ` Kamalesh Babulal
1 sibling, 1 reply; 44+ messages in thread
From: Balbir Singh @ 2016-02-24 6:37 UTC (permalink / raw)
To: Torsten Duwe, Michael Ellerman
Cc: Petr Mladek, Jessica Yu, Jiri Kosina, linux-kernel,
Steven Rostedt, Kamalesh Babulal, live-patching, Miroslav Benes,
linuxppc-dev
On 24/02/16 04:00, Torsten Duwe wrote:
> On Wed, Feb 17, 2016 at 02:08:41PM +1100, Michael Ellerman wrote:
>> That stub uses r2 to find the location of itself, but it only works if r2 holds
>> the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.
> Here's my solution, a bit rough still. This replaces the module_64.c change
> from patch 2/8:
>
> I shuffle the trampoline instructions so the R2-save-to-stack comes first.
> This allows me to construct a profiling trampoline code that
> looks very similar. The first instruction, harmful to -mprofile-kernel
> can now be replaced with a load of the *kernel* TOC value via paca.
> Arithmetic is done in r11, to keep it bitwise identical where possible.
> Likewise the result is "moved" to r12 via an addi.
Michael has a similar change that he intends to post. I gave this a run but
the system crashes on boot.
>
> What do you think?
>
> Torsten
>
>
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -27,6 +27,7 @@
> #include <linux/bug.h>
> #include <linux/uaccess.h>
> #include <asm/module.h>
> +#include <asm/asm-offsets.h>
Creates an include conflict for me. NMI_MASK, PGD_TABLE_SIZE, etc
are already defined elsewhere.
> #include <asm/firmware.h>
> #include <asm/code-patching.h>
> #include <linux/sort.h>
> @@ -123,10 +124,10 @@ struct ppc64_stub_entry
> */
>
> static u32 ppc64_stub_insns[] = {
> - 0x3d620000, /* addis r11,r2, <high> */
> - 0x396b0000, /* addi r11,r11, <low> */
> /* Save current r2 value in magic place on the stack. */
> 0xf8410000|R2_STACK_OFFSET, /* std r2,R2_STACK_OFFSET(r1) */
> + 0x3d620000, /* addis r11,r2, <high> */
> + 0x396b0000, /* addi r11,r11, <low> */
> 0xe98b0020, /* ld r12,32(r11) */
> #if !defined(_CALL_ELF) || _CALL_ELF != 2
> /* Set up new r2 from function descriptor */
> @@ -136,13 +137,30 @@ static u32 ppc64_stub_insns[] = {
> 0x4e800420 /* bctr */
> };
>
> +/* In case of _mcount calls or dynamic ftracing with -mprofile-kernel,
> + * the stack frame already holds the TOC value of the original
> + * caller. And even worse, for a leaf function without global data
> + * references, R2 holds the TOC of the caller's caller, e.g. is
> + * completely undefined. So: do not dare to write r2 anywhere, and use
> + * the kernel's TOC to find _mcount / ftrace_caller. Mcount and
> + * ftrace_caller will then take care of the r2 value themselves.
> + */
> +static u32 ppc64_profile_stub_insns[] = {
> + 0xe98d0000|PACATOC, /* ld r12,PACATOC(r13) */
> + 0x3d6c0000, /* addis r11,r12, <high> */
> + 0x396b0000, /* addi r11,r11, <low> */
> + 0x398b0000, /* addi r12,r11,0 */
> + 0x7d8903a6, /* mtctr r12 */
> + 0x4e800420 /* bctr */
> +};
> +
> #ifdef CONFIG_DYNAMIC_FTRACE
>
> static u32 ppc64_stub_mask[] = {
> + 0xee330000,
> + 0xfff10000,
> 0xffff0000,
> - 0xffff0000,
> - 0xffffffff,
> - 0xffffffff,
> + 0x2fffffdf,
> #if !defined(_CALL_ELF) || _CALL_ELF != 2
> 0xffffffff,
> #endif
> @@ -168,10 +186,15 @@ bool is_module_trampoline(u32 *p)
> if ((insna & mask) != (insnb & mask))
> return false;
> }
> + if (insns[0] != ppc64_stub_insns[0] &&
> + insns[0] != ppc64_profile_stub_insns[0])
> + return false;
>
Michael was mentioning a better way of doing this, we can simplify the
checking bits
> return true;
> }
>
> +extern unsigned long __toc_start;
> +
> int module_trampoline_target(struct module *mod, u32 *trampoline,
> unsigned long *target)
> {
> @@ -180,7 +203,7 @@ int module_trampoline_target(struct modu
> long offset;
> void *toc_entry;
>
> - if (probe_kernel_read(buf, trampoline, sizeof(buf)))
> + if (probe_kernel_read(buf, trampoline+1, sizeof(buf)))
> return -EFAULT;
>
> upper = buf[0] & 0xffff;
> @@ -189,6 +212,13 @@ int module_trampoline_target(struct modu
> /* perform the addis/addi, both signed */
> offset = ((short)upper << 16) + (short)lower;
>
> + /* profiling trampolines work differently */
> + if ((buf[0] & 0xFFFF0000) == 0x3D6C0000)
> + {
> + *target = offset + (unsigned long)(&__toc_start) + 0x8000UL;
> + return 0;
> + }
> +
> /*
> * Now get the address this trampoline jumps to. This
> * is always 32 bytes into our trampoline stub.
> @@ -427,14 +457,24 @@ static inline unsigned long my_r2(Elf64_
> static inline int create_stub(Elf64_Shdr *sechdrs,
> struct ppc64_stub_entry *entry,
> unsigned long addr,
> - struct module *me)
> + struct module *me,
> + bool prof)
> {
> long reladdr;
>
> - memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
> + if (prof)
> + {
> + memcpy(entry->jump, ppc64_profile_stub_insns,
> + sizeof(ppc64_stub_insns));
>
> - /* Stub uses address relative to r2. */
> - reladdr = (unsigned long)entry - my_r2(sechdrs, me);
> + /* Stub uses address relative to kernel TOC. */
> + reladdr = addr - ((unsigned long)(&__toc_start) + 0x8000UL);
> + } else {
> + memcpy(entry->jump, ppc64_stub_insns, sizeof(ppc64_stub_insns));
> +
> + /* Stub uses address relative to r2. */
> + reladdr = (unsigned long)entry - my_r2(sechdrs, me);
> + }
> if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> pr_err("%s: Address %p of stub out of range of %p.\n",
> me->name, (void *)reladdr, (void *)my_r2);
> @@ -442,8 +482,8 @@ static inline int create_stub(Elf64_Shdr
> }
> pr_debug("Stub %p get data from reladdr %li\n", entry, reladdr);
>
> - entry->jump[0] |= PPC_HA(reladdr);
> - entry->jump[1] |= PPC_LO(reladdr);
> + entry->jump[1] |= PPC_HA(reladdr);
> + entry->jump[2] |= PPC_LO(reladdr);
> entry->funcdata = func_desc(addr);
> return 1;
> }
> @@ -452,7 +492,8 @@ static inline int create_stub(Elf64_Shdr
> stub to set up the TOC ptr (r2) for the function. */
> static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
> unsigned long addr,
> - struct module *me)
> + struct module *me,
> + bool prof)
> {
> struct ppc64_stub_entry *stubs;
> unsigned int i, num_stubs;
> @@ -468,44 +509,17 @@ static unsigned long stub_for_addr(Elf64
> return (unsigned long)&stubs[i];
> }
>
> - if (!create_stub(sechdrs, &stubs[i], addr, me))
> + if (!create_stub(sechdrs, &stubs[i], addr, me, prof))
> return 0;
>
> return (unsigned long)&stubs[i];
> }
>
> -#ifdef CC_USING_MPROFILE_KERNEL
> -static int is_early_mcount_callsite(u32 *instruction)
> -{
> - /* -mprofile-kernel sequence starting with
> - * mflr r0 and maybe std r0, LRSAVE(r1).
> - */
> - if ((instruction[-3] == PPC_INST_MFLR &&
> - instruction[-2] == PPC_INST_STD_LR) ||
> - instruction[-2] == PPC_INST_MFLR) {
> - /* Nothing to be done here, it's an _mcount
> - * call location and r2 will have to be
> - * restored in the _mcount function.
> - */
> - return 1;
> - }
> - return 0;
> -}
> -#else
> -/* without -mprofile-kernel, mcount calls are never early */
> -static int is_early_mcount_callsite(u32 *instruction)
> -{
> - return 0;
> -}
> -#endif
> -
We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now
that the ppc64_profile_stub_insns does not save r2
> /* We expect a noop next: if it is, replace it with instruction to
> restore r2. */
> static int restore_r2(u32 *instruction, struct module *me)
> {
> if (*instruction != PPC_INST_NOP) {
> - if (is_early_mcount_callsite(instruction))
> - return 1;
> pr_err("%s: Expect noop after relocate, got %08x\n",
> me->name, *instruction);
> return 0;
> @@ -515,6 +529,12 @@ static int restore_r2(u32 *instruction,
> return 1;
> }
>
> +#ifdef CC_USING_MPROFILE_KERNEL
> +#define IS_KERNEL_PROFILING_CALL (!strcmp("_mcount", strtab+sym->st_name))
> +#else
> +#define IS_KERNEL_PROFILING_CALL 0
> +#endif
> +
> int apply_relocate_add(Elf64_Shdr *sechdrs,
> const char *strtab,
> unsigned int symindex,
> @@ -630,11 +650,15 @@ int apply_relocate_add(Elf64_Shdr *sechd
> case R_PPC_REL24:
> /* FIXME: Handle weak symbols here --RR */
> if (sym->st_shndx == SHN_UNDEF) {
> + bool prof = false;
> + if (IS_KERNEL_PROFILING_CALL)
> + prof = true;
> /* External: go via stub */
> - value = stub_for_addr(sechdrs, value, me);
> + value = stub_for_addr(sechdrs, value, me, prof);
> if (!value)
> return -ENOENT;
> - if (!restore_r2((u32 *)location + 1, me))
> + if (!prof &&
> + !restore_r2((u32 *)location + 1, me))
> return -ENOEXEC;
> } else
> value += local_entry_offset(sym);
> @@ -722,7 +746,7 @@ int apply_relocate_add(Elf64_Shdr *sechd
> me->arch.toc = my_r2(sechdrs, me);
> me->arch.tramp = stub_for_addr(sechdrs,
> (unsigned long)ftrace_caller,
> - me);
> + me, true);
> #endif
>
> return 0;
Looks like we are getting closer to the final solution
Thanks,
Balbir
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-24 6:37 ` Balbir Singh
@ 2016-02-24 6:55 ` Balbir Singh
2016-02-24 9:23 ` Torsten Duwe
0 siblings, 1 reply; 44+ messages in thread
From: Balbir Singh @ 2016-02-24 6:55 UTC (permalink / raw)
To: Torsten Duwe, Michael Ellerman
Cc: Petr Mladek, Jessica Yu, Jiri Kosina, linux-kernel,
Steven Rostedt, Kamalesh Babulal, live-patching, Miroslav Benes,
linuxppc-dev
<snip>
We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now that the ppc64_profile_stub_insns does not save r2
> Looks like we are getting closer to the final solution Thanks, Balbir
With the SQUASH_TOC_SAVE_INSNS removed, ftrace function seems to work, but function_graph is broken. I've not yet debugged this.
[ 77.182430] b'Oops: Kernel access of bad area, sig: 11 [#1]'
[ 77.182464] b'SMP NR_CPUS=32 NUMA pSeries'
[ 77.182513] b'Modules linked in: sr_mod cdrom virtio_blk virtio_net ibmvscsi scsi_transport_srp scsi_mod virtio_pci virtio_ring virtio'
[ 77.182661] b'CPU: 1 PID: 2287 Comm: sshd Not tainted 4.5.0-rc4-00007-g1968536-dirty #143'
[ 77.182709] b'task: c000000037b6bc00 ti: c00000003e8c4000 task.ti: c00000003e8c4000'
[ 77.182757] b'NIP: c000000000194ebc LR: c000000000049d4c CTR: d0000000004f1434'
[ 77.182804] b'REGS: c00000003e8c72a0 TRAP: 0300 Not tainted (4.5.0-rc4-00007-g1968536-dirty)'
[ 77.182858] b'MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 28282828 XER: 20000000'
[ 77.183008] b'CFAR: c00000000017f400 DAR: d000000000653c70 DSISR: 40000000 SOFTE: 1 '
[ 77.183008] b'GPR00: c000000000009f3c c00000003e8c7520 d0000000004fde40 c00000000077da34 '
[ 77.183008] b'GPR04: d0000000004f1430 c00000003e008100 c00000003719e520 c00000003719e008 '
[ 77.183008] b'GPR08: c00000000113ea50 d00000000064de40 d0000000004f57b8 c000000000009d1c '
[ 77.183008] b'GPR12: c00000000077da34 c00000000fff8400 00000000000005a8 0000000040000000 '
[ 77.183008] b'GPR16: 0000000022000000 00000000000346db c00000003e8c7720 c00000000113fbe0 '
[ 77.183008] b'GPR20: 0000000000000000 c00000000113fbd0 c000000037a9dc00 0000000000000000 '
[ 77.183008] b'GPR24: 0000000000000000 c00000000113fbe0 c000000037077000 c000000037077090 '
[ 77.183008] b'GPR28: c0000000379982d8 d0000000004f1430 c00000000077da34 c000000037077068 '
[ 77.183788] b'NIP [c000000000194ebc] ftrace_graph_is_dead+0xc/0x20'
[ 77.183850] b'LR [c000000000049d4c] prepare_ftrace_return+0x2c/0x110'
---Type <return> to continue, or q <return> to quit---
[ 77.183890] b'Call Trace:'
[ 77.183911] b'[c00000003e8c7520] [c000000037a9dc00] 0xc000000037a9dc00 (unreliable)'
[ 77.183987] b'[c00000003e8c7570] [c000000000009f3c] ftrace_graph_caller+0x34/0x74'
[ 77.184080] b'[c00000003e8c75e0] [c00000000077da34] dev_hard_start_xmit+0x374/0x4e0'
[ 77.184139] b'[c00000003e8c76c0] [c000000000009f7c] return_to_handler+0x0/0x58 (bad_page_fault+0x130/0x150)'
[ 77.184210] b'[c00000003e8c7760] [c000000000009f7c] return_to_handler+0x0/0x58 (handle_page_fault+0x2c/0x30)'
[ 77.184281] b'[c00000003e8c7800] [c000000000009f7c] return_to_handler+0x0/0x58 (sch_direct_xmit+0xe0/0x2d0)'
[ 77.184369] b'[c00000003e8c7860] [c000000000009f7c] return_to_handler+0x0/0x58 (__dev_queue_xmit+0x2d4/0x6a0)'
[ 77.184473] b'[c00000003e8c78f0] [c000000000009f7c] return_to_handler+0x0/0x58 (return_to_handler+0x0/0x58)'
[ 77.184544] b'[c00000003e8c7930] [c000000000009f7c] return_to_handler+0x0/0x58 (ip_finish_output2+0x348/0x420)'
[ 77.184614] b'[c00000003e8c79a0] [c000000000009f7c] return_to_handler+0x0/0x58 (return_to_handler+0x0/0x58)'
[ 77.184684] b'[c00000003e8c7a70] [c000000000009f7c] return_to_handler+0x0/0x58 (ip_output+0xd0/0x160)'
[ 77.184754] b'[c00000003e8c7ae0] [c000000000009f7c] return_to_handler+0x0/0x58 (ip_local_out+0x6c/0x90)'
[ 77.184823] b'[c00000003e8c7b30] [c000000000009f7c] return_to_handler+0x0/0x58 (return_to_handler+0x0/0x58)'
[ 77.184893] b'[c00000003e8c7c00] [c000000000009f7c] return_to_handler+0x0/0x58 (tcp_transmit_skb+0x980/0xa50)'
[ 77.184969] b'[c00000003e8c7c40] [c000000000009f7c] return_to_handler+0x0/0x58 (tcp_write_xmit+0xd9c/0x1120)'
[ 77.185039] b'[c00000003e8c7c60] [c000000000009f7c] return_to_handler+0x0/0x58 (__tcp_push_pending_frames+0x50/0x130)'
[ 77.185117] b'[c00000003e8c7d00] [c000000000009f7c] return_to_handler+0x0/0x58 (tcp_push+0x194/0x1e0)'
[ 77.185192] b'[c00000003e8c7d90] [c000000000009f7c] return_to_handler+0x0/0x58 (tcp_sendmsg+0xa54/0xce0)'
[ 77.185262] b'[c00000003e8c7de0] [c000000000009f7c] return_to_handler+0x0/0x58 (inet_sendmsg+0xd8/0x100)'
[ 77.185342] b'[c00000003e8c7e30] [c000000000009f7c] return_to_handler+0x0/0x58 (sock_sendmsg+0x38/0x60)'
[ 77.185416] b'Instruction dump:'
[ 77.185469] b'60000000 4bfe3b89 60000000 e8610020 38210030 e8010010 7c0803a6 4bffff40 '
[ 77.185566] b'60420000 3c4c00fa 3842e750 3d220015 <88695e30> 4e800020 60000000 60000000 '
[ 77.185668] b'---[ end trace 78e882547ec0a563 ]---'
[ 79.191159] b'Kernel panic - not syncing: Fatal exception in interrupt'
Warm Regards,
Balbir Singh.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-24 6:55 ` Balbir Singh
@ 2016-02-24 9:23 ` Torsten Duwe
2016-02-24 11:22 ` Balbir Singh
0 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-24 9:23 UTC (permalink / raw)
To: Balbir Singh
Cc: Michael Ellerman, Petr Mladek, Jessica Yu, Jiri Kosina,
linux-kernel, Steven Rostedt, Kamalesh Babulal, live-patching,
Miroslav Benes, linuxppc-dev
On Wed, Feb 24, 2016 at 05:55:35PM +1100, Balbir Singh wrote:
> <snip>
>
> We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now that the ppc64_profile_stub_insns does not save r2
Sure -- this was meant to _replace_ the changes from patch 2/8, not on top.
And yes, it exposes duplicate definitions, but does not cause them AFAICS.
The two unasked questions about it were: Is Michael's solution on a similar
basis? Is this worth any further effort e.g. put into v9?
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-24 9:23 ` Torsten Duwe
@ 2016-02-24 11:22 ` Balbir Singh
0 siblings, 0 replies; 44+ messages in thread
From: Balbir Singh @ 2016-02-24 11:22 UTC (permalink / raw)
To: Torsten Duwe
Cc: Michael Ellerman, Petr Mladek, Jessica Yu, Jiri Kosina,
linux-kernel, Steven Rostedt, Kamalesh Babulal, live-patching,
Miroslav Benes, linuxppc-dev
On 24/02/16 20:23, Torsten Duwe wrote:
> On Wed, Feb 24, 2016 at 05:55:35PM +1100, Balbir Singh wrote:
>> <snip>
>>
>> We need to remove the SQUASH_TOC_SAVE_INSNS bits as well, now that the ppc64_profile_stub_insns does not save r2
> Sure -- this was meant to _replace_ the changes from patch 2/8, not on top.
> And yes, it exposes duplicate definitions, but does not cause them AFAICS.
> The two unasked questions about it were: Is Michael's solution on a similar
> basis? Is this worth any further effort e.g. put into v9?
>
>
My bad you did mention _replace_, but I think 2/8 and 6/8 of tightly bound
together, so the replacement is not straight forward. Yes, it is heading in
a similar direction, but it focuses mostly on ftrace. I think v9 makes sense,
but I'll let Michael comment on this as well]
Personally, I think your v8 or v9 + Michael's changes - RECORD_C_MCOUNT +
some changes (yet to code them based on v8/v9/ftrace stability) should get
the full live patching working.
Balbir Singh.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build
2016-02-23 17:00 ` Torsten Duwe
2016-02-24 6:37 ` Balbir Singh
@ 2016-02-24 7:51 ` Kamalesh Babulal
1 sibling, 0 replies; 44+ messages in thread
From: Kamalesh Babulal @ 2016-02-24 7:51 UTC (permalink / raw)
To: Torsten Duwe
Cc: Michael Ellerman, Petr Mladek, Jessica Yu, Jiri Kosina,
linux-kernel, Steven Rostedt, live-patching, Miroslav Benes,
linuxppc-dev
* Torsten Duwe <duwe@lst.de> [2016-02-23 18:00:17]:
> On Wed, Feb 17, 2016 at 02:08:41PM +1100, Michael Ellerman wrote:
> >
> > That stub uses r2 to find the location of itself, but it only works if r2 holds
> > the TOC for scsi_mod.ko. In this case r2 still contains ibmvscsi.ko's TOC.
>
> Here's my solution, a bit rough still. This replaces the module_64.c change
> from patch 2/8:
>
> I shuffle the trampoline instructions so the R2-save-to-stack comes first.
> This allows me to construct a profiling trampoline code that
> looks very similar. The first instruction, harmful to -mprofile-kernel
> can now be replaced with a load of the *kernel* TOC value via paca.
> Arithmetic is done in r11, to keep it bitwise identical where possible.
> Likewise the result is "moved" to r12 via an addi.
>
> What do you think?
>
Hi Torsten,
I hit build failure, after replacing this patch with patch 2/8 module_64.c
hunk.
CC arch/powerpc/kernel/module.o
CC arch/powerpc/kernel/module_64.o
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:14:0: error: "NMI_MASK" redefined [-Werror]
#define NMI_MASK 1048576 /* NMI_MASK # */
^
In file included from include/linux/spinlock.h:50:0,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/stat.h:18,
from include/linux/module.h:10,
from arch/powerpc/kernel/module_64.c:21:
include/linux/preempt.h:46:0: note: this is the location of the previous definition
#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:148:0: error: "CLONE_VM" redefined [-Werror]
#define CLONE_VM 256 /* CLONE_VM # */
^
In file included from include/linux/sched.h:4:0,
from ./arch/powerpc/include/asm/elf.h:12,
from include/linux/elf.h:4,
from include/linux/module.h:15,
from arch/powerpc/kernel/module_64.c:21:
include/uapi/linux/sched.h:8:0: note: this is the location of the previous definition
#define CLONE_VM 0x00000100 /* set if VM shared between processes */
^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:149:0: error: "CLONE_UNTRACED" redefined [-Werror]
#define CLONE_UNTRACED 8388608 /* CLONE_UNTRACED # */
^
In file included from include/linux/sched.h:4:0,
from ./arch/powerpc/include/asm/elf.h:12,
from include/linux/elf.h:4,
from include/linux/module.h:15,
from arch/powerpc/kernel/module_64.c:21:
include/uapi/linux/sched.h:22:0: note: this is the location of the previous definition
#define CLONE_UNTRACED 0x00800000 /* set if the tracing process can't force CLONE_PTRACE on this clone */
^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:185:0: error: "NSEC_PER_SEC" redefined [-Werror]
#define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC # */
^
In file included from include/linux/time.h:7:0,
from include/linux/stat.h:18,
from include/linux/module.h:10,
from arch/powerpc/kernel/module_64.c:21:
include/linux/time64.h:35:0: note: this is the location of the previous definition
#define NSEC_PER_SEC 1000000000L
^
In file included from ./arch/powerpc/include/asm/asm-offsets.h:1:0,
from arch/powerpc/kernel/module_64.c:30:
include/generated/asm-offsets.h:188:0: error: "PGD_TABLE_SIZE" redefined [-Werror]
#define PGD_TABLE_SIZE 32768 /* PGD_TABLE_SIZE # */
^
In file included from ./arch/powerpc/include/asm/book3s/64/hash.h:58:0,
from ./arch/powerpc/include/asm/book3s/64/pgtable.h:8,
from ./arch/powerpc/include/asm/mmu-hash64.h:24,
from ./arch/powerpc/include/asm/mmu.h:185,
from ./arch/powerpc/include/asm/lppaca.h:36,
from ./arch/powerpc/include/asm/paca.h:21,
from ./arch/powerpc/include/asm/hw_irq.h:42,
from ./arch/powerpc/include/asm/irqflags.h:11,
from include/linux/irqflags.h:15,
from include/linux/spinlock.h:53,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/stat.h:18,
from include/linux/module.h:10,
from arch/powerpc/kernel/module_64.c:21:
./arch/powerpc/include/asm/book3s/64/hash-64k.h:133:0: note: this is the location of the previous definition
#define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
^
cc1: all warnings being treated as errors
scripts/Makefile.build:258: recipe for target 'arch/powerpc/kernel/module_64.o' failed
make[1]: *** [arch/powerpc/kernel/module_64.o] Error 1
Makefile:950: recipe for target 'arch/powerpc/kernel' failed
make: *** [arch/powerpc/kernel] Error 2
Thanks,
Kamalesh.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
2016-01-28 15:32 ` [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build Torsten Duwe
@ 2016-02-10 16:21 ` Torsten Duwe
2016-02-17 10:55 ` Michael Ellerman
2016-02-10 16:22 ` [PATCH v8 2/8] ppc64le FTRACE_WITH_REGS implementation Torsten Duwe
` (6 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
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.
* arch/powerpc/kernel/entry_64.S:
- modify the default _mcount to be prepared for such call sites
- have the ftrace_graph_caller save function arguments before
calling its C helper prepare_ftrace_return
* arch/powerpc/include/asm/code-patching.h:
- define some common macros to make things readable.
- pull the R2 stack location definition from
arch/powerpc/kernel/module_64.c
* arch/powerpc/kernel/module_64.c:
- enhance binary code examination to handle the new patterns.
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/include/asm/code-patching.h | 24 ++++++++++++++++
arch/powerpc/kernel/entry_64.S | 48 +++++++++++++++++++++++++++++++-
arch/powerpc/kernel/ftrace.c | 44 ++++++++++++++++++++++-------
arch/powerpc/kernel/module_64.c | 31 +++++++++++++++++++--
4 files changed, 133 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 840a550..7820b32 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -99,4 +99,28 @@ static inline unsigned long ppc_global_function_entry(void *func)
#endif
}
+#ifdef CONFIG_PPC64
+/* Some instruction encodings commonly used in dynamic ftracing
+ * and function live patching:
+ */
+
+/* This must match the definition of STK_GOT in <asm/ppc_asm.h> */
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+#define R2_STACK_OFFSET 24
+#else
+#define R2_STACK_OFFSET 40
+#endif
+
+/* load / store the TOC from / into the stack frame */
+#define PPC_INST_LD_TOC (PPC_INST_LD | ___PPC_RT(__REG_R2) | \
+ ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
+#define PPC_INST_STD_TOC (PPC_INST_STD | ___PPC_RS(__REG_R2) | \
+ ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
+
+/* usually preceded by a mflr r0 */
+#define PPC_INST_STD_LR (PPC_INST_STD | ___PPC_RS(__REG_R0) | \
+ ___PPC_RA(__REG_R1) | PPC_LR_STKOFF)
+
+#endif /* CONFIG_PPC64 */
+
#endif /* _ASM_POWERPC_CODE_PATCHING_H */
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 0d525ce..2a7313c 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1143,7 +1143,10 @@ _GLOBAL(enter_prom)
#ifdef CONFIG_DYNAMIC_FTRACE
_GLOBAL(mcount)
_GLOBAL(_mcount)
- blr
+ mflr r12
+ mtctr r12
+ mtlr r0
+ bctr
_GLOBAL_TOC(ftrace_caller)
/* Taken from output of objdump from lib64/glibc */
@@ -1198,6 +1201,7 @@ _GLOBAL(ftrace_stub)
#endif /* CONFIG_DYNAMIC_FTRACE */
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#ifndef CC_USING_MPROFILE_KERNEL
_GLOBAL(ftrace_graph_caller)
/* load r4 with local address */
ld r4, 128(r1)
@@ -1222,6 +1226,48 @@ _GLOBAL(ftrace_graph_caller)
addi r1, r1, 112
blr
+#else /* CC_USING_MPROFILE_KERNEL */
+_GLOBAL(ftrace_graph_caller)
+ /* 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 */
+ subi r4, r4, MCOUNT_INSN_SIZE
+
+ bl prepare_ftrace_return
+ nop
+
+ /*
+ * prepare_ftrace_return gives us the address we divert to.
+ * Change the LR to this.
+ */
+ 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
+#endif /* CC_USING_MPROFILE_KERNEL */
+
_GLOBAL(return_to_handler)
/* need to save return values */
std r4, -32(r1)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 44d4d8e..861af90 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -287,16 +287,14 @@ int ftrace_make_nop(struct module *mod,
#ifdef CONFIG_MODULES
#ifdef CONFIG_PPC64
+/* Examine the existing instructions for __ftrace_make_call.
+ * They should effectively be a NOP, and follow formal constraints,
+ * depending on the ABI. Return false if they don't.
+ */
+#ifndef CC_USING_MPROFILE_KERNEL
static int
-__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
{
- unsigned int op[2];
- void *ip = (void *)rec->ip;
-
- /* read where this goes */
- if (probe_kernel_read(op, ip, sizeof(op)))
- return -EFAULT;
-
/*
* We expect to see:
*
@@ -306,8 +304,34 @@ __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.
*/
- if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
- pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
+ if ((op0 != 0x48000008) || ((op1 & 0xffff0000) != 0xe8410000))
+ return 0;
+ return 1;
+}
+#else
+static int
+expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
+{
+ /* look for patched "NOP" on ppc64 with -mprofile-kernel */
+ if (op0 != PPC_INST_NOP)
+ return 0;
+ return 1;
+}
+#endif
+
+static int
+__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+ unsigned int op[2];
+ void *ip = (void *)rec->ip;
+
+ /* read where this goes */
+ if (probe_kernel_read(op, ip, sizeof(op)))
+ return -EFAULT;
+
+ if (!expected_nop_sequence(ip, op[0], op[1])) {
+ pr_err("Unexpected call sequence at %p: %x %x\n",
+ ip, op[0], op[1]);
return -EINVAL;
}
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ac64ffd..72a1a52 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -41,7 +41,6 @@
--RR. */
#if defined(_CALL_ELF) && _CALL_ELF == 2
-#define R2_STACK_OFFSET 24
/* An address is simply the address of the function. */
typedef unsigned long func_desc_t;
@@ -73,7 +72,6 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
return PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
}
#else
-#define R2_STACK_OFFSET 40
/* An address is address of the OPD entry, which contains address of fn. */
typedef struct ppc64_opd_entry func_desc_t;
@@ -476,17 +474,44 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
return (unsigned long)&stubs[i];
}
+#ifdef CC_USING_MPROFILE_KERNEL
+static int is_early_mcount_callsite(u32 *instruction)
+{
+ /* -mprofile-kernel sequence starting with
+ * mflr r0 and maybe std r0, LRSAVE(r1).
+ */
+ if ((instruction[-3] == PPC_INST_MFLR &&
+ instruction[-2] == PPC_INST_STD_LR) ||
+ instruction[-2] == PPC_INST_MFLR) {
+ /* Nothing to be done here, it's an _mcount
+ * call location and r2 will have to be
+ * restored in the _mcount function.
+ */
+ return 1;
+ }
+ return 0;
+}
+#else
+/* without -mprofile-kernel, mcount calls are never early */
+static int is_early_mcount_callsite(u32 *instruction)
+{
+ return 0;
+}
+#endif
+
/* We expect a noop next: if it is, replace it with instruction to
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
if (*instruction != PPC_INST_NOP) {
+ if (is_early_mcount_callsite(instruction))
+ return 1;
pr_err("%s: Expect noop after relocate, got %08x\n",
me->name, *instruction);
return 0;
}
/* ld r2,R2_STACK_OFFSET(r1) */
- *instruction = 0xe8410000 | R2_STACK_OFFSET;
+ *instruction = PPC_INST_LD_TOC;
return 1;
}
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel
2016-02-10 16:21 ` [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Torsten Duwe
@ 2016-02-17 10:55 ` Michael Ellerman
2016-02-17 11:30 ` Torsten Duwe
0 siblings, 1 reply; 44+ messages in thread
From: Michael Ellerman @ 2016-02-17 10:55 UTC (permalink / raw)
To: Torsten Duwe
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Wed, 2016-02-10 at 17:21 +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.
...
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index ac64ffd..72a1a52 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -476,17 +474,44 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
> return (unsigned long)&stubs[i];
> }
>
> +#ifdef CC_USING_MPROFILE_KERNEL
> +static int is_early_mcount_callsite(u32 *instruction)
> +{
> + /* -mprofile-kernel sequence starting with
> + * mflr r0 and maybe std r0, LRSAVE(r1).
> + */
> + if ((instruction[-3] == PPC_INST_MFLR &&
> + instruction[-2] == PPC_INST_STD_LR) ||
> + instruction[-2] == PPC_INST_MFLR) {
> + /* Nothing to be done here, it's an _mcount
> + * call location and r2 will have to be
> + * restored in the _mcount function.
> + */
> + return 1;
> + }
> + return 0;
> +}
So this logic to deal with the 2 vs 3 instruction version of the mcount
sequence is problematic.
On a kernel built with the 2 instruction version this will fault when the
function we're looking at is located at the beginning of a page. Because
instruction[-3] goes off the front of the mapping.
We can probably fix that. But it's still a bit dicey.
I'm wondering if we want to just say we only support the 2 instruction version.
Currently that means GCC 6 only, or a distro compiler with the backport of
e95d0248dace. But we could also ask GCC to backport it to 4.9 and 5.
Thoughts?
cheers
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel
2016-02-17 10:55 ` Michael Ellerman
@ 2016-02-17 11:30 ` Torsten Duwe
2016-02-17 11:39 ` Michael Ellerman
0 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-17 11:30 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Wed, Feb 17, 2016 at 09:55:40PM +1100, Michael Ellerman wrote:
> On Wed, 2016-02-10 at 17:21 +0100, Torsten Duwe wrote:
>
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -476,17 +474,44 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
> > return (unsigned long)&stubs[i];
> > }
> >
> > +#ifdef CC_USING_MPROFILE_KERNEL
> > +static int is_early_mcount_callsite(u32 *instruction)
> > +{
> > + /* -mprofile-kernel sequence starting with
> > + * mflr r0 and maybe std r0, LRSAVE(r1).
> > + */
> > + if ((instruction[-3] == PPC_INST_MFLR &&
> > + instruction[-2] == PPC_INST_STD_LR) ||
> > + instruction[-2] == PPC_INST_MFLR) {
> > + /* Nothing to be done here, it's an _mcount
> > + * call location and r2 will have to be
> > + * restored in the _mcount function.
> > + */
> > + return 1;
> > + }
> > + return 0;
> > +}
>
> On a kernel built with the 2 instruction version this will fault when the
> function we're looking at is located at the beginning of a page. Because
> instruction[-3] goes off the front of the mapping.
>
> We can probably fix that. But it's still a bit dicey.
Not necessarily. Now that it's a separate function, it can be nested a bit deeper,
so we don't take chances on compiler optimisation:
if (instruction[-2] == PPC_INST_STD_LR) /* where should R0 come from? there must be... */
{
if (instruction[-3] == PPC_INST_MFLR)
return 1;
}
else if (instruction[-2] == PPC_INST_MFLR)
return 1;
return 0;
> I'm wondering if we want to just say we only support the 2 instruction version.
> Currently that means GCC 6 only, or a distro compiler with the backport of
> e95d0248dace. But we could also ask GCC to backport it to 4.9 and 5.
>
> Thoughts?
IMHO that's a too weak reason for a too strong limitation. OTOH getting everyone
to use the 2 insn version sounds appealing...
Is e95d0248dace self-sufficient or does it depend on other improvements?
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel
2016-02-17 11:30 ` Torsten Duwe
@ 2016-02-17 11:39 ` Michael Ellerman
0 siblings, 0 replies; 44+ messages in thread
From: Michael Ellerman @ 2016-02-17 11:39 UTC (permalink / raw)
To: Torsten Duwe
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Wed, 2016-02-17 at 12:30 +0100, Torsten Duwe wrote:
> On Wed, Feb 17, 2016 at 09:55:40PM +1100, Michael Ellerman wrote:
> >
> > On a kernel built with the 2 instruction version this will fault when the
> > function we're looking at is located at the beginning of a page. Because
> > instruction[-3] goes off the front of the mapping.
> >
> > We can probably fix that. But it's still a bit dicey.
>
> Not necessarily. Now that it's a separate function, it can be nested a bit deeper,
> so we don't take chances on compiler optimisation:
>
> if (instruction[-2] == PPC_INST_STD_LR) /* where should R0 come from? there must be... */
> {
> if (instruction[-3] == PPC_INST_MFLR)
> return 1;
> }
> else if (instruction[-2] == PPC_INST_MFLR)
> return 1;
> return 0;
Yeah true that should work in practice.
It's still trivial to construct a module that will oops the loader, but I guess
that's always been true.
> > I'm wondering if we want to just say we only support the 2 instruction version.
> > Currently that means GCC 6 only, or a distro compiler with the backport of
> > e95d0248dace. But we could also ask GCC to backport it to 4.9 and 5.
>
> IMHO that's a too weak reason for a too strong limitation. OTOH getting everyone
> to use the 2 insn version sounds appealing...
Fair enough. I'm just trying to manage the complexity explosion.
I'd certainly advocate that you backport it to your toolchain.
> Is e95d0248dace self-sufficient or does it depend on other improvements?
AFAIK it's self sufficient, it just deletes a single line. I'll ask the GCC
guys tomorrow if they can backport it if you don't beat me to it :)
cheers
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v8 2/8] ppc64le FTRACE_WITH_REGS implementation
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
2016-01-28 15:32 ` [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build Torsten Duwe
2016-02-10 16:21 ` [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Torsten Duwe
@ 2016-02-10 16:22 ` Torsten Duwe
2016-02-10 16:22 ` [PATCH v8 3/8] ppc use ftrace_modify_all_code default Torsten Duwe
` (5 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:22 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
Implement FTRACE_WITH_REGS for powerpc64, on ELF ABI v2.
Initial work started by Vojtech Pavlik, used with permission.
* arch/powerpc/kernel/entry_64.S:
- Implement an effective ftrace_caller that works from
within the kernel binary as well as from modules.
* arch/powerpc/kernel/ftrace.c:
- be prepared to deal with ppc64 ELF ABI v2, especially
calls to _mcount that result from gcc -mprofile-kernel
- a little more error verbosity
* arch/powerpc/kernel/module_64.c:
- do not save the TOC pointer on the trampoline when the
destination is ftrace_caller. This trampoline jump happens from
a function prologue before a new stack frame is set up, so bad
things may happen otherwise...
- relax is_module_trampoline() to recognise the modified
trampoline.
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/include/asm/ftrace.h | 5 +++
arch/powerpc/kernel/entry_64.S | 78 +++++++++++++++++++++++++++++++++++++++
arch/powerpc/kernel/ftrace.c | 64 +++++++++++++++++++++++++++++---
arch/powerpc/kernel/module_64.c | 25 ++++++++++++-
4 files changed, 165 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index ef89b14..50ca758 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -46,6 +46,8 @@
extern void _mcount(void);
#ifdef CONFIG_DYNAMIC_FTRACE
+# define FTRACE_ADDR ((unsigned long)ftrace_caller)
+# define FTRACE_REGS_ADDR FTRACE_ADDR
static inline unsigned long ftrace_call_adjust(unsigned long addr)
{
/* reloction of mcount call site is the same as the address */
@@ -58,6 +60,9 @@ struct dyn_arch_ftrace {
#endif /* CONFIG_DYNAMIC_FTRACE */
#endif /* __ASSEMBLY__ */
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
#endif
#if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_PPC64) && !defined(__ASSEMBLY__)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 2a7313c..c063564 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1148,6 +1148,7 @@ _GLOBAL(_mcount)
mtlr r0
bctr
+#ifndef CC_USING_MPROFILE_KERNEL
_GLOBAL_TOC(ftrace_caller)
/* Taken from output of objdump from lib64/glibc */
mflr r3
@@ -1169,6 +1170,83 @@ _GLOBAL(ftrace_graph_stub)
ld r0, 128(r1)
mtlr r0
addi r1, r1, 112
+#else
+_GLOBAL(ftrace_caller)
+ std r0,LRSAVE(r1)
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+ mflr r0
+ bl 2f
+2: mflr r12
+ mtlr r0
+ mr r0,r2 /* save callee's TOC */
+ addis r2,r12,(.TOC.-ftrace_caller-12)@ha
+ addi r2,r2,(.TOC.-ftrace_caller-12)@l
+#else
+ mr r0,r2
+#endif
+ ld r12,LRSAVE(r1) /* get caller's address */
+
+ stdu r1,-SWITCH_FRAME_SIZE(r1)
+
+ std r12, _LINK(r1)
+ SAVE_8GPRS(0,r1)
+ std r0, 24(r1) /* save TOC */
+ SAVE_8GPRS(8,r1)
+ SAVE_8GPRS(16,r1)
+ SAVE_8GPRS(24,r1)
+
+ addis r3,r2,function_trace_op@toc@ha
+ addi r3,r3,function_trace_op@toc@l
+ ld r5,0(r3)
+
+ mflr r3
+ std r3, _NIP(r1)
+ std r3, 16(r1)
+ subi r3, r3, MCOUNT_INSN_SIZE
+ mfmsr r4
+ std r4, _MSR(r1)
+ mfctr r4
+ std r4, _CTR(r1)
+ mfxer r4
+ std r4, _XER(r1)
+ mr r4, r12
+ addi r6, r1 ,STACK_FRAME_OVERHEAD
+
+.globl ftrace_call
+ftrace_call:
+ bl ftrace_stub
+ nop
+
+ ld r3, _NIP(r1)
+ mtlr r3
+
+ REST_8GPRS(0,r1)
+ REST_8GPRS(8,r1)
+ REST_8GPRS(16,r1)
+ REST_8GPRS(24,r1)
+
+ addi r1, r1, SWITCH_FRAME_SIZE
+
+ ld r12, LRSAVE(r1) /* get caller's address */
+ mtlr r12
+ mr r2,r0 /* restore callee's TOC */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ stdu r1, -112(r1)
+.globl ftrace_graph_call
+ftrace_graph_call:
+ b ftrace_graph_stub
+_GLOBAL(ftrace_graph_stub)
+ addi r1, r1, 112
+#endif
+
+ mflr r0 /* move this LR to CTR */
+ mtctr r0
+
+ ld r0,LRSAVE(r1) /* restore callee's lr at _mcount site */
+ mtlr r0
+ bctr /* jump after _mcount site */
+#endif /* CC_USING_MPROFILE_KERNEL */
_GLOBAL(ftrace_stub)
blr
#else
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 861af90..1fad1b3 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -61,8 +61,11 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
return -EFAULT;
/* Make sure it is what we expect it to be */
- if (replaced != old)
+ if (replaced != old) {
+ pr_err("%p: replaced (%#x) != old (%#x)",
+ (void *)ip, replaced, old);
return -EINVAL;
+ }
/* replace the text with the new text */
if (patch_instruction((unsigned int *)ip, new))
@@ -106,14 +109,16 @@ static int
__ftrace_make_nop(struct module *mod,
struct dyn_ftrace *rec, unsigned long addr)
{
- unsigned int op;
+ unsigned int op, op0, op1, pop;
unsigned long entry, ptr;
unsigned long ip = rec->ip;
void *tramp;
/* read where this goes */
- if (probe_kernel_read(&op, (void *)ip, sizeof(int)))
+ if (probe_kernel_read(&op, (void *)ip, sizeof(int))) {
+ pr_err("Fetching opcode failed.\n");
return -EFAULT;
+ }
/* Make sure that that this is still a 24bit jump */
if (!is_bl_op(op)) {
@@ -158,10 +163,49 @@ __ftrace_make_nop(struct module *mod,
*
* Use a b +8 to jump over the load.
*/
- op = 0x48000008; /* b +8 */
- if (patch_instruction((unsigned int *)ip, op))
+ pop = PPC_INST_BRANCH | 8; /* b +8 */
+
+ /*
+ * Check what is in the next instruction. We can see ld r2,40(r1), but
+ * on first pass after boot we will see mflr r0.
+ */
+ if (probe_kernel_read(&op, (void *)(ip+4), MCOUNT_INSN_SIZE)) {
+ pr_err("Fetching op failed.\n");
+ return -EFAULT;
+ }
+
+ if (op != PPC_INST_LD_TOC)
+ {
+ if (probe_kernel_read(&op0, (void *)(ip-8), MCOUNT_INSN_SIZE)) {
+ pr_err("Fetching op0 failed.\n");
+ return -EFAULT;
+ }
+
+ if (probe_kernel_read(&op1, (void *)(ip-4), MCOUNT_INSN_SIZE)) {
+ pr_err("Fetching op1 failed.\n");
+ return -EFAULT;
+ }
+
+ /* mflr r0 ; [ std r0,LRSAVE(r1) ]? */
+ if ( (op0 != PPC_INST_MFLR ||
+ op1 != PPC_INST_STD_LR)
+ && op1 != PPC_INST_MFLR )
+ {
+ pr_err("Unexpected instructions around bl _mcount\n"
+ "when enabling dynamic ftrace!\t"
+ "(%08x,%08x,bl,%08x)\n", op0, op1, op);
+ return -EINVAL;
+ }
+
+ /* When using -mkernel_profile there is no load to jump over */
+ pop = PPC_INST_NOP;
+ }
+
+ if (patch_instruction((unsigned int *)ip, pop)) {
+ pr_err("Patching NOP failed.\n");
return -EPERM;
+ }
return 0;
}
@@ -287,6 +331,14 @@ int ftrace_make_nop(struct module *mod,
#ifdef CONFIG_MODULES
#ifdef CONFIG_PPC64
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+ unsigned long addr)
+{
+ return ftrace_make_call(rec, addr);
+}
+#endif
+
/* Examine the existing instructions for __ftrace_make_call.
* They should effectively be a NOP, and follow formal constraints,
* depending on the ABI. Return false if they don't.
@@ -354,7 +406,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return 0;
}
-#else
+#else /* !CONFIG_PPC64: */
static int
__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 72a1a52..720e8fd 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -136,12 +136,25 @@ static u32 ppc64_stub_insns[] = {
0x4e800420 /* bctr */
};
+#ifdef CC_USING_MPROFILE_KERNEL
+/* In case of _mcount calls or dynamic ftracing, Do not save the
+ * current callee's TOC (in R2) again into the original caller's stack
+ * frame during this trampoline hop. The stack frame already holds
+ * that of the original caller. _mcount and ftrace_caller will take
+ * care of this TOC value themselves.
+ */
+#define SQUASH_TOC_SAVE_INSN(trampoline_addr) \
+ (((struct ppc64_stub_entry *)(trampoline_addr))->jump[2] = PPC_INST_NOP)
+#else
+#define SQUASH_TOC_SAVE_INSN(trampoline_addr)
+#endif
+
#ifdef CONFIG_DYNAMIC_FTRACE
static u32 ppc64_stub_mask[] = {
0xffff0000,
0xffff0000,
- 0xffffffff,
+ 0x00000000,
0xffffffff,
#if !defined(_CALL_ELF) || _CALL_ELF != 2
0xffffffff,
@@ -168,6 +181,9 @@ bool is_module_trampoline(u32 *p)
if ((insna & mask) != (insnb & mask))
return false;
}
+ if (insns[2] != ppc64_stub_insns[2] &&
+ insns[2] != PPC_INST_NOP)
+ return false;
return true;
}
@@ -636,6 +652,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
return -ENOENT;
if (!restore_r2((u32 *)location + 1, me))
return -ENOEXEC;
+ /* Squash the TOC saver for profiler calls */
+ if (!strcmp("_mcount", strtab+sym->st_name))
+ SQUASH_TOC_SAVE_INSN(value);
} else
value += local_entry_offset(sym);
@@ -723,6 +742,10 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
me->arch.tramp = stub_for_addr(sechdrs,
(unsigned long)ftrace_caller,
me);
+ /* ftrace_caller will take care of the TOC;
+ * do not clobber original caller's value.
+ */
+ SQUASH_TOC_SAVE_INSN(me->arch.tramp);
#endif
return 0;
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v8 3/8] ppc use ftrace_modify_all_code default
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
` (2 preceding siblings ...)
2016-02-10 16:22 ` [PATCH v8 2/8] ppc64le FTRACE_WITH_REGS implementation Torsten Duwe
@ 2016-02-10 16:22 ` Torsten Duwe
2016-02-10 16:25 ` [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe
` (4 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:22 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
Convert ppc's arch_ftrace_update_code from its own function copy
to use the generic default functionality (without stop_machine --
our instructions are properly aligned and the replacements atomic ;)
With this we gain error checking and the much-needed function_trace_op
handling.
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/kernel/ftrace.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 1fad1b3..ef8b916 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -531,20 +531,12 @@ void ftrace_replace_code(int enable)
}
}
+/* Use the default ftrace_modify_all_code, but without
+ * stop_machine().
+ */
void arch_ftrace_update_code(int command)
{
- if (command & FTRACE_UPDATE_CALLS)
- ftrace_replace_code(1);
- else if (command & FTRACE_DISABLE_CALLS)
- ftrace_replace_code(0);
-
- if (command & FTRACE_UPDATE_TRACE_FUNC)
- ftrace_update_ftrace_func(ftrace_trace_function);
-
- if (command & FTRACE_START_FUNC_RET)
- ftrace_enable_ftrace_graph_caller();
- else if (command & FTRACE_STOP_FUNC_RET)
- ftrace_disable_ftrace_graph_caller();
+ ftrace_modify_all_code(command);
}
int __init ftrace_dyn_arch_init(void)
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
` (3 preceding siblings ...)
2016-02-10 16:22 ` [PATCH v8 3/8] ppc use ftrace_modify_all_code default Torsten Duwe
@ 2016-02-10 16:25 ` Torsten Duwe
2016-02-11 7:48 ` Balbir Singh
2016-02-10 16:27 ` [PATCH v8 5/8] ppc64 ftrace_with_regs: disable profiling for some files Torsten Duwe
` (3 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:25 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
* arch/powerpc/Makefile:
- globally use -mprofile-kernel in case it's configured,
available and bug-free.
* arch/powerpc/gcc-mprofile-kernel-notrace.sh:
- make sure -mprofile-kernel works and has none of the
known bugs.
* arch/powerpc/kernel/ftrace.c:
- error out on compile with HAVE_DYNAMIC_FTRACE_WITH_REGS
and a buggy compiler.
* arch/powerpc/Kconfig / kernel/trace/Kconfig:
- declare that ppc64le HAVE_MPROFILE_KERNEL and
HAVE_DYNAMIC_FTRACE_WITH_REGS, and use it.
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/Kconfig | 2 ++
arch/powerpc/Makefile | 17 +++++++++++++++
arch/powerpc/gcc-mprofile-kernel-notrace.sh | 33 +++++++++++++++++++++++++++++
arch/powerpc/kernel/ftrace.c | 5 +++++
kernel/trace/Kconfig | 5 +++++
5 files changed, 62 insertions(+)
create mode 100755 arch/powerpc/gcc-mprofile-kernel-notrace.sh
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e4824fd..e5f288c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -94,8 +94,10 @@ config PPC
select OF_RESERVED_MEM
select HAVE_FTRACE_MCOUNT_RECORD
select HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_REGS if PPC64 && CPU_LITTLE_ENDIAN
select HAVE_FUNCTION_TRACER
select HAVE_FUNCTION_GRAPH_TRACER
+ select HAVE_MPROFILE_KERNEL if PPC64 && CPU_LITTLE_ENDIAN
select SYSCTL_EXCEPTION_TRACE
select ARCH_WANT_OPTIONAL_GPIOLIB
select VIRT_TO_BUS if !PPC64
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 96efd82..08a3952 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -133,6 +133,23 @@ else
CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
endif
+ifeq ($(CONFIG_PPC64),y)
+ifdef CONFIG_HAVE_MPROFILE_KERNEL
+
+ifdef CONFIG_DYNAMIC_FTRACE
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/arch/powerpc/gcc-mprofile-kernel-notrace.sh $(CC) -I$(srctree)/include -D__KERNEL__), y)
+CC_USING_MPROFILE_KERNEL := -mprofile-kernel
+endif
+endif
+
+ifdef CC_USING_MPROFILE_KERNEL
+CC_FLAGS_FTRACE := -pg $(CC_USING_MPROFILE_KERNEL)
+KBUILD_CPPFLAGS += -DCC_USING_MPROFILE_KERNEL
+endif
+
+endif
+endif
+
CFLAGS-$(CONFIG_CELL_CPU) += $(call cc-option,-mcpu=cell)
CFLAGS-$(CONFIG_POWER4_CPU) += $(call cc-option,-mcpu=power4)
CFLAGS-$(CONFIG_POWER5_CPU) += $(call cc-option,-mcpu=power5)
diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
new file mode 100755
index 0000000..68d6482
--- /dev/null
+++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+# Test whether the compile option -mprofile-kernel
+# generates profiling code ( = a call to mcount), and
+# whether a function without any global references sets
+# the TOC pointer properly at the beginning, and
+# whether the "notrace" function attribute successfully
+# suppresses the _mcount call.
+
+echo "int func() { return 0; }" | \
+ $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+ grep -q "mcount"
+
+trace_result=$?
+
+echo "int func() { return 0; }" | \
+ $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+ sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
+
+leaf_toc_result=$?
+
+/bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \
+ $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+ grep -q "mcount"
+
+notrace_result=$?
+
+if [ "$trace_result" -eq "0" -a \
+ "$leaf_toc_result" -eq "0" -a \
+ "$notrace_result" -eq "1" ]; then
+ echo y
+else
+ echo n
+fi
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index ef8b916..29b7014 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -28,6 +28,11 @@
#ifdef CONFIG_DYNAMIC_FTRACE
+#if defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && defined(CONFIG_PPC64) && \
+ !defined(CC_USING_MPROFILE_KERNEL)
+#error "DYNAMIC_FTRACE_WITH_REGS requires working -mprofile-kernel"
+#endif
+
static unsigned int
ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
{
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index e45db6b..a138f6d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -52,6 +52,11 @@ config HAVE_FENTRY
help
Arch supports the gcc options -pg with -mfentry
+config HAVE_MPROFILE_KERNEL
+ bool
+ help
+ Arch supports the gcc options -pg with -mprofile-kernel
+
config HAVE_C_RECORDMCOUNT
bool
help
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-10 16:25 ` [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe
@ 2016-02-11 7:48 ` Balbir Singh
2016-02-11 8:39 ` Kamalesh Babulal
2016-02-11 8:42 ` Torsten Duwe
0 siblings, 2 replies; 44+ messages in thread
From: Balbir Singh @ 2016-02-11 7:48 UTC (permalink / raw)
To: Torsten Duwe, Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
snip
> diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> new file mode 100755
> index 0000000..68d6482
> --- /dev/null
> +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +# Test whether the compile option -mprofile-kernel
> +# generates profiling code ( = a call to mcount), and
> +# whether a function without any global references sets
> +# the TOC pointer properly at the beginning, and
> +# whether the "notrace" function attribute successfully
> +# suppresses the _mcount call.
> +
> +echo "int func() { return 0; }" | \
> + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> + grep -q "mcount"
> +
> +trace_result=$?
> +
> +echo "int func() { return 0; }" | \
> + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> +
> +leaf_toc_result=$?
> +
leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
and give the patches a spin
> +/bin/echo -e "#include \nnotrace int func() { return 0; }" | \
> + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> + grep -q "mcount"
> +
> +notrace_result=$?
> +
> +if [ "$trace_result" -eq "0" -a \
> + "$leaf_toc_result" -eq "0" -a \
> + "$notrace_result" -eq "1" ]; then
> + echo y
> +else
> + echo n
> +fi
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 7:48 ` Balbir Singh
@ 2016-02-11 8:39 ` Kamalesh Babulal
2016-02-11 9:35 ` Balbir Singh
2016-02-11 8:42 ` Torsten Duwe
1 sibling, 1 reply; 44+ messages in thread
From: Kamalesh Babulal @ 2016-02-11 8:39 UTC (permalink / raw)
To: Balbir Singh
Cc: Torsten Duwe, Michael Ellerman, Jiri Kosina, Miroslav Benes,
Petr Mladek, Jessica Yu, Steven Rostedt, linuxppc-dev,
linux-kernel, live-patching
* Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
> On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
>
> snip
>
> > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > new file mode 100755
> > index 0000000..68d6482
> > --- /dev/null
> > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > @@ -0,0 +1,33 @@
> > +#!/bin/sh
> > +# Test whether the compile option -mprofile-kernel
> > +# generates profiling code ( = a call to mcount), and
> > +# whether a function without any global references sets
> > +# the TOC pointer properly at the beginning, and
> > +# whether the "notrace" function attribute successfully
> > +# suppresses the _mcount call.
> > +
> > +echo "int func() { return 0; }" | \
> > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > + grep -q "mcount"
> > +
> > +trace_result=$?
> > +
> > +echo "int func() { return 0; }" | \
> > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > +
> > +leaf_toc_result=$?
> > +
>
> leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> and give the patches a spin
>
It fails for me to on ppc64le but pass over ppc64
# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/ppc64-redhat-linux/5.1.1/lto-wrapper
Target: ppc64-redhat-linux
[...]
gcc version 5.1.1 20150422 (Red Hat 5.1.1-1) (GCC)
# echo "int func() { return 0; }" | gcc -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | sed -n -e '/func:/,/bl _mcount/p'
func:
.quad .L.func,.TOC.@tocbase
.previous
.type func, @function
.L.func:
mflr 0
std 0,16(1)
bl _mcount
# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/powerpc64le-linux-gnu/5/lto-wrapper
Target: powerpc64le-linux-gnu
[...]
gcc version 5.3.1 20160205 (Ubuntu/IBM 5.3.1-8ubuntu2)
# echo "int func() { return 0; }" | gcc -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | sed -n -e '/func:/,/bl _mcount/p'
func:
mflr 0
std 0,16(1)
bl _mcount
I will try it over gcc-6.
Thanks,
Kamalesh
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 8:39 ` Kamalesh Babulal
@ 2016-02-11 9:35 ` Balbir Singh
2016-02-11 13:00 ` Murali Sampath
2016-02-11 13:00 ` Murali Sampath
0 siblings, 2 replies; 44+ messages in thread
From: Balbir Singh @ 2016-02-11 9:35 UTC (permalink / raw)
To: Kamalesh Babulal
Cc: Torsten Duwe, Michael Ellerman, Jiri Kosina, Miroslav Benes,
Petr Mladek, Jessica Yu, Steven Rostedt, linuxppc-dev,
linux-kernel, live-patching
On Thu, 2016-02-11 at 14:09 +0530, Kamalesh Babulal wrote:
> * Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
>
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> >
> > snip
> >
> > > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > new file mode 100755
> > > index 0000000..68d6482
> > > --- /dev/null
> > > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > @@ -0,0 +1,33 @@
> > > +#!/bin/sh
> > > +# Test whether the compile option -mprofile-kernel
> > > +# generates profiling code ( = a call to mcount), and
> > > +# whether a function without any global references sets
> > > +# the TOC pointer properly at the beginning, and
> > > +# whether the "notrace" function attribute successfully
> > > +# suppresses the _mcount call.
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + grep -q "mcount"
> > > +
> > > +trace_result=$?
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > > +
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> >
>
> It fails for me to on ppc64le but pass over ppc64
>
This series is for ppc64le only, so we can safely ignore ppc64 for now
Regards,
Balbir Singh.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 9:35 ` Balbir Singh
@ 2016-02-11 13:00 ` Murali Sampath
2016-02-11 13:00 ` Murali Sampath
1 sibling, 0 replies; 44+ messages in thread
From: Murali Sampath @ 2016-02-11 13:00 UTC (permalink / raw)
To: Balbir Singh, Kamalesh Babulal
Cc: Petr Mladek, Jessica Yu, linux-kernel, Steven Rostedt,
Torsten Duwe, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
Original Message
From: Balbir Singh
Sent: Thursday, February 11, 2016 4:37 AM
To: Kamalesh Babulal
Cc: Petr Mladek; Jessica Yu; linux-kernel@vger.kernel.org; Steven Rostedt; =
Torsten Duwe; Jiri Kosina; live-patching@vger.kernel.org; Miroslav Benes; l=
inuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
On Thu, 2016-02-11 at 14:09 +0530, Kamalesh Babulal wrote:
> * Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
>
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> >
> > snip
> >
> > > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > new file mode 100755
> > > index 0000000..68d6482
> > > --- /dev/null
> > > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > @@ -0,0 +1,33 @@
> > > +#!/bin/sh
> > > +# Test whether the compile option -mprofile-kernel
> > > +# generates profiling code ( =3D a call to mcount), and
> > > +# whether a function without any global references sets
> > > +# the TOC pointer properly at the beginning, and
> > > +# whether the "notrace" function attribute successfully
> > > +# suppresses the _mcount call.
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + grep -q "mcount"
> > > +
> > > +trace_result=3D$?
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=3D$?
> > > +
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> >
>
> It fails for me to on ppc64le but pass over ppc64
>
This series is for ppc64le only, so we can safely ignore ppc64 for now
Regards,
Balbir Singh.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 9:35 ` Balbir Singh
2016-02-11 13:00 ` Murali Sampath
@ 2016-02-11 13:00 ` Murali Sampath
1 sibling, 0 replies; 44+ messages in thread
From: Murali Sampath @ 2016-02-11 13:00 UTC (permalink / raw)
To: Balbir Singh, Kamalesh Babulal
Cc: Petr Mladek, Jessica Yu, linux-kernel, Steven Rostedt,
Torsten Duwe, Jiri Kosina, live-patching, Miroslav Benes,
linuxppc-dev
=FD t
Original Message
From: Balbir Singh
Sent: Thursday, February 11, 2016 4:37 AM
To: Kamalesh Babulal
Cc: Petr Mladek; Jessica Yu; linux-kernel@vger.kernel.org; Steven Rostedt; =
Torsten Duwe; Jiri Kosina; live-patching@vger.kernel.org; Miroslav Benes; l=
inuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
On Thu, 2016-02-11 at 14:09 +0530, Kamalesh Babulal wrote:
> * Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
>
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> >
> > snip
> >
> > > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > new file mode 100755
> > > index 0000000..68d6482
> > > --- /dev/null
> > > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > @@ -0,0 +1,33 @@
> > > +#!/bin/sh
> > > +# Test whether the compile option -mprofile-kernel
> > > +# generates profiling code ( =3D a call to mcount), and
> > > +# whether a function without any global references sets
> > > +# the TOC pointer properly at the beginning, and
> > > +# whether the "notrace" function attribute successfully
> > > +# suppresses the _mcount call.
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + grep -q "mcount"
> > > +
> > > +trace_result=3D$?
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=3D$?
> > > +
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> >
>
> It fails for me to on ppc64le but pass over ppc64
>
This series is for ppc64le only, so we can safely ignore ppc64 for now
Regards,
Balbir Singh.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 7:48 ` Balbir Singh
2016-02-11 8:39 ` Kamalesh Babulal
@ 2016-02-11 8:42 ` Torsten Duwe
2016-02-11 9:34 ` Balbir Singh
2016-02-15 10:27 ` Michael Ellerman
1 sibling, 2 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-11 8:42 UTC (permalink / raw)
To: Balbir Singh
Cc: Michael Ellerman, Jiri Kosina, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Thu, Feb 11, 2016 at 06:48:17PM +1100, Balbir Singh wrote:
> On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > +
> > +echo "int func() { return 0; }" | \
> > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > +
> > +leaf_toc_result=$?
> > +
>
> leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> and give the patches a spin
Don't bother. _All_ gccs are broken in that respect currently.
AFAIK Anton is working on this. You have to fake the test, like
static int a; return a++;
Gcc fails to set the TOC for profiled "leaf" functions, where it
thinks no global/static symbols are referenced.
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 8:42 ` Torsten Duwe
@ 2016-02-11 9:34 ` Balbir Singh
2016-02-15 10:27 ` Michael Ellerman
1 sibling, 0 replies; 44+ messages in thread
From: Balbir Singh @ 2016-02-11 9:34 UTC (permalink / raw)
To: Torsten Duwe
Cc: Michael Ellerman, Jiri Kosina, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Thu, 2016-02-11 at 09:42 +0100, Torsten Duwe wrote:
> On Thu, Feb 11, 2016 at 06:48:17PM +1100, Balbir Singh wrote:
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > > +
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
>
> Don't bother. _All_ gccs are broken in that respect currently.
> AFAIK Anton is working on this. You have to fake the test, like
> static int a; return a++;
>
> Gcc fails to set the TOC for profiled "leaf" functions, where it
> thinks no global/static symbols are referenced.
>
>
Thanks for the quick answer
BTW, do we expect static/non global functions to be called from
patched contexts? I understand that not supporting it will
complicate a patch.
Balbir Singh
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-11 8:42 ` Torsten Duwe
2016-02-11 9:34 ` Balbir Singh
@ 2016-02-15 10:27 ` Michael Ellerman
2016-02-15 12:56 ` Jiri Kosina
2016-02-15 14:04 ` Torsten Duwe
1 sibling, 2 replies; 44+ messages in thread
From: Michael Ellerman @ 2016-02-15 10:27 UTC (permalink / raw)
To: Torsten Duwe, Balbir Singh
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
Hi guys,
Sorry I haven't been keeping up to date with this thread I've been away.
On Thu, 2016-02-11 at 09:42 +0100, Torsten Duwe wrote:
> On Thu, Feb 11, 2016 at 06:48:17PM +1100, Balbir Singh wrote:
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > > +
> > > +echo "int func() { return 0; }" | \
> > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
>
> Don't bother. _All_ gccs are broken in that respect currently.
I'm not sure where we got our wires crossed on this one, but this is not a gcc
bug. In fact it's a feature :)
There is explicit code in gcc to check whether the TOC setup is needed and only
emit it when it's required. One case where it's *not* required is when the
function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).
So I think this is just one of the "fun" details we have to deal with to
implement kernel tracing.
The first implication of this is that there is not a single offset value for
the _mcount call, it needs to be calculated per-function. In practice there
will only be two values, so we could try both, or we could just have the code
work it out dynamically by looking for the expected code sequence?
Secondly it means the ftrace trampoline needs to cope with being called with r2
containing something other than the kernel TOC (ie. a module's TOC pointer).
But I think that's solvable also?
cheers
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-15 10:27 ` Michael Ellerman
@ 2016-02-15 12:56 ` Jiri Kosina
2016-02-15 14:04 ` Torsten Duwe
1 sibling, 0 replies; 44+ messages in thread
From: Jiri Kosina @ 2016-02-15 12:56 UTC (permalink / raw)
To: Michael Ellerman
Cc: Torsten Duwe, Balbir Singh, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Mon, 15 Feb 2016, Michael Ellerman wrote:
> > > > +echo "int func() { return 0; }" | \
> > > > + $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > > + sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > > +
> > > > +leaf_toc_result=$?
> > >
> > > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > > and give the patches a spin
> >
> > Don't bother. _All_ gccs are broken in that respect currently.
>
> I'm not sure where we got our wires crossed on this one, but this is not a gcc
> bug. In fact it's a feature :)
>
> There is explicit code in gcc to check whether the TOC setup is needed and only
> emit it when it's required. One case where it's *not* required is when the
> function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).
As gcc actually implements a '-mprofile-kernel' option, it's pretty much
aware of the fact that the inserted space will be used by the kernel for
inserting a call (as that's the sole point of the whole thing).
Therefore it must not consider any traceable function to be leaf (even
though it might "look" leaf from the source code); if it does, then the
mprofile-kernel option is useless.
So I actually would dare to call it a bug.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-15 10:27 ` Michael Ellerman
2016-02-15 12:56 ` Jiri Kosina
@ 2016-02-15 14:04 ` Torsten Duwe
2016-02-15 22:21 ` Torsten Duwe
2016-02-16 10:09 ` Michael Ellerman
1 sibling, 2 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-15 14:04 UTC (permalink / raw)
To: Michael Ellerman
Cc: Balbir Singh, Jiri Kosina, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Mon, Feb 15, 2016 at 09:27:15PM +1100, Michael Ellerman wrote:
>
> There is explicit code in gcc to check whether the TOC setup is needed and only
That's undestood. The claim here is: that check is incomplete, at least.
> emit it when it's required. One case where it's *not* required is when the
> function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).
n.b. I cannot find this symbol in the 4.9.3 tree, but I know what you mean.
The point here is: If you profile using "-pg", gcc perfectly recognises that
it is generating a call to "_mcount", which may be non-local, and loads the TOC.
If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
load, for a similar assembler calling sequence.
Looking at the code I can _understand_ why this is so, but my GCC knowledge
is not that deep that I could easily _fix_ this reliably.
[...]
> Secondly it means the ftrace trampoline needs to cope with being called with r2
> containing something other than the kernel TOC (ie. a module's TOC pointer).
> But I think that's solvable also?
That was the alternative I asked about; but given that the _mcount / ftrace_caller
trampoline hardly differs from a normal trampoline (so far), loading R2 would be the
general case, or an excessive special case handling would result.
I'd rather see gcc getting fixed than to have workarounds in all projects that use
it, and others agreed.
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-15 14:04 ` Torsten Duwe
@ 2016-02-15 22:21 ` Torsten Duwe
2016-02-16 4:53 ` Balbir Singh
2016-02-16 10:09 ` Michael Ellerman
1 sibling, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-15 22:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: Balbir Singh, Jiri Kosina, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Mon, Feb 15, 2016 at 03:04:08PM +0100, Torsten Duwe wrote:
> If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> load, for a similar assembler calling sequence.
>
> Looking at the code I can _understand_ why this is so, but my GCC knowledge
> is not that deep that I could easily _fix_ this reliably.
Nonetheless, here's a proposal.
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -25154,6 +25154,10 @@ rs6000_emit_prologue (void)
{
cfun->machine->r2_setup_needed = df_regs_ever_live_p (TOC_REGNUM);
+ /* Profiling _will_ generate a call to a global _mcount. */
+ if (crtl->profile)
+ cfun->machine->r2_setup_needed = true;
+
/* With -mminimal-toc we may generate an extra use of r2 below. */
if (TARGET_TOC && TARGET_MINIMAL_TOC && get_pool_size () != 0)
cfun->machine->r2_setup_needed = true;
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-15 22:21 ` Torsten Duwe
@ 2016-02-16 4:53 ` Balbir Singh
0 siblings, 0 replies; 44+ messages in thread
From: Balbir Singh @ 2016-02-16 4:53 UTC (permalink / raw)
To: Torsten Duwe, Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Mon, 2016-02-15 at 23:21 +0100, Torsten Duwe wrote:
> On Mon, Feb 15, 2016 at 03:04:08PM +0100, Torsten Duwe wrote:
> > If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> > load, for a similar assembler calling sequence.
> >
> > Looking at the code I can _understand_ why this is so, but my GCC knowledge
> > is not that deep that I could easily _fix_ this reliably.
>
> Nonetheless, here's a proposal.
>
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -25154,6 +25154,10 @@ rs6000_emit_prologue (void)
> {
> cfun->machine->r2_setup_needed = df_regs_ever_live_p (TOC_REGNUM);
>
> + /* Profiling _will_ generate a call to a global _mcount. */
> + if (crtl->profile)
> + cfun->machine->r2_setup_needed = true;
> +
> /* With -mminimal-toc we may generate an extra use of r2 below. */
> if (TARGET_TOC && TARGET_MINIMAL_TOC && get_pool_size () != 0)
> cfun->machine->r2_setup_needed = true;
I was wondering if ftrace() works correctly, specifically dynamic ftrace.
We should be able to get livepatching to work as well. I can see r2 being
saved in the stub in ftrace_caller in patch 2/8
+ mr r0,r2 /* save callee's TOC */
+ addis r2,r12,(.TOC.-ftrace_caller-12)@ha
+ addi r2,r2,(.TOC.-ftrace_caller-12)@l
The only limitation today is figuring out the correct offset to patch
(8 or 16) depending on whether the TOC stub is generated or not by the
compiler
If the sequence is well known, we could potentially scan instructions
or go to the hash that ftrace maintains and search in there with an offset
of 8 to 16.
Balbir Singh
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-15 14:04 ` Torsten Duwe
2016-02-15 22:21 ` Torsten Duwe
@ 2016-02-16 10:09 ` Michael Ellerman
2016-02-16 10:35 ` Torsten Duwe
1 sibling, 1 reply; 44+ messages in thread
From: Michael Ellerman @ 2016-02-16 10:09 UTC (permalink / raw)
To: Torsten Duwe
Cc: Balbir Singh, Jiri Kosina, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Mon, 2016-02-15 at 15:04 +0100, Torsten Duwe wrote:
> On Mon, Feb 15, 2016 at 09:27:15PM +1100, Michael Ellerman wrote:
> >
> > There is explicit code in gcc to check whether the TOC setup is needed and only
>
> That's undestood. The claim here is: that check is incomplete, at least.
OK at least we agree on what's happening.
> > emit it when it's required. One case where it's *not* required is when the
> > function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).
>
> n.b. I cannot find this symbol in the 4.9.3 tree, but I know what you mean.
>
> The point here is: If you profile using "-pg", gcc perfectly recognises that
> it is generating a call to "_mcount", which may be non-local, and loads the TOC.
> If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> load, for a similar assembler calling sequence.
That's by design.
mprofile-kernel is supposed to create as little overhead as possible in the
non-traced case. All of the burden is shifted to the trace function (_mcount).
The reason to do that is because modern distros always build with tracing, but
most of the time tracing will not actually be active. So we want the cost of
tracing-built-in-but-disabled to be ~zero.
> > Secondly it means the ftrace trampoline needs to cope with being called with r2
> > containing something other than the kernel TOC (ie. a module's TOC pointer).
> > But I think that's solvable also?
>
> That was the alternative I asked about; but given that the _mcount / ftrace_caller
> trampoline hardly differs from a normal trampoline (so far), loading R2 would be the
> general case, or an excessive special case handling would result.
I'm not sure I follow what you mean there at the end.
Requiring ftrace_caller() to load the kernel TOC is not a problem IMHO.
I think I have an easier way to do it, I'll reply to the patch with that (if it
works).
cheers
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables
2016-02-16 10:09 ` Michael Ellerman
@ 2016-02-16 10:35 ` Torsten Duwe
0 siblings, 0 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-16 10:35 UTC (permalink / raw)
To: Michael Ellerman
Cc: Balbir Singh, Jiri Kosina, Miroslav Benes, Petr Mladek,
Jessica Yu, Steven Rostedt, linuxppc-dev, linux-kernel,
live-patching
On Tue, Feb 16, 2016 at 09:09:16PM +1100, Michael Ellerman wrote:
> On Mon, 2016-02-15 at 15:04 +0100, Torsten Duwe wrote:
> > If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> > load, for a similar assembler calling sequence.
>
> That's by design.
Ah, ok.
> mprofile-kernel is supposed to create as little overhead as possible in the
> non-traced case. All of the burden is shifted to the trace function (_mcount).
... or its helpers, see below.
> The reason to do that is because modern distros always build with tracing, but
> most of the time tracing will not actually be active. So we want the cost of
> tracing-built-in-but-disabled to be ~zero.
Ok, that's a design goal.
> > That was the alternative I asked about; but given that the _mcount / ftrace_caller
> > trampoline hardly differs from a normal trampoline (so far), loading R2 would be the
> > general case, or an excessive special case handling would result.
>
> I'm not sure I follow what you mean there at the end.
This suggests you have not yet actively debugged this problem ;-)
> Requiring ftrace_caller() to load the kernel TOC is not a problem IMHO.
The problem is, you don't get to ftrace_caller in the first place :)
> I think I have an easier way to do it, I'll reply to the patch with that (if it
> works).
I doubt so. Either it works, _or_ it is easier ;)
To save you some work: by the design of minimal overhead you try to follow,
SQUASH_TOC_SAVE_INSN from my patches isn't sufficient. You'll need to load the
_current_ TOC _on_ the trampoline, and in turn it will be different from the regular
trampolines; and that needs to be recognised, or the normal module linker logic
won't work.
OTOH my proposed GCC change only affects a very limited number of functions...
Looking forward to your patch!
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v8 5/8] ppc64 ftrace_with_regs: disable profiling for some files
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
` (4 preceding siblings ...)
2016-02-10 16:25 ` [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe
@ 2016-02-10 16:27 ` Torsten Duwe
2016-02-10 16:34 ` [PATCH v8 6/8] Implement kernel live patching for ppc64le (ABIv2) Torsten Duwe
` (2 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:27 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
Using -mprofile-kernel on early boot code not only confuses the
checker but is also useless, as the infrastructure is not yet in
place. Proceed like with -pg (remove it from CFLAGS), equally with
time.o, ftrace and its helper files.
* arch/powerpc/kernel/Makefile,
arch/powerpc/lib/Makefile:
- remove -mprofile-kernel from low level, boot code and
code-patching objects' CFLAGS.
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/kernel/Makefile | 12 ++++++------
arch/powerpc/lib/Makefile | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 794f22a..44667fd 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -16,14 +16,14 @@ endif
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
-CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog -mprofile-kernel
# do not trace tracer code
-CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog -mprofile-kernel
# timers used by tracing
-CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog -mprofile-kernel
endif
obj-y := cputable.o ptrace.o syscalls.o \
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index a47e142..98e22b2 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -6,8 +6,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
-CFLAGS_REMOVE_code-patching.o = -pg
-CFLAGS_REMOVE_feature-fixups.o = -pg
+CFLAGS_REMOVE_code-patching.o = -pg -mprofile-kernel
+CFLAGS_REMOVE_feature-fixups.o = -pg -mprofile-kernel
obj-y += string.o alloc.o crtsavres.o ppc_ksyms.o code-patching.o \
feature-fixups.o
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v8 6/8] Implement kernel live patching for ppc64le (ABIv2)
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
` (5 preceding siblings ...)
2016-02-10 16:27 ` [PATCH v8 5/8] ppc64 ftrace_with_regs: disable profiling for some files Torsten Duwe
@ 2016-02-10 16:34 ` Torsten Duwe
2016-02-11 9:01 ` Miroslav Benes
2016-02-10 16:36 ` [PATCH v8 7/8] Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it is selected Torsten Duwe
2016-02-11 6:18 ` [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Balbir Singh
8 siblings, 1 reply; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:34 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
* create the appropriate files+functions
arch/powerpc/include/asm/livepatch.h
klp_check_compiler_support,
klp_arch_set_pc
arch/powerpc/kernel/livepatch.c with a stub for
klp_write_module_reloc
This is architecture-independent work in progress.
* introduce a fixup in arch/powerpc/kernel/entry_64.S
for local calls that are becoming global due to live patching.
And of course do the main KLP thing: return to a maybe different
address, possibly altered by the live patching ftrace op.
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/include/asm/livepatch.h | 45 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/entry_64.S | 51 +++++++++++++++++++++++++++++++++---
arch/powerpc/kernel/livepatch.c | 38 +++++++++++++++++++++++++++
3 files changed, 130 insertions(+), 4 deletions(-)
create mode 100644 arch/powerpc/include/asm/livepatch.h
create mode 100644 arch/powerpc/kernel/livepatch.c
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
new file mode 100644
index 0000000..44e8a2d
--- /dev/null
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -0,0 +1,45 @@
+/*
+ * livepatch.h - powerpc-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2015 SUSE
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _ASM_POWERPC64_LIVEPATCH_H
+#define _ASM_POWERPC64_LIVEPATCH_H
+
+#include <linux/module.h>
+#include <linux/ftrace.h>
+
+#ifdef CONFIG_LIVEPATCH
+static inline int klp_check_compiler_support(void)
+{
+#if !defined(_CALL_ELF) || _CALL_ELF != 2 || !defined(CC_USING_MPROFILE_KERNEL)
+ return 1;
+#endif
+ return 0;
+}
+
+extern int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value);
+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+ regs->nip = ip;
+}
+#else
+#error Live patching support is disabled; check CONFIG_LIVEPATCH
+#endif
+
+#endif /* _ASM_POWERPC64_LIVEPATCH_H */
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index c063564..52c7a15 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1202,6 +1202,9 @@ _GLOBAL(ftrace_caller)
mflr r3
std r3, _NIP(r1)
std r3, 16(r1)
+#ifdef CONFIG_LIVEPATCH
+ mr r14,r3 /* remember old NIP */
+#endif
subi r3, r3, MCOUNT_INSN_SIZE
mfmsr r4
std r4, _MSR(r1)
@@ -1218,7 +1221,10 @@ ftrace_call:
nop
ld r3, _NIP(r1)
- mtlr r3
+ mtctr r3 /* prepare to jump there */
+#ifdef CONFIG_LIVEPATCH
+ cmpd r14,r3 /* has NIP been altered? */
+#endif
REST_8GPRS(0,r1)
REST_8GPRS(8,r1)
@@ -1231,6 +1237,27 @@ ftrace_call:
mtlr r12
mr r2,r0 /* restore callee's TOC */
+#ifdef CONFIG_LIVEPATCH
+ beq+ 4f /* likely(old_NIP == new_NIP) */
+
+ /* For a local call, restore this TOC after calling the patch function.
+ * For a global call, it does not matter what we restore here,
+ * since the global caller does its own restore right afterwards,
+ * anyway. Just insert a KLP_return_helper frame in any case,
+ * so a patch function can always count on the changed stack offsets.
+ */
+ stdu r1,-32(r1) /* open new mini stack frame */
+ std r0,24(r1) /* save TOC now, unconditionally. */
+ bl 5f
+5: mflr r12
+ addi r12,r12,(KLP_return_helper+4-.)@l
+ std r12,LRSAVE(r1)
+ mtlr r12
+ mfctr r12 /* allow for TOC calculation in newfunc */
+ bctr
+4:
+#endif
+
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
stdu r1, -112(r1)
.globl ftrace_graph_call
@@ -1240,15 +1267,31 @@ _GLOBAL(ftrace_graph_stub)
addi r1, r1, 112
#endif
- mflr r0 /* move this LR to CTR */
- mtctr r0
-
ld r0,LRSAVE(r1) /* restore callee's lr at _mcount site */
mtlr r0
bctr /* jump after _mcount site */
#endif /* CC_USING_MPROFILE_KERNEL */
_GLOBAL(ftrace_stub)
blr
+
+#ifdef CONFIG_LIVEPATCH
+/* Helper function for local calls that are becoming global
+ due to live patching.
+ We can't simply patch the NOP after the original call,
+ because, depending on the consistency model, some kernel
+ threads may still have called the original, local function
+ *without* saving their TOC in the respective stack frame slot,
+ so the decision is made per-thread during function return by
+ maybe inserting a KLP_return_helper frame or not.
+*/
+KLP_return_helper:
+ ld r2,24(r1) /* restore TOC (saved by ftrace_caller) */
+ addi r1, r1, 32 /* destroy mini stack frame */
+ ld r0,LRSAVE(r1) /* get the real return address */
+ mtlr r0
+ blr
+#endif
+
#else
_GLOBAL_TOC(_mcount)
/* Taken from output of objdump from lib64/glibc */
diff --git a/arch/powerpc/kernel/livepatch.c b/arch/powerpc/kernel/livepatch.c
new file mode 100644
index 0000000..cdd15f1
--- /dev/null
+++ b/arch/powerpc/kernel/livepatch.c
@@ -0,0 +1,38 @@
+/*
+ * livepatch.c - powerpc-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2015 SUSE
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/module.h>
+#include <asm/livepatch.h>
+
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod: module in which the section to be modified is found
+ * @type: ELF relocation type (see asm/elf.h)
+ * @loc: address that the relocation should be written to
+ * @value: relocation value (sym address + addend)
+ *
+ * This function writes a relocation to the specified location for
+ * a particular module.
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ /* This requires infrastructure changes; we need the loadinfos. */
+ pr_err("klp_write_module_reloc not yet supported\n");
+ return -ENOSYS;
+}
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v8 6/8] Implement kernel live patching for ppc64le (ABIv2)
2016-02-10 16:34 ` [PATCH v8 6/8] Implement kernel live patching for ppc64le (ABIv2) Torsten Duwe
@ 2016-02-11 9:01 ` Miroslav Benes
0 siblings, 0 replies; 44+ messages in thread
From: Miroslav Benes @ 2016-02-11 9:01 UTC (permalink / raw)
To: Torsten Duwe
Cc: Michael Ellerman, Jiri Kosina, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
On Wed, 10 Feb 2016, Torsten Duwe wrote:
> diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
> new file mode 100644
> index 0000000..44e8a2d
> --- /dev/null
> +++ b/arch/powerpc/include/asm/livepatch.h
> @@ -0,0 +1,45 @@
> +/*
> + * livepatch.h - powerpc-specific Kernel Live Patching Core
> + *
> + * Copyright (C) 2015 SUSE
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef _ASM_POWERPC64_LIVEPATCH_H
> +#define _ASM_POWERPC64_LIVEPATCH_H
> +
> +#include <linux/module.h>
> +#include <linux/ftrace.h>
> +
> +#ifdef CONFIG_LIVEPATCH
> +static inline int klp_check_compiler_support(void)
> +{
> +#if !defined(_CALL_ELF) || _CALL_ELF != 2 || !defined(CC_USING_MPROFILE_KERNEL)
> + return 1;
> +#endif
> + return 0;
> +}
> +
> +extern int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value);
It would be better to make this one static inline and move 'return
-ENOSYS;' here. Thus there would be no arch/powerpc/kernel/livepatch.c.
See s390 code for reference.
> +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> +{
> + regs->nip = ip;
> +}
> +#else
> +#error Live patching support is disabled; check CONFIG_LIVEPATCH
Change the error to
#error Include linux/livepatch.h, not asm/livepatch.h
please. See 383bf44d1a8b ("livepatch: change the error message in
asm/livepatch.h header files").
Thanks Torsten,
Miroslav
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v8 7/8] Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it is selected
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
` (6 preceding siblings ...)
2016-02-10 16:34 ` [PATCH v8 6/8] Implement kernel live patching for ppc64le (ABIv2) Torsten Duwe
@ 2016-02-10 16:36 ` Torsten Duwe
2016-02-11 6:18 ` [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Balbir Singh
8 siblings, 0 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-10 16:36 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jiri Kosina, Miroslav Benes, Petr Mladek, Jessica Yu,
Steven Rostedt, linuxppc-dev, linux-kernel, live-patching
Signed-off-by: Torsten Duwe <duwe@suse.de>
---
arch/powerpc/Kconfig | 3 +++
arch/powerpc/kernel/Makefile | 1 +
2 files changed, 4 insertions(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e5f288c..8c7a327 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -160,6 +160,7 @@ config PPC
select ARCH_HAS_DEVMEM_IS_ALLOWED
select HAVE_ARCH_SECCOMP_FILTER
select ARCH_HAS_UBSAN_SANITIZE_ALL
+ select HAVE_LIVEPATCH if PPC64 && CPU_LITTLE_ENDIAN
config GENERIC_CSUM
def_bool CPU_LITTLE_ENDIAN
@@ -1093,3 +1094,5 @@ config PPC_LIB_RHEAP
bool
source "arch/powerpc/kvm/Kconfig"
+
+source "kernel/livepatch/Kconfig"
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 44667fd..405efce 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
obj-$(CONFIG_TRACING) += trace_clock.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
ifneq ($(CONFIG_PPC_INDIRECT_PIO),y)
obj-y += iomap.o
--
1.8.5.6
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2)
2016-02-10 17:29 [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe
` (7 preceding siblings ...)
2016-02-10 16:36 ` [PATCH v8 7/8] Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it is selected Torsten Duwe
@ 2016-02-11 6:18 ` Balbir Singh
2016-02-11 8:38 ` Torsten Duwe
8 siblings, 1 reply; 44+ messages in thread
From: Balbir Singh @ 2016-02-11 6:18 UTC (permalink / raw)
To: Torsten Duwe, Michael Ellerman
Cc: Petr Mladek, Jessica Yu, Jiri Kosina, linux-kernel,
Steven Rostedt, live-patching, Miroslav Benes, linuxppc-dev
On Wed, 2016-02-10 at 18:29 +0100, Torsten Duwe wrote:
> Changes since V7:
> * drop "notrace" attribute for MMU-aiding functions
> and their callees.
> * merge "-mprofile-kernel"-stripping patches into one.
>
> Changes since v6:
> * include Petr's patch, on popular demand ;)
> * move #ifdefs out of functions for readability;
> introduce static helper functions instead.
> * No more literal binary instructions in hex,
> at least not added by this patch set.
> * add compile time checker to detect the presence
> of known-good -mprofile-kernel support.
> * limit Kconfig / compile to the configurations really supported:
> + (static) FTRACE with -pg
> + DYNAMIC_FTRACE with -pg with or without -mprofile-kernel
> (depending on the compiler)
> + DYNAMIC_FTRACE_WITH_REGS only with -mprofile-kernel
> (will error out if the compiler is broken)
>
> Changes since v5:
> * extra "std r0,LRSAVE(r1)" for gcc-6
> This makes the code compiler-agnostic.
> * Follow Petr Mladek's suggestion to avoid
> redefinition of HAVE_LIVEPATCH
>
> Changes since v4:
> * change comment style in entry_64.S to C89
> (nobody is using assembler syntax comments there).
> * the bool function restore_r2 shouldn't return 2,
> that's a little confusing.
> * Test whether the compiler supports -mprofile-kernel
> and only then define CC_USING_MPROFILE_KERNEL
> * also make the return value of klp_check_compiler_support
> depend on that.
>
> Major changes since v3:
> * the graph tracer works now.
> It turned out the stack frame it tried to manipulate does not
> exist at that point.
> * changes only needed in order to support -mprofile-kernel are now
> in a separate patch, prepended.
> * Kconfig cleanup so this is only selectable on ppc64le.
>
> Petr Mladek (1):
> livepatch: Detect offset for the ftrace location during build
>
> Torsten Duwe (7):
> ppc64 (le): prepare for -mprofile-kernel
> ppc64le FTRACE_WITH_REGS implementation
> ppc use ftrace_modify_all_code default
> ppc64 ftrace_with_regs configuration variables
> ppc64 ftrace_with_regs: disable profiling for some files
> Implement kernel live patching for ppc64le (ABIv2)
> Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it
> is selected
>
> arch/powerpc/Kconfig | 6 +
> arch/powerpc/Makefile | 17 +++
> arch/powerpc/gcc-mprofile-kernel-notrace.sh | 33 ++++++
> arch/powerpc/include/asm/code-patching.h | 24 ++++
> arch/powerpc/include/asm/ftrace.h | 5 +
> arch/powerpc/include/asm/livepatch.h | 45 ++++++++
> arch/powerpc/kernel/Makefile | 13 ++-
> arch/powerpc/kernel/entry_64.S | 169
> +++++++++++++++++++++++++++-
> arch/powerpc/kernel/ftrace.c | 129 ++++++++++++++++-----
> arch/powerpc/kernel/livepatch.c | 38 +++++++
> arch/powerpc/kernel/module_64.c | 56 ++++++++-
> arch/powerpc/lib/Makefile | 4 +-
> arch/s390/Kconfig | 1 +
> kernel/livepatch/Makefile | 13 +++
> kernel/livepatch/core.c | 12 +-
> kernel/livepatch/ftrace-test.c | 6 +
> kernel/trace/Kconfig | 5 +
> scripts/recordmcount.c | 6 +-
> scripts/recordmcount.h | 17 ++-
> 19 files changed, 552 insertions(+), 47 deletions(-)
> create mode 100755 arch/powerpc/gcc-mprofile-kernel-notrace.sh
> create mode 100644 arch/powerpc/include/asm/livepatch.h
> create mode 100644 arch/powerpc/kernel/livepatch.c
> create mode 100644 kernel/livepatch/ftrace-test.c
>
Quick question - I presume these apply on top of 4.5.0-rc2?
Balbir Singh.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2)
2016-02-11 6:18 ` [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Balbir Singh
@ 2016-02-11 8:38 ` Torsten Duwe
0 siblings, 0 replies; 44+ messages in thread
From: Torsten Duwe @ 2016-02-11 8:38 UTC (permalink / raw)
To: Balbir Singh
Cc: Michael Ellerman, Petr Mladek, Jessica Yu, Jiri Kosina,
linux-kernel, Steven Rostedt, live-patching, Miroslav Benes,
linuxppc-dev
On Thu, Feb 11, 2016 at 05:18:23PM +1100, Balbir Singh wrote:
>
> Quick question - I presume these apply on top of 4.5.0-rc2?
Yes.
Torsten
^ permalink raw reply [flat|nested] 44+ messages in thread