The map_groups__fixup_end() was called to set end addresses of kernel
map and module maps. But now machine__create_modules() is set the end
address of modules properly so the only remaining piece is the kernel
map. We can set it with adjacent module's address directly instead of
calling the map_groups__fixup_end(). If there's no module after the
kernel map, the end address will be ~0ULL.
Reported-by: Kim Phillips <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/machine.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 2eca8478e24f..be328416de61 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1019,13 +1019,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
return ret;
}
-static void map_groups__fixup_end(struct map_groups *mg)
-{
- int i;
- for (i = 0; i < MAP__NR_TYPES; ++i)
- __map_groups__fixup_end(mg, i);
-}
-
static char *get_kernel_version(const char *root_dir)
{
char version[PATH_MAX];
@@ -1233,7 +1226,9 @@ int machine__create_kernel_maps(struct machine *machine)
{
struct dso *kernel = machine__get_kernel(machine);
const char *name = NULL;
+ struct map *map;
u64 addr = 0;
+ u64 end = ~0ULL;
int ret;
if (kernel == NULL)
@@ -1259,13 +1254,14 @@ int machine__create_kernel_maps(struct machine *machine)
machine__destroy_kernel_maps(machine);
return -1;
}
- machine__set_kernel_mmap(machine, addr, 0);
}
- /*
- * Now that we have all the maps created, just set the ->end of them:
- */
- map_groups__fixup_end(&machine->kmaps);
+ /* update end address of the kernel map using adjacent module address */
+ map = map__next(machine__kernel_map(machine));
+ if (map)
+ end = map->start;
+
+ machine__set_kernel_mmap(machine, addr, end);
return 0;
}
--
2.16.2
On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> The map_groups__fixup_end() was called to set end addresses of kernel
> map and module maps. But now machine__create_modules() is set the end
> address of modules properly so the only remaining piece is the kernel
> map. We can set it with adjacent module's address directly instead of
> calling the map_groups__fixup_end(). If there's no module after the
> kernel map, the end address will be ~0ULL.
>
> Reported-by: Kim Phillips <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
looks good
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
> ---
> tools/perf/util/machine.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 2eca8478e24f..be328416de61 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1019,13 +1019,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
> return ret;
> }
>
> -static void map_groups__fixup_end(struct map_groups *mg)
> -{
> - int i;
> - for (i = 0; i < MAP__NR_TYPES; ++i)
> - __map_groups__fixup_end(mg, i);
> -}
> -
> static char *get_kernel_version(const char *root_dir)
> {
> char version[PATH_MAX];
> @@ -1233,7 +1226,9 @@ int machine__create_kernel_maps(struct machine *machine)
> {
> struct dso *kernel = machine__get_kernel(machine);
> const char *name = NULL;
> + struct map *map;
> u64 addr = 0;
> + u64 end = ~0ULL;
> int ret;
>
> if (kernel == NULL)
> @@ -1259,13 +1254,14 @@ int machine__create_kernel_maps(struct machine *machine)
> machine__destroy_kernel_maps(machine);
> return -1;
> }
> - machine__set_kernel_mmap(machine, addr, 0);
> }
>
> - /*
> - * Now that we have all the maps created, just set the ->end of them:
> - */
> - map_groups__fixup_end(&machine->kmaps);
> + /* update end address of the kernel map using adjacent module address */
> + map = map__next(machine__kernel_map(machine));
> + if (map)
> + end = map->start;
> +
> + machine__set_kernel_mmap(machine, addr, end);
> return 0;
> }
>
> --
> 2.16.2
>
Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > The map_groups__fixup_end() was called to set end addresses of kernel
> > map and module maps. But now machine__create_modules() is set the end
> > address of modules properly so the only remaining piece is the kernel
> > map. We can set it with adjacent module's address directly instead of
> > calling the map_groups__fixup_end(). If there's no module after the
> > kernel map, the end address will be ~0ULL.
I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
or something like that...
But, anyway, Kim, does this fix things for you? Can I have your
Tested-by?
> > Reported-by: Kim Phillips <[email protected]>
> > Signed-off-by: Namhyung Kim <[email protected]>
>
> looks good
>
> Acked-by: Jiri Olsa <[email protected]>
>
> thanks,
> jirka
>
> > ---
> > tools/perf/util/machine.c | 20 ++++++++------------
> > 1 file changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 2eca8478e24f..be328416de61 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1019,13 +1019,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
> > return ret;
> > }
> >
> > -static void map_groups__fixup_end(struct map_groups *mg)
> > -{
> > - int i;
> > - for (i = 0; i < MAP__NR_TYPES; ++i)
> > - __map_groups__fixup_end(mg, i);
> > -}
> > -
> > static char *get_kernel_version(const char *root_dir)
> > {
> > char version[PATH_MAX];
> > @@ -1233,7 +1226,9 @@ int machine__create_kernel_maps(struct machine *machine)
> > {
> > struct dso *kernel = machine__get_kernel(machine);
> > const char *name = NULL;
> > + struct map *map;
> > u64 addr = 0;
> > + u64 end = ~0ULL;
> > int ret;
> >
> > if (kernel == NULL)
> > @@ -1259,13 +1254,14 @@ int machine__create_kernel_maps(struct machine *machine)
> > machine__destroy_kernel_maps(machine);
> > return -1;
> > }
> > - machine__set_kernel_mmap(machine, addr, 0);
> > }
> >
> > - /*
> > - * Now that we have all the maps created, just set the ->end of them:
> > - */
> > - map_groups__fixup_end(&machine->kmaps);
> > + /* update end address of the kernel map using adjacent module address */
> > + map = map__next(machine__kernel_map(machine));
> > + if (map)
> > + end = map->start;
> > +
> > + machine__set_kernel_mmap(machine, addr, end);
> > return 0;
> > }
> >
> > --
> > 2.16.2
> >
On Mon, 16 Apr 2018 10:51:25 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> > On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > map and module maps. But now machine__create_modules() is set the end
> > > address of modules properly so the only remaining piece is the kernel
> > > map. We can set it with adjacent module's address directly instead of
> > > calling the map_groups__fixup_end(). If there's no module after the
> > > kernel map, the end address will be ~0ULL.
>
> I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
> or something like that...
>
> But, anyway, Kim, does this fix things for you? Can I have your
> Tested-by?
No, perf test 1 still fails:
vmlinux symtab matches kallsyms: FAILED!
Kim
Em Mon, Apr 16, 2018 at 11:07:30AM -0500, Kim Phillips escreveu:
> On Mon, 16 Apr 2018 10:51:25 -0300
> Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> > Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> > > On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > > map and module maps. But now machine__create_modules() is set the end
> > > > address of modules properly so the only remaining piece is the kernel
> > > > map. We can set it with adjacent module's address directly instead of
> > > > calling the map_groups__fixup_end(). If there's no module after the
> > > > kernel map, the end address will be ~0ULL.
> >
> > I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
> > or something like that...
> >
> > But, anyway, Kim, does this fix things for you? Can I have your
> > Tested-by?
>
> No, perf test 1 still fails:
>
> vmlinux symtab matches kallsyms: FAILED!
Ok, -vv please
- Arnaldo
On Mon, 16 Apr 2018 13:58:00 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Mon, Apr 16, 2018 at 11:07:30AM -0500, Kim Phillips escreveu:
> > On Mon, 16 Apr 2018 10:51:25 -0300
> > Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> > > Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> > > > On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > > > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > > > map and module maps. But now machine__create_modules() is set the end
> > > > > address of modules properly so the only remaining piece is the kernel
> > > > > map. We can set it with adjacent module's address directly instead of
> > > > > calling the map_groups__fixup_end(). If there's no module after the
> > > > > kernel map, the end address will be ~0ULL.
> > >
> > > I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
> > > or something like that...
> > >
> > > But, anyway, Kim, does this fix things for you? Can I have your
> > > Tested-by?
> >
> > No, perf test 1 still fails:
> >
> > vmlinux symtab matches kallsyms: FAILED!
>
> Ok, -vv please
a perf/urgent from last week (commit 918965d4897) + this patch:
$ sudo ./perf test -vv 1 |& head
1: vmlinux symtab matches kallsyms :
--- start ---
test child forked, pid 6194
Looking at the vmlinux_path (8 entries long)
Using /lib/modules/4.16.0+/build/vmlinux for symbols
ERR : 0xffff200008081000: do_undefinstr not on kallsyms
ERR : 0xffff2000080810b8: do_sysinstr not on kallsyms
ERR : 0xffff200008081258: do_debug_exception not on kallsyms
ERR : 0xffff200008081648: do_mem_abort not on kallsyms
ERR : 0xffff2000080818b8: do_el0_irq_bp_hardening not on kallsyms
$ sudo ./perf test -vv 1 |& tail
ERR : 0xffff20000a1d37c8: tramp_exit_native not on kallsyms
ERR : 0xffff20000a1d37e8: tramp_exit_compat not on kallsyms
ERR : 0xffff20000a1d4000: __entry_tramp_text_end not on kallsyms
WARN: Maps only in vmlinux:
ffff200008080000-ffff200008081000 10000 [kernel].head.text
ffff20000aec0000-ffff20000aff7548 2e50000 [kernel].init.text
ffff20000aff7548-ffff20000b0126d4 2f87548 [kernel].exit.text
test child finished with -1
---- end ----
vmlinux symtab matches kallsyms: FAILED!
$
Kim
On Mon, 16 Apr 2018 12:24:07 -0500
Kim Phillips <[email protected]> wrote:
> On Mon, 16 Apr 2018 13:58:00 -0300
> Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> > Em Mon, Apr 16, 2018 at 11:07:30AM -0500, Kim Phillips escreveu:
> > > On Mon, 16 Apr 2018 10:51:25 -0300
> > > Arnaldo Carvalho de Melo <[email protected]> wrote:
> > >
> > > > Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> > > > > On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > > > > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > > > > map and module maps. But now machine__create_modules() is set the end
> > > > > > address of modules properly so the only remaining piece is the kernel
> > > > > > map. We can set it with adjacent module's address directly instead of
> > > > > > calling the map_groups__fixup_end(). If there's no module after the
> > > > > > kernel map, the end address will be ~0ULL.
> > > >
> > > > I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
> > > > or something like that...
> > > >
> > > > But, anyway, Kim, does this fix things for you? Can I have your
> > > > Tested-by?
> > >
> > > No, perf test 1 still fails:
> > >
> > > vmlinux symtab matches kallsyms: FAILED!
> >
> > Ok, -vv please
>
> a perf/urgent from last week (commit 918965d4897) + this patch:
>
> $ sudo ./perf test -vv 1 |& head
> 1: vmlinux symtab matches kallsyms :
> --- start ---
> test child forked, pid 6194
> Looking at the vmlinux_path (8 entries long)
> Using /lib/modules/4.16.0+/build/vmlinux for symbols
> ERR : 0xffff200008081000: do_undefinstr not on kallsyms
> ERR : 0xffff2000080810b8: do_sysinstr not on kallsyms
> ERR : 0xffff200008081258: do_debug_exception not on kallsyms
> ERR : 0xffff200008081648: do_mem_abort not on kallsyms
> ERR : 0xffff2000080818b8: do_el0_irq_bp_hardening not on kallsyms
> $ sudo ./perf test -vv 1 |& tail
> ERR : 0xffff20000a1d37c8: tramp_exit_native not on kallsyms
> ERR : 0xffff20000a1d37e8: tramp_exit_compat not on kallsyms
> ERR : 0xffff20000a1d4000: __entry_tramp_text_end not on kallsyms
> WARN: Maps only in vmlinux:
> ffff200008080000-ffff200008081000 10000 [kernel].head.text
> ffff20000aec0000-ffff20000aff7548 2e50000 [kernel].init.text
> ffff20000aff7548-ffff20000b0126d4 2f87548 [kernel].exit.text
> test child finished with -1
> ---- end ----
> vmlinux symtab matches kallsyms: FAILED!
this patch's advertised "If there's no module after the kernel map, the
end address will be ~0ULL." doesn't seem to be working: the value it
gets for 'end' is 0xffff200008080000.
I even hardcoded 'end' variable to the value for '_end' in kallsyms
prior to the machine__set_kernel_mmap() call, and test 1 still fails.
The problem is earlier on in the code, somewhere.
I've moved to working on the updated perf/urgent. Reverting the
original patch doesn't make test 1 resume succeeding anymore.
Pointers appreciated.
Kim
On Mon, Apr 16, 2018 at 05:48:11PM -0500, Kim Phillips wrote:
> On Mon, 16 Apr 2018 12:24:07 -0500
> Kim Phillips <[email protected]> wrote:
>
> > On Mon, 16 Apr 2018 13:58:00 -0300
> > Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> > > Em Mon, Apr 16, 2018 at 11:07:30AM -0500, Kim Phillips escreveu:
> > > > On Mon, 16 Apr 2018 10:51:25 -0300
> > > > Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > >
> > > > > Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> > > > > > On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > > > > > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > > > > > map and module maps. But now machine__create_modules() is set the end
> > > > > > > address of modules properly so the only remaining piece is the kernel
> > > > > > > map. We can set it with adjacent module's address directly instead of
> > > > > > > calling the map_groups__fixup_end(). If there's no module after the
> > > > > > > kernel map, the end address will be ~0ULL.
> > > > >
> > > > > I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
> > > > > or something like that...
> > > > >
> > > > > But, anyway, Kim, does this fix things for you? Can I have your
> > > > > Tested-by?
> > > >
> > > > No, perf test 1 still fails:
> > > >
> > > > vmlinux symtab matches kallsyms: FAILED!
> > >
> > > Ok, -vv please
> >
> > a perf/urgent from last week (commit 918965d4897) + this patch:
> >
> > $ sudo ./perf test -vv 1 |& head
> > 1: vmlinux symtab matches kallsyms :
> > --- start ---
> > test child forked, pid 6194
> > Looking at the vmlinux_path (8 entries long)
> > Using /lib/modules/4.16.0+/build/vmlinux for symbols
> > ERR : 0xffff200008081000: do_undefinstr not on kallsyms
> > ERR : 0xffff2000080810b8: do_sysinstr not on kallsyms
> > ERR : 0xffff200008081258: do_debug_exception not on kallsyms
> > ERR : 0xffff200008081648: do_mem_abort not on kallsyms
> > ERR : 0xffff2000080818b8: do_el0_irq_bp_hardening not on kallsyms
> > $ sudo ./perf test -vv 1 |& tail
> > ERR : 0xffff20000a1d37c8: tramp_exit_native not on kallsyms
> > ERR : 0xffff20000a1d37e8: tramp_exit_compat not on kallsyms
> > ERR : 0xffff20000a1d4000: __entry_tramp_text_end not on kallsyms
> > WARN: Maps only in vmlinux:
> > ffff200008080000-ffff200008081000 10000 [kernel].head.text
> > ffff20000aec0000-ffff20000aff7548 2e50000 [kernel].init.text
> > ffff20000aff7548-ffff20000b0126d4 2f87548 [kernel].exit.text
> > test child finished with -1
> > ---- end ----
> > vmlinux symtab matches kallsyms: FAILED!
>
> this patch's advertised "If there's no module after the kernel map, the
> end address will be ~0ULL." doesn't seem to be working: the value it
> gets for 'end' is 0xffff200008080000.
For the vmlinux, right?
To be precise, it should be "If there's no map after the kernel map".
It seems vmlinux adds more maps after the kernel map for those .head,
.init and .exit text sections.
>
> I even hardcoded 'end' variable to the value for '_end' in kallsyms
> prior to the machine__set_kernel_mmap() call, and test 1 still fails.
I guess it was overwritten by the machine__set_kernel_mmap() then.
>
> The problem is earlier on in the code, somewhere.
To be sure, could you please show me the /proc/kallsymb output around
the missing symbols (do_undefinstr, ...)?
>
> I've moved to working on the updated perf/urgent. Reverting the
> original patch doesn't make test 1 resume succeeding anymore.
>
> Pointers appreciated.
The missing symbols are in the missing maps. They should be in the
kernel maps for the kallsyms case IMHO. Could you please run the test
with below patch?
Thanks,
Namhyung
---------------8<----------------
diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
index 1e5adb65632a..b14ed70fa440 100644
--- a/tools/perf/tests/vmlinux-kallsyms.c
+++ b/tools/perf/tests/vmlinux-kallsyms.c
@@ -163,6 +163,17 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
continue;
}
+ } else if (pair) {
+ struct map *kmap = map_groups__find(&kallsyms.kmaps, type, mem_start);
+ struct map *vmap = map_groups__find(&vmlinux.kmaps, type, mem_start);
+
+ pr_debug("ERR : %#" PRIx64 ": diff addr v: %s k: %#" PRIx64 " %s\n",
+ mem_start, sym->name, UM(pair->start), pair->name);
+
+ if (kmap && vmap) {
+ pr_debug(" : map v: %s k: %s\n",
+ vmap->dso->short_name, kmap->dso->short_name);
+ }
} else
pr_debug("ERR : %#" PRIx64 ": %s not on kallsyms\n",
mem_start, sym->name);
On Tue, 17 Apr 2018 11:27:26 +0900
Namhyung Kim <[email protected]> wrote:
> On Mon, Apr 16, 2018 at 05:48:11PM -0500, Kim Phillips wrote:
> > On Mon, 16 Apr 2018 12:24:07 -0500
> > Kim Phillips <[email protected]> wrote:
> >
> > > On Mon, 16 Apr 2018 13:58:00 -0300
> > > Arnaldo Carvalho de Melo <[email protected]> wrote:
> > >
> > > > Em Mon, Apr 16, 2018 at 11:07:30AM -0500, Kim Phillips escreveu:
> > > > > On Mon, 16 Apr 2018 10:51:25 -0300
> > > > > Arnaldo Carvalho de Melo <[email protected]> wrote:
> > > > >
> > > > > > Em Mon, Apr 16, 2018 at 11:23:45AM +0200, Jiri Olsa escreveu:
> > > > > > > On Mon, Apr 16, 2018 at 01:22:40PM +0900, Namhyung Kim wrote:
> > > > > > > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > > > > > > map and module maps. But now machine__create_modules() is set the end
> > > > > > > > address of modules properly so the only remaining piece is the kernel
> > > > > > > > map. We can set it with adjacent module's address directly instead of
> > > > > > > > calling the map_groups__fixup_end(). If there's no module after the
> > > > > > > > kernel map, the end address will be ~0ULL.
> > > > > >
> > > > > > I wonder if it wouldn't be better to have it as last symbol + PAGE_SIZE
> > > > > > or something like that...
> > > > > >
> > > > > > But, anyway, Kim, does this fix things for you? Can I have your
> > > > > > Tested-by?
> > > > >
> > > > > No, perf test 1 still fails:
> > > > >
> > > > > vmlinux symtab matches kallsyms: FAILED!
> > > >
> > > > Ok, -vv please
> > >
> > > a perf/urgent from last week (commit 918965d4897) + this patch:
> > >
> > > $ sudo ./perf test -vv 1 |& head
> > > 1: vmlinux symtab matches kallsyms :
> > > --- start ---
> > > test child forked, pid 6194
> > > Looking at the vmlinux_path (8 entries long)
> > > Using /lib/modules/4.16.0+/build/vmlinux for symbols
> > > ERR : 0xffff200008081000: do_undefinstr not on kallsyms
> > > ERR : 0xffff2000080810b8: do_sysinstr not on kallsyms
> > > ERR : 0xffff200008081258: do_debug_exception not on kallsyms
> > > ERR : 0xffff200008081648: do_mem_abort not on kallsyms
> > > ERR : 0xffff2000080818b8: do_el0_irq_bp_hardening not on kallsyms
> > > $ sudo ./perf test -vv 1 |& tail
> > > ERR : 0xffff20000a1d37c8: tramp_exit_native not on kallsyms
> > > ERR : 0xffff20000a1d37e8: tramp_exit_compat not on kallsyms
> > > ERR : 0xffff20000a1d4000: __entry_tramp_text_end not on kallsyms
> > > WARN: Maps only in vmlinux:
> > > ffff200008080000-ffff200008081000 10000 [kernel].head.text
> > > ffff20000aec0000-ffff20000aff7548 2e50000 [kernel].init.text
> > > ffff20000aff7548-ffff20000b0126d4 2f87548 [kernel].exit.text
> > > test child finished with -1
> > > ---- end ----
> > > vmlinux symtab matches kallsyms: FAILED!
> >
> > this patch's advertised "If there's no module after the kernel map, the
> > end address will be ~0ULL." doesn't seem to be working: the value it
> > gets for 'end' is 0xffff200008080000.
>
> For the vmlinux, right?
yes, map__next(machine__kernel_map(machine)) has the start address
of the single module currently loaded:
ffff200002290000 t $x [arm_ccn]
ffff200002290000 t arm_ccn_pmu_events_is_visible [arm_ccn]
The beginning of the kernel is..later:
ffff200008080000 t _head
ffff200008080000 T _text
and its end according to grep -w _end /proc/kallsyms is:
ffff20000d5f9000 B _end
but end was assigned to the beginning of arm_ccn (0xffff200002290000),
which is upside-down.
> To be precise, it should be "If there's no map after the kernel map".
In numerical address order, in maps in map_groups__insert order, or
some other order?
> It seems vmlinux adds more maps after the kernel map for those .head,
> .init and .exit text sections.
those shouldn't matter, I don't think - x86 has .init and .exit also,
and I don't necessarily see the .head addresses being involved. Let me
know if this is indeed going to be a problem.
> > I even hardcoded 'end' variable to the value for '_end' in kallsyms
> > prior to the machine__set_kernel_mmap() call, and test 1 still fails.
>
> I guess it was overwritten by the machine__set_kernel_mmap() then.
Yes.
> > The problem is earlier on in the code, somewhere.
>
> To be sure, could you please show me the /proc/kallsymb output around
> the missing symbols (do_undefinstr, ...)?
see below
> > I've moved to working on the updated perf/urgent. Reverting the
> > original patch doesn't make test 1 resume succeeding anymore.
> >
> > Pointers appreciated.
>
> The missing symbols are in the missing maps. They should be in the
> kernel maps for the kallsyms case IMHO. Could you please run the test
> with below patch?
Thanks! Turns out that in the case of no modules loaded, commit
a257e02579e4270 "arm64/kernel: don't ban ADRP to work around Cortex-A53
erratum #843419" added a 'veneer' symbol with a different start
address, so instead of:
ERR : 0xffff44eb0a6c9620: module_emit_adrp_veneer not on kallsyms
I got:
ERR : 0xffff44eb0a6c9620: diff addr v: module_emit_adrp_veneer k: 0xffff44eb0a6c9500 module_emit_plt_entry
: map v: [kernel] k: [kernel]
So it helped solve the no-modules-loaded case. I've updated my perf
tool to the latest acme's perf/urgent (commit e6a0a610a60), added this
patch "perf tools: set kernel end address properly", and this diff,
enhanced/re-purposed it a bit, and included a fix for succeeding the
test 1 when there are no modules loaded - see the end of this email. I
doubt it's acceptable as-is, so please take a look.
Meanwhile, in order to help debug the module(s) loaded case, I'm
providing:
- output of the above perf, called with 'test -vvvvv 1' with
no modules loaded (now passes test):
- http://paste.ubuntu.com/p/xBgpjTyjFp/
- output of the above perf, called with 'test -vvvvv 1' with
the arm-ccn module loaded (does not pass test):
- http://paste.ubuntu.com/p/nWYxC9c5y2/
- /proc/kallsyms with the module loaded:
- http://paste.ubuntu.com/p/pKskqyqdsK/
Thanks for your help,
Kim
diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
index 0051b1ee8450..5c4a2e208bbc 100644
--- a/tools/perf/arch/arm64/util/sym-handling.c
+++ b/tools/perf/arch/arm64/util/sym-handling.c
@@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
ehdr.e_type == ET_DYN;
}
#endif
+
+const char *arch__normalize_symbol_name(const char *name)
+{
+ /*
+ * arm64 kernels compensating for a CPU erratum can put up a
+ * module_emit_adrp_veneer in place of a module_emit_plt_entry
+ */
+ if (name && strlen(name) >= 23 &&
+ !strncmp(name, "module_emit_adrp_veneer", 23))
+ return "module_emit_plt_entry";
+
+ return name;
+}
diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
index 1e5adb65632a..07064e76947d 100644
--- a/tools/perf/tests/vmlinux-kallsyms.c
+++ b/tools/perf/tests/vmlinux-kallsyms.c
@@ -163,6 +163,29 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
continue;
}
+ } else if (pair) {
+ s64 skew = mem_start - UM(pair->start);
+ struct map *kmap = map_groups__find(&kallsyms.kmaps, type, mem_start);
+ struct map *vmap = map_groups__find(&vmlinux.kmaps, type, mem_start);
+
+ /*
+ * arm64 kernels compensating for a CPU erratum can put up a
+ * module_emit_adrp_veneer in place of a module_emit_plt_entry
+ */
+ if (llabs(skew) < page_size)
+ {
+ pr_debug("NO ERR FOR SKEW %ld: %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
+ skew, mem_start, sym->name, UM(pair->start), pair->name);
+ continue;
+ }
+
+ pr_debug("ERR : %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
+ mem_start, sym->name, UM(pair->start), pair->name);
+
+ if (kmap && vmap) {
+ pr_debug(" : map v: %s k: %s\n",
+ vmap->dso->short_name, kmap->dso->short_name);
+ }
} else
pr_debug("ERR : %#" PRIx64 ": %s not on kallsyms\n",
mem_start, sym->name);
On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
> On Tue, 17 Apr 2018 11:27:26 +0900
> Namhyung Kim <[email protected]> wrote:
> > On Mon, Apr 16, 2018 at 05:48:11PM -0500, Kim Phillips wrote:
> > > > a perf/urgent from last week (commit 918965d4897) + this patch:
> > > >
> > > > $ sudo ./perf test -vv 1 |& head
> > > > 1: vmlinux symtab matches kallsyms :
> > > > --- start ---
> > > > test child forked, pid 6194
> > > > Looking at the vmlinux_path (8 entries long)
> > > > Using /lib/modules/4.16.0+/build/vmlinux for symbols
> > > > ERR : 0xffff200008081000: do_undefinstr not on kallsyms
> > > > ERR : 0xffff2000080810b8: do_sysinstr not on kallsyms
> > > > ERR : 0xffff200008081258: do_debug_exception not on kallsyms
> > > > ERR : 0xffff200008081648: do_mem_abort not on kallsyms
> > > > ERR : 0xffff2000080818b8: do_el0_irq_bp_hardening not on kallsyms
> > > > $ sudo ./perf test -vv 1 |& tail
> > > > ERR : 0xffff20000a1d37c8: tramp_exit_native not on kallsyms
> > > > ERR : 0xffff20000a1d37e8: tramp_exit_compat not on kallsyms
> > > > ERR : 0xffff20000a1d4000: __entry_tramp_text_end not on kallsyms
> > > > WARN: Maps only in vmlinux:
> > > > ffff200008080000-ffff200008081000 10000 [kernel].head.text
> > > > ffff20000aec0000-ffff20000aff7548 2e50000 [kernel].init.text
> > > > ffff20000aff7548-ffff20000b0126d4 2f87548 [kernel].exit.text
> > > > test child finished with -1
> > > > ---- end ----
> > > > vmlinux symtab matches kallsyms: FAILED!
> > >
> > > this patch's advertised "If there's no module after the kernel map, the
> > > end address will be ~0ULL." doesn't seem to be working: the value it
> > > gets for 'end' is 0xffff200008080000.
> >
> > For the vmlinux, right?
>
> yes, map__next(machine__kernel_map(machine)) has the start address
> of the single module currently loaded:
>
> ffff200002290000 t $x [arm_ccn]
> ffff200002290000 t arm_ccn_pmu_events_is_visible [arm_ccn]
>
> The beginning of the kernel is..later:
>
> ffff200008080000 t _head
> ffff200008080000 T _text
>
> and its end according to grep -w _end /proc/kallsyms is:
>
> ffff20000d5f9000 B _end
>
> but end was assigned to the beginning of arm_ccn (0xffff200002290000),
> which is upside-down.
So ARM64 has modules below the kernel.
>
> > To be precise, it should be "If there's no map after the kernel map".
>
> In numerical address order, in maps in map_groups__insert order, or
> some other order?
The map_groups__insert() also sorts the map by address, so they should
be identical. I think the problem is perf assumes the kernel is the
first map in the kmaps. When it calls maps_groups__insert() it uses
start address of 0 for the kernel map. It seems always true for x86
but not for ARM64.
While it changes the start address in machine__set_kernel_mmap() it
doesn't change the order in the kmaps.
Could you please test below patch (on top) then?
Thanks,
Namhyung
---8<---
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index be328416de61..0f3c4bc7b90f 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1228,7 +1228,6 @@ int machine__create_kernel_maps(struct machine *machine)
const char *name = NULL;
struct map *map;
u64 addr = 0;
- u64 end = ~0ULL;
int ret;
if (kernel == NULL)
@@ -1254,14 +1253,25 @@ int machine__create_kernel_maps(struct machine *machine)
machine__destroy_kernel_maps(machine);
return -1;
}
+
+ /* we have a real start address now, so re-order the kmaps */
+ map = machine__kernel_map(machine);
+
+ map__get(map);
+ map_groups__remove(&machine->kmaps, map);
+
+ /* assume it's the last in the kmaps */
+ machine__set_kernel_mmap(machine, addr, ~0ULL);
+
+ map_groups__insert(&machine->kmaps, map);
+ map__put(map);
}
/* update end address of the kernel map using adjacent module address */
map = map__next(machine__kernel_map(machine));
if (map)
- end = map->start;
+ machine__set_kernel_mmap(machine, addr, map->start);
- machine__set_kernel_mmap(machine, addr, end);
return 0;
}
On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
> diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
> index 0051b1ee8450..5c4a2e208bbc 100644
> --- a/tools/perf/arch/arm64/util/sym-handling.c
> +++ b/tools/perf/arch/arm64/util/sym-handling.c
> @@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
> ehdr.e_type == ET_DYN;
> }
> #endif
> +
> +const char *arch__normalize_symbol_name(const char *name)
> +{
> + /*
> + * arm64 kernels compensating for a CPU erratum can put up a
> + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> + */
> + if (name && strlen(name) >= 23 &&
> + !strncmp(name, "module_emit_adrp_veneer", 23))
> + return "module_emit_plt_entry";
> +
> + return name;
> +}
I don't know it's always preferable or just for the test. It it's the
latter it may be better to move it to the test code.
> diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> index 1e5adb65632a..07064e76947d 100644
> --- a/tools/perf/tests/vmlinux-kallsyms.c
> +++ b/tools/perf/tests/vmlinux-kallsyms.c
> @@ -163,6 +163,29 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
>
> continue;
> }
> + } else if (pair) {
> + s64 skew = mem_start - UM(pair->start);
> + struct map *kmap = map_groups__find(&kallsyms.kmaps, type, mem_start);
> + struct map *vmap = map_groups__find(&vmlinux.kmaps, type, mem_start);
> +
> + /*
> + * arm64 kernels compensating for a CPU erratum can put up a
> + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> + */
> + if (llabs(skew) < page_size)
It seems that we needs to check it's the ARM64 at least. If it's a
rare case we might need to add more paranoid checks.
Thanks,
Namhyung
> + {
> + pr_debug("NO ERR FOR SKEW %ld: %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
> + skew, mem_start, sym->name, UM(pair->start), pair->name);
> + continue;
> + }
> +
> + pr_debug("ERR : %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
> + mem_start, sym->name, UM(pair->start), pair->name);
> +
> + if (kmap && vmap) {
> + pr_debug(" : map v: %s k: %s\n",
> + vmap->dso->short_name, kmap->dso->short_name);
> + }
> } else
> pr_debug("ERR : %#" PRIx64 ": %s not on kallsyms\n",
> mem_start, sym->name);
On Thu, 19 Apr 2018 11:37:12 +0900
Namhyung Kim <[email protected]> wrote:
> On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
> > On Tue, 17 Apr 2018 11:27:26 +0900
> > Namhyung Kim <[email protected]> wrote:
> > > On Mon, Apr 16, 2018 at 05:48:11PM -0500, Kim Phillips wrote:
> > > > > a perf/urgent from last week (commit 918965d4897) + this patch:
> > > > >
> > > > > $ sudo ./perf test -vv 1 |& head
> > > > > 1: vmlinux symtab matches kallsyms :
> > > > > --- start ---
> > > > > test child forked, pid 6194
> > > > > Looking at the vmlinux_path (8 entries long)
> > > > > Using /lib/modules/4.16.0+/build/vmlinux for symbols
> > > > > ERR : 0xffff200008081000: do_undefinstr not on kallsyms
> > > > > ERR : 0xffff2000080810b8: do_sysinstr not on kallsyms
> > > > > ERR : 0xffff200008081258: do_debug_exception not on kallsyms
> > > > > ERR : 0xffff200008081648: do_mem_abort not on kallsyms
> > > > > ERR : 0xffff2000080818b8: do_el0_irq_bp_hardening not on kallsyms
> > > > > $ sudo ./perf test -vv 1 |& tail
> > > > > ERR : 0xffff20000a1d37c8: tramp_exit_native not on kallsyms
> > > > > ERR : 0xffff20000a1d37e8: tramp_exit_compat not on kallsyms
> > > > > ERR : 0xffff20000a1d4000: __entry_tramp_text_end not on kallsyms
> > > > > WARN: Maps only in vmlinux:
> > > > > ffff200008080000-ffff200008081000 10000 [kernel].head.text
> > > > > ffff20000aec0000-ffff20000aff7548 2e50000 [kernel].init.text
> > > > > ffff20000aff7548-ffff20000b0126d4 2f87548 [kernel].exit.text
> > > > > test child finished with -1
> > > > > ---- end ----
> > > > > vmlinux symtab matches kallsyms: FAILED!
> > > >
> > > > this patch's advertised "If there's no module after the kernel map, the
> > > > end address will be ~0ULL." doesn't seem to be working: the value it
> > > > gets for 'end' is 0xffff200008080000.
> > >
> > > For the vmlinux, right?
> >
> > yes, map__next(machine__kernel_map(machine)) has the start address
> > of the single module currently loaded:
> >
> > ffff200002290000 t $x [arm_ccn]
> > ffff200002290000 t arm_ccn_pmu_events_is_visible [arm_ccn]
> >
> > The beginning of the kernel is..later:
> >
> > ffff200008080000 t _head
> > ffff200008080000 T _text
> >
> > and its end according to grep -w _end /proc/kallsyms is:
> >
> > ffff20000d5f9000 B _end
> >
> > but end was assigned to the beginning of arm_ccn (0xffff200002290000),
> > which is upside-down.
>
> So ARM64 has modules below the kernel.
right.
> > > To be precise, it should be "If there's no map after the kernel map".
> >
> > In numerical address order, in maps in map_groups__insert order, or
> > some other order?
>
> The map_groups__insert() also sorts the map by address, so they should
Had missed that, sorry.
> be identical. I think the problem is perf assumes the kernel is the
> first map in the kmaps. When it calls maps_groups__insert() it uses
> start address of 0 for the kernel map. It seems always true for x86
> but not for ARM64.
>
> While it changes the start address in machine__set_kernel_mmap() it
> doesn't change the order in the kmaps.
>
> Could you please test below patch (on top) then?
Much better, thank you! Together with the "module_emit_adrp_veneer not
on kallsyms" mitigation diff, test 1 now passes on arm64 systems both
with modules loaded, and without, and with and without RANDOMIZE_BASE.
I would send a patch (From: you), but I think you'd do a better job on
writing the commit text, so would you care to, please?
Thanks,
Kim
On Thu, 19 Apr 2018 11:54:24 +0900
Namhyung Kim <[email protected]> wrote:
> On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
> > diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
> > index 0051b1ee8450..5c4a2e208bbc 100644
> > --- a/tools/perf/arch/arm64/util/sym-handling.c
> > +++ b/tools/perf/arch/arm64/util/sym-handling.c
> > @@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
> > ehdr.e_type == ET_DYN;
> > }
> > #endif
> > +
> > +const char *arch__normalize_symbol_name(const char *name)
> > +{
> > + /*
> > + * arm64 kernels compensating for a CPU erratum can put up a
> > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> > + */
> > + if (name && strlen(name) >= 23 &&
> > + !strncmp(name, "module_emit_adrp_veneer", 23))
> > + return "module_emit_plt_entry";
> > +
> > + return name;
> > +}
>
> I don't know it's always preferable or just for the test. It it's the
> latter it may be better to move it to the test code.
AFACT, the veneer is a moniker and doesn't technically exist, and
shouldn't be being looked-up. Both chunks of this diff are needed to
pass perf test 1: this chunk above is because in
arch__normalize_symbol_name(), we squash the perf test 1's "<veneer>
not in *kallsyms*" problem, and in the below chunk, we prevent it
coming up when the test code iterates over the *vmlinux* symbols. I.e.
we need to prevent the veneer from coming up in both kallsyms *and*
vmlinux.
> > diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> > index 1e5adb65632a..07064e76947d 100644
> > --- a/tools/perf/tests/vmlinux-kallsyms.c
> > +++ b/tools/perf/tests/vmlinux-kallsyms.c
> > @@ -163,6 +163,29 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
> >
> > continue;
> > }
> > + } else if (pair) {
> > + s64 skew = mem_start - UM(pair->start);
> > + struct map *kmap = map_groups__find(&kallsyms.kmaps, type, mem_start);
> > + struct map *vmap = map_groups__find(&vmlinux.kmaps, type, mem_start);
> > +
> > + /*
> > + * arm64 kernels compensating for a CPU erratum can put up a
> > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> > + */
> > + if (llabs(skew) < page_size)
>
> It seems that we needs to check it's the ARM64 at least. If it's a
OK.
> rare case we might need to add more paranoid checks.
It's certainly rare: Adding the authors of the veneer to cc for
comments:
Will, Ard, how probable are veneer-style symbols such as the
one introduced in commit a257e0257 "arm64/kernel: don't ban ADRP to
work around Cortex-A53 erratum #843419" to happen again in the future?
I would have thought WARNing on within-a-pagesize would be OK,
Namhyung. Are you suggesting checking instead for a hardcoded veneer
symbol string?
Thanks,
Kim
> > + {
> > + pr_debug("NO ERR FOR SKEW %ld: %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
> > + skew, mem_start, sym->name, UM(pair->start), pair->name);
> > + continue;
> > + }
> > +
> > + pr_debug("ERR : %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
> > + mem_start, sym->name, UM(pair->start), pair->name);
> > +
> > + if (kmap && vmap) {
> > + pr_debug(" : map v: %s k: %s\n",
> > + vmap->dso->short_name, kmap->dso->short_name);
> > + }
> > } else
> > pr_debug("ERR : %#" PRIx64 ": %s not on kallsyms\n",
> > mem_start, sym->name);
On Thu, Apr 19, 2018 at 06:20:24PM -0500, Kim Phillips wrote:
> On Thu, 19 Apr 2018 11:37:12 +0900
> Namhyung Kim <[email protected]> wrote:
> > I think the problem is perf assumes the kernel is the
> > first map in the kmaps. When it calls maps_groups__insert() it uses
> > start address of 0 for the kernel map. It seems always true for x86
> > but not for ARM64.
> >
> > While it changes the start address in machine__set_kernel_mmap() it
> > doesn't change the order in the kmaps.
> >
> > Could you please test below patch (on top) then?
>
> Much better, thank you! Together with the "module_emit_adrp_veneer not
> on kallsyms" mitigation diff, test 1 now passes on arm64 systems both
> with modules loaded, and without, and with and without RANDOMIZE_BASE.
>
> I would send a patch (From: you), but I think you'd do a better job on
> writing the commit text, so would you care to, please?
Sure, what about this?
Thanks,
Namhyung
From 1d9fa3b21bd7a964af43dcc9d486a09917d795e9 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <[email protected]>
Date: Mon, 19 Feb 2018 19:05:45 +0900
Subject: [PATCH] perf tools: set kernel end address properly
The map_groups__fixup_end() was called to set end addresses of kernel
map and module maps. But now machine__create_modules() is set the end
address of modules properly, the only remaining piece is the kernel map.
We can set it with adjacent module's address directly instead of calling
the map_groups__fixup_end(). If there's no module after the kernel map,
the end address will be ~0ULL.
Since it also changes the start address of the kernel map, it needs to
re-insert the map to the kmaps in order to keep a correct ordering. Kim
reported that it caused problems on ARM64.
Reported-by: Kim Phillips <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/machine.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 2eca8478e24f..32d50492505d 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1019,13 +1019,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
return ret;
}
-static void map_groups__fixup_end(struct map_groups *mg)
-{
- int i;
- for (i = 0; i < MAP__NR_TYPES; ++i)
- __map_groups__fixup_end(mg, i);
-}
-
static char *get_kernel_version(const char *root_dir)
{
char version[PATH_MAX];
@@ -1233,6 +1226,7 @@ int machine__create_kernel_maps(struct machine *machine)
{
struct dso *kernel = machine__get_kernel(machine);
const char *name = NULL;
+ struct map *map;
u64 addr = 0;
int ret;
@@ -1259,13 +1253,25 @@ int machine__create_kernel_maps(struct machine *machine)
machine__destroy_kernel_maps(machine);
return -1;
}
- machine__set_kernel_mmap(machine, addr, 0);
+
+ /* we have a real start address now, so re-order the kmaps */
+ map = machine__kernel_map(machine);
+
+ map__get(map);
+ map_groups__remove(&machine->kmaps, map);
+
+ /* assume it's the last in the kmaps */
+ machine__set_kernel_mmap(machine, addr, ~0ULL);
+
+ map_groups__insert(&machine->kmaps, map);
+ map__put(map);
}
- /*
- * Now that we have all the maps created, just set the ->end of them:
- */
- map_groups__fixup_end(&machine->kmaps);
+ /* update end address of the kernel map using adjacent module address */
+ map = map__next(machine__kernel_map(machine));
+ if (map)
+ machine__set_kernel_mmap(machine, addr, map->start);
+
return 0;
}
--
2.16.2
On Thu, Apr 19, 2018 at 06:33:13PM -0500, Kim Phillips wrote:
> On Thu, 19 Apr 2018 11:54:24 +0900
> Namhyung Kim <[email protected]> wrote:
>
> > On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
> > > diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
> > > index 0051b1ee8450..5c4a2e208bbc 100644
> > > --- a/tools/perf/arch/arm64/util/sym-handling.c
> > > +++ b/tools/perf/arch/arm64/util/sym-handling.c
> > > @@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
> > > ehdr.e_type == ET_DYN;
> > > }
> > > #endif
> > > +
> > > +const char *arch__normalize_symbol_name(const char *name)
> > > +{
> > > + /*
> > > + * arm64 kernels compensating for a CPU erratum can put up a
> > > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> > > + */
> > > + if (name && strlen(name) >= 23 &&
> > > + !strncmp(name, "module_emit_adrp_veneer", 23))
> > > + return "module_emit_plt_entry";
> > > +
> > > + return name;
> > > +}
> >
> > I don't know it's always preferable or just for the test. It it's the
> > latter it may be better to move it to the test code.
>
> AFACT, the veneer is a moniker and doesn't technically exist, and
> shouldn't be being looked-up. Both chunks of this diff are needed to
> pass perf test 1: this chunk above is because in
> arch__normalize_symbol_name(), we squash the perf test 1's "<veneer>
> not in *kallsyms*" problem, and in the below chunk, we prevent it
> coming up when the test code iterates over the *vmlinux* symbols. I.e.
> we need to prevent the veneer from coming up in both kallsyms *and*
> vmlinux.
>
> > > diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> > > index 1e5adb65632a..07064e76947d 100644
> > > --- a/tools/perf/tests/vmlinux-kallsyms.c
> > > +++ b/tools/perf/tests/vmlinux-kallsyms.c
> > > @@ -163,6 +163,29 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
> > >
> > > continue;
> > > }
> > > + } else if (pair) {
> > > + s64 skew = mem_start - UM(pair->start);
> > > + struct map *kmap = map_groups__find(&kallsyms.kmaps, type, mem_start);
> > > + struct map *vmap = map_groups__find(&vmlinux.kmaps, type, mem_start);
> > > +
> > > + /*
> > > + * arm64 kernels compensating for a CPU erratum can put up a
> > > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> > > + */
> > > + if (llabs(skew) < page_size)
> >
> > It seems that we needs to check it's the ARM64 at least. If it's a
>
> OK.
>
> > rare case we might need to add more paranoid checks.
>
> It's certainly rare: Adding the authors of the veneer to cc for
> comments:
>
> Will, Ard, how probable are veneer-style symbols such as the
> one introduced in commit a257e0257 "arm64/kernel: don't ban ADRP to
> work around Cortex-A53 erratum #843419" to happen again in the future?
>
> I would have thought WARNing on within-a-pagesize would be OK,
> Namhyung. Are you suggesting checking instead for a hardcoded veneer
> symbol string?
Anything to prevent false-negatives (possibly on other archs).
Thanks,
Namhyung
>
> Thanks,
>
> Kim
>
> > > + {
> > > + pr_debug("NO ERR FOR SKEW %ld: %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
> > > + skew, mem_start, sym->name, UM(pair->start), pair->name);
> > > + continue;
> > > + }
> > > +
> > > + pr_debug("ERR : %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
> > > + mem_start, sym->name, UM(pair->start), pair->name);
> > > +
> > > + if (kmap && vmap) {
> > > + pr_debug(" : map v: %s k: %s\n",
> > > + vmap->dso->short_name, kmap->dso->short_name);
> > > + }
> > > } else
> > > pr_debug("ERR : %#" PRIx64 ": %s not on kallsyms\n",
> > > mem_start, sym->name);
On 20 April 2018 at 01:33, Kim Phillips <[email protected]> wrote:
> On Thu, 19 Apr 2018 11:54:24 +0900
> Namhyung Kim <[email protected]> wrote:
>
>> On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
>> > diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
>> > index 0051b1ee8450..5c4a2e208bbc 100644
>> > --- a/tools/perf/arch/arm64/util/sym-handling.c
>> > +++ b/tools/perf/arch/arm64/util/sym-handling.c
>> > @@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
>> > ehdr.e_type == ET_DYN;
>> > }
>> > #endif
>> > +
>> > +const char *arch__normalize_symbol_name(const char *name)
>> > +{
>> > + /*
>> > + * arm64 kernels compensating for a CPU erratum can put up a
>> > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
>> > + */
>> > + if (name && strlen(name) >= 23 &&
>> > + !strncmp(name, "module_emit_adrp_veneer", 23))
>> > + return "module_emit_plt_entry";
>> > +
>> > + return name;
>> > +}
>>
>> I don't know it's always preferable or just for the test. It it's the
>> latter it may be better to move it to the test code.
>
> AFACT, the veneer is a moniker and doesn't technically exist, and
> shouldn't be being looked-up. Both chunks of this diff are needed to
> pass perf test 1: this chunk above is because in
> arch__normalize_symbol_name(), we squash the perf test 1's "<veneer>
> not in *kallsyms*" problem, and in the below chunk, we prevent it
> coming up when the test code iterates over the *vmlinux* symbols. I.e.
> we need to prevent the veneer from coming up in both kallsyms *and*
> vmlinux.
>
I don't have all the context here, so I don't know what exactly
arch__normalize_symbol_name() is trying to accomplish.
What I do know is that module_emit_adrp_veneer() and
module_emit_plt_entry() are not part of the veneer themselves: they
are ordinary routines that are part of the module loader, and which
populate the allocated veneer space on demand when encountering ADRP
instructions that need to be rerouted.
>> > diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
>> > index 1e5adb65632a..07064e76947d 100644
>> > --- a/tools/perf/tests/vmlinux-kallsyms.c
>> > +++ b/tools/perf/tests/vmlinux-kallsyms.c
>> > @@ -163,6 +163,29 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
>> >
>> > continue;
>> > }
>> > + } else if (pair) {
>> > + s64 skew = mem_start - UM(pair->start);
>> > + struct map *kmap = map_groups__find(&kallsyms.kmaps, type, mem_start);
>> > + struct map *vmap = map_groups__find(&vmlinux.kmaps, type, mem_start);
>> > +
>> > + /*
>> > + * arm64 kernels compensating for a CPU erratum can put up a
>> > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
>> > + */
>> > + if (llabs(skew) < page_size)
>>
>> It seems that we needs to check it's the ARM64 at least. If it's a
>
> OK.
>
>> rare case we might need to add more paranoid checks.
>
> It's certainly rare: Adding the authors of the veneer to cc for
> comments:
>
> Will, Ard, how probable are veneer-style symbols such as the
> one introduced in commit a257e0257 "arm64/kernel: don't ban ADRP to
> work around Cortex-A53 erratum #843419" to happen again in the future?
>
Distro kernels typically enable full KASLR, so on systems that
implement EFI_RNG_PROTOCOL, all function calls from modules into the
kernel proper are redirected via veneers. (Note that these are bl
instructions not adrp instructions though).
> I would have thought WARNing on within-a-pagesize would be OK,
> Namhyung. Are you suggesting checking instead for a hardcoded veneer
> symbol string?
>
Veneers don't have symbol strings. Veneers are anonymous sequences of
instructions living in a patch of R-X mapped module space somewhere.
The only symbol strings are for the routines that generate these
veneers, not for the veneers themselves.
>> > + pr_debug("NO ERR FOR SKEW %ld: %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
>> > + skew, mem_start, sym->name, UM(pair->start), pair->name);
>> > + continue;
>> > + }
>> > +
>> > + pr_debug("ERR : %#" PRIx64 ": diff start addr v: %s k: %#" PRIx64 " %s\n",
>> > + mem_start, sym->name, UM(pair->start), pair->name);
>> > +
>> > + if (kmap && vmap) {
>> > + pr_debug(" : map v: %s k: %s\n",
>> > + vmap->dso->short_name, kmap->dso->short_name);
>> > + }
>> > } else
>> > pr_debug("ERR : %#" PRIx64 ": %s not on kallsyms\n",
>> > mem_start, sym->name);
On Fri, 20 Apr 2018 08:59:15 +0900
Namhyung Kim <[email protected]> wrote:
> The map_groups__fixup_end() was called to set end addresses of kernel
> map and module maps. But now machine__create_modules() is set the end
> address of modules properly, the only remaining piece is the kernel map.
> We can set it with adjacent module's address directly instead of calling
> the map_groups__fixup_end(). If there's no module after the kernel map,
> the end address will be ~0ULL.
>
> Since it also changes the start address of the kernel map, it needs to
> re-insert the map to the kmaps in order to keep a correct ordering. Kim
> reported that it caused problems on ARM64.
>
> Reported-by: Kim Phillips <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
Acked-by: Kim Phillips <[email protected]>
Thanks,
Kim
Em Fri, Apr 20, 2018 at 06:23:58PM -0500, Kim Phillips escreveu:
> On Fri, 20 Apr 2018 08:59:15 +0900
> Namhyung Kim <[email protected]> wrote:
>
> > The map_groups__fixup_end() was called to set end addresses of kernel
> > map and module maps. But now machine__create_modules() is set the end
> > address of modules properly, the only remaining piece is the kernel map.
> > We can set it with adjacent module's address directly instead of calling
> > the map_groups__fixup_end(). If there's no module after the kernel map,
> > the end address will be ~0ULL.
> >
> > Since it also changes the start address of the kernel map, it needs to
> > re-insert the map to the kmaps in order to keep a correct ordering. Kim
> > reported that it caused problems on ARM64.
> >
> > Reported-by: Kim Phillips <[email protected]>
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
>
> Acked-by: Kim Phillips <[email protected]>
So as you actually seem to have tested this, I'm taking this as a
Tested-by instead, to better reflect what you did, ok?
- Arnaldo
On Mon, 23 Apr 2018 10:52:21 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Fri, Apr 20, 2018 at 06:23:58PM -0500, Kim Phillips escreveu:
> > On Fri, 20 Apr 2018 08:59:15 +0900
> > Namhyung Kim <[email protected]> wrote:
> >
> > > The map_groups__fixup_end() was called to set end addresses of kernel
> > > map and module maps. But now machine__create_modules() is set the end
> > > address of modules properly, the only remaining piece is the kernel map.
> > > We can set it with adjacent module's address directly instead of calling
> > > the map_groups__fixup_end(). If there's no module after the kernel map,
> > > the end address will be ~0ULL.
> > >
> > > Since it also changes the start address of the kernel map, it needs to
> > > re-insert the map to the kmaps in order to keep a correct ordering. Kim
> > > reported that it caused problems on ARM64.
> > >
> > > Reported-by: Kim Phillips <[email protected]>
> > > Signed-off-by: Namhyung Kim <[email protected]>
> > > ---
> >
> > Acked-by: Kim Phillips <[email protected]>
>
> So as you actually seem to have tested this, I'm taking this as a
> Tested-by instead, to better reflect what you did, ok?
Sure.
Thanks,
Kim
On Fri, 20 Apr 2018 10:10:32 +0200
Ard Biesheuvel <[email protected]> wrote:
> On 20 April 2018 at 01:33, Kim Phillips <[email protected]> wrote:
> > On Thu, 19 Apr 2018 11:54:24 +0900
> > Namhyung Kim <[email protected]> wrote:
> >
> >> On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
> >> > diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
> >> > index 0051b1ee8450..5c4a2e208bbc 100644
> >> > --- a/tools/perf/arch/arm64/util/sym-handling.c
> >> > +++ b/tools/perf/arch/arm64/util/sym-handling.c
> >> > @@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
> >> > ehdr.e_type == ET_DYN;
> >> > }
> >> > #endif
> >> > +
> >> > +const char *arch__normalize_symbol_name(const char *name)
> >> > +{
> >> > + /*
> >> > + * arm64 kernels compensating for a CPU erratum can put up a
> >> > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
> >> > + */
> >> > + if (name && strlen(name) >= 23 &&
> >> > + !strncmp(name, "module_emit_adrp_veneer", 23))
> >> > + return "module_emit_plt_entry";
> >> > +
> >> > + return name;
> >> > +}
> >>
> >> I don't know it's always preferable or just for the test. It it's the
> >> latter it may be better to move it to the test code.
> >
> > AFACT, the veneer is a moniker and doesn't technically exist, and
> > shouldn't be being looked-up. Both chunks of this diff are needed to
> > pass perf test 1: this chunk above is because in
> > arch__normalize_symbol_name(), we squash the perf test 1's "<veneer>
> > not in *kallsyms*" problem, and in the below chunk, we prevent it
> > coming up when the test code iterates over the *vmlinux* symbols. I.e.
> > we need to prevent the veneer from coming up in both kallsyms *and*
> > vmlinux.
>
> I don't have all the context here, so I don't know what exactly
> arch__normalize_symbol_name() is trying to accomplish.
Sorry about that. The problem is that perf test 1 ("vmlinux
symtab matches kallsyms") is failing, because
module_emit_adrp_veneer exists in /boot/vmlinux but doesn't exist
in /proc/kallsyms, and, sure enough, it's not in the latter.
> What I do know is that module_emit_adrp_veneer() and
> module_emit_plt_entry() are not part of the veneer themselves: they
> are ordinary routines that are part of the module loader, and which
> populate the allocated veneer space on demand when encountering ADRP
> instructions that need to be rerouted.
...
> > >> rare case we might need to add more paranoid checks.
> >
> > It's certainly rare: Adding the authors of the veneer to cc for
> > comments:
> >
> > Will, Ard, how probable are veneer-style symbols such as the
> > one introduced in commit a257e0257 "arm64/kernel: don't ban ADRP to
> > work around Cortex-A53 erratum #843419" to happen again in the future?
> >
>
> Distro kernels typically enable full KASLR, so on systems that
> implement EFI_RNG_PROTOCOL, all function calls from modules into the
> kernel proper are redirected via veneers. (Note that these are bl
> instructions not adrp instructions though).
>
> > I would have thought WARNing on within-a-pagesize would be OK,
> > Namhyung. Are you suggesting checking instead for a hardcoded veneer
> > symbol string?
>
> Veneers don't have symbol strings. Veneers are anonymous sequences of
> instructions living in a patch of R-X mapped module space somewhere.
> The only symbol strings are for the routines that generate these
> veneers, not for the veneers themselves.
OK, thanks, so AFAICT this means the function module_emit_adrp_veneer()
should still show up in /proc/kallsyms, but it's not. I'm also seeing
some weird characters, e.g., here there's a byte with a binary 02 value
following a weird symbol named 'L14472' (part of the xfs module?):
$ grep L1 /proc/kallsyms | hexdump -C | head
<snip>
00000540 30 30 30 30 30 20 72 20 2e 4c 31 34 34 37 32 02 |00000 r .L14472.|
<snip>
arch__normalize_symbol_name() is a place where architecture code can
clean up symbol names in perf, and I thought module_emit_adrp_veneer()
was a veneer itself, but it seems that's not the case, so the
literal string check for it shouldn't be needed. The test is still
failing though because it doesn't show up in kallsyms...
Thanks,
Kim
On 23 April 2018 at 23:43, Kim Phillips <[email protected]> wrote:
> On Fri, 20 Apr 2018 10:10:32 +0200
> Ard Biesheuvel <[email protected]> wrote:
>
>> On 20 April 2018 at 01:33, Kim Phillips <[email protected]> wrote:
>> > On Thu, 19 Apr 2018 11:54:24 +0900
>> > Namhyung Kim <[email protected]> wrote:
>> >
>> >> On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote:
>> >> > diff --git a/tools/perf/arch/arm64/util/sym-handling.c b/tools/perf/arch/arm64/util/sym-handling.c
>> >> > index 0051b1ee8450..5c4a2e208bbc 100644
>> >> > --- a/tools/perf/arch/arm64/util/sym-handling.c
>> >> > +++ b/tools/perf/arch/arm64/util/sym-handling.c
>> >> > @@ -20,3 +20,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
>> >> > ehdr.e_type == ET_DYN;
>> >> > }
>> >> > #endif
>> >> > +
>> >> > +const char *arch__normalize_symbol_name(const char *name)
>> >> > +{
>> >> > + /*
>> >> > + * arm64 kernels compensating for a CPU erratum can put up a
>> >> > + * module_emit_adrp_veneer in place of a module_emit_plt_entry
>> >> > + */
>> >> > + if (name && strlen(name) >= 23 &&
>> >> > + !strncmp(name, "module_emit_adrp_veneer", 23))
>> >> > + return "module_emit_plt_entry";
>> >> > +
>> >> > + return name;
>> >> > +}
>> >>
>> >> I don't know it's always preferable or just for the test. It it's the
>> >> latter it may be better to move it to the test code.
>> >
>> > AFACT, the veneer is a moniker and doesn't technically exist, and
>> > shouldn't be being looked-up. Both chunks of this diff are needed to
>> > pass perf test 1: this chunk above is because in
>> > arch__normalize_symbol_name(), we squash the perf test 1's "<veneer>
>> > not in *kallsyms*" problem, and in the below chunk, we prevent it
>> > coming up when the test code iterates over the *vmlinux* symbols. I.e.
>> > we need to prevent the veneer from coming up in both kallsyms *and*
>> > vmlinux.
>>
>> I don't have all the context here, so I don't know what exactly
>> arch__normalize_symbol_name() is trying to accomplish.
>
> Sorry about that. The problem is that perf test 1 ("vmlinux
> symtab matches kallsyms") is failing, because
> module_emit_adrp_veneer exists in /boot/vmlinux but doesn't exist
> in /proc/kallsyms, and, sure enough, it's not in the latter.
>
>> What I do know is that module_emit_adrp_veneer() and
>> module_emit_plt_entry() are not part of the veneer themselves: they
>> are ordinary routines that are part of the module loader, and which
>> populate the allocated veneer space on demand when encountering ADRP
>> instructions that need to be rerouted.
> ...
>> > >> rare case we might need to add more paranoid checks.
>> >
>> > It's certainly rare: Adding the authors of the veneer to cc for
>> > comments:
>> >
>> > Will, Ard, how probable are veneer-style symbols such as the
>> > one introduced in commit a257e0257 "arm64/kernel: don't ban ADRP to
>> > work around Cortex-A53 erratum #843419" to happen again in the future?
>> >
>>
>> Distro kernels typically enable full KASLR, so on systems that
>> implement EFI_RNG_PROTOCOL, all function calls from modules into the
>> kernel proper are redirected via veneers. (Note that these are bl
>> instructions not adrp instructions though).
>>
>> > I would have thought WARNing on within-a-pagesize would be OK,
>> > Namhyung. Are you suggesting checking instead for a hardcoded veneer
>> > symbol string?
>>
>> Veneers don't have symbol strings. Veneers are anonymous sequences of
>> instructions living in a patch of R-X mapped module space somewhere.
>> The only symbol strings are for the routines that generate these
>> veneers, not for the veneers themselves.
>
> OK, thanks, so AFAICT this means the function module_emit_adrp_veneer()
> should still show up in /proc/kallsyms, but it's not. I'm also seeing
> some weird characters, e.g., here there's a byte with a binary 02 value
> following a weird symbol named 'L14472' (part of the xfs module?):
>
> $ grep L1 /proc/kallsyms | hexdump -C | head
> <snip>
> 00000540 30 30 30 30 30 20 72 20 2e 4c 31 34 34 37 32 02 |00000 r .L14472.|
> <snip>
>
> arch__normalize_symbol_name() is a place where architecture code can
> clean up symbol names in perf, and I thought module_emit_adrp_veneer()
> was a veneer itself, but it seems that's not the case, so the
> literal string check for it shouldn't be needed. The test is still
> failing though because it doesn't show up in kallsyms...
>
This turns out to be an unintended side effect of the fact that we
(I?) taught kallsyms to disregard symbols ending in "_veneer"
So we should probably rename the function, and everything will be fine.
On Tue, 24 Apr 2018 08:13:57 +0200
Ard Biesheuvel <[email protected]> wrote:
> On 23 April 2018 at 23:43, Kim Phillips <[email protected]> wrote:
> > arch__normalize_symbol_name() is a place where architecture code can
> > clean up symbol names in perf, and I thought module_emit_adrp_veneer()
> > was a veneer itself, but it seems that's not the case, so the
> > literal string check for it shouldn't be needed. The test is still
> > failing though because it doesn't show up in kallsyms...
>
> This turns out to be an unintended side effect of the fact that we
> (I?) taught kallsyms to disregard symbols ending in "_veneer"
>
> So we should probably rename the function, and everything will be fine.
ok so what should we rename module_emit_adrp_veneer to?:
module_emit_adrp_veneer_
module_emit_adrp_veneer_fn
module_emit_adrp_veneer_nokallsyms
or..?
Kim
On 24 April 2018 at 14:50, Kim Phillips <[email protected]> wrote:
> On Tue, 24 Apr 2018 08:13:57 +0200
> Ard Biesheuvel <[email protected]> wrote:
>
>> On 23 April 2018 at 23:43, Kim Phillips <[email protected]> wrote:
>> > arch__normalize_symbol_name() is a place where architecture code can
>> > clean up symbol names in perf, and I thought module_emit_adrp_veneer()
>> > was a veneer itself, but it seems that's not the case, so the
>> > literal string check for it shouldn't be needed. The test is still
>> > failing though because it doesn't show up in kallsyms...
>>
>> This turns out to be an unintended side effect of the fact that we
>> (I?) taught kallsyms to disregard symbols ending in "_veneer"
>>
>> So we should probably rename the function, and everything will be fine.
>
> ok so what should we rename module_emit_adrp_veneer to?:
>
> module_emit_adrp_veneer_
> module_emit_adrp_veneer_fn
> module_emit_adrp_veneer_nokallsyms
>
> or..?
>
How about
module_emit_veneer_for_adrp
?
Commit a257e02579e ("arm64/kernel: don't ban ADRP to work around
Cortex-A53 erratum #843419") introduced a function whose name ends with
"_veneer".
This clashes with commit bd8b22d2888e ("Kbuild: kallsyms: ignore veneers
emitted by the ARM linker"), which removes symbols ending in "_veneer"
from kallsyms.
The problem was manifested as 'perf test -vvvvv vmlinux' failed,
correctly claiming the symbol 'module_emit_adrp_veneer' was present in
vmlinux, but not in kallsyms.
...
ERR : 0xffff00000809aa58: module_emit_adrp_veneer not on kallsyms
...
test child finished with -1
---- end ----
vmlinux symtab matches kallsyms: FAILED!
Fix the problem by renaming module_emit_veneer_for_adrp,
module_emit_veneer_for_adrp. Now the test passes.
Fixes: a257e02579e ("arm64/kernel: don't ban ADRP to work around Cortex-A53 erratum #843419")
Cc: Ard Biesheuvel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Michal Marek <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
arch/arm64/include/asm/module.h | 2 +-
arch/arm64/kernel/module-plts.c | 2 +-
arch/arm64/kernel/module.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index b6dbbe3123a9..97d0ef12e2ff 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -39,7 +39,7 @@ struct mod_arch_specific {
u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym);
-u64 module_emit_adrp_veneer(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index fa3637284a3d..f0690c2ca3e0 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -43,7 +43,7 @@ u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
}
#ifdef CONFIG_ARM64_ERRATUM_843419
-u64 module_emit_adrp_veneer(struct module *mod, void *loc, u64 val)
+u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
{
struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
&mod->arch.init;
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 719fde8dcc19..155fd91e78f4 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -215,7 +215,7 @@ static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val)
insn &= ~BIT(31);
} else {
/* out of range for ADR -> emit a veneer */
- val = module_emit_adrp_veneer(mod, place, val & ~0xfff);
+ val = module_emit_veneer_for_adrp(mod, place, val & ~0xfff);
if (!val)
return -ENOEXEC;
insn = aarch64_insn_gen_branch_imm((u64)place, val,
--
2.17.0
On 24 April 2018 at 17:13, Kim Phillips <[email protected]> wrote:
> Commit a257e02579e ("arm64/kernel: don't ban ADRP to work around
> Cortex-A53 erratum #843419") introduced a function whose name ends with
> "_veneer".
>
> This clashes with commit bd8b22d2888e ("Kbuild: kallsyms: ignore veneers
> emitted by the ARM linker"), which removes symbols ending in "_veneer"
> from kallsyms.
>
> The problem was manifested as 'perf test -vvvvv vmlinux' failed,
> correctly claiming the symbol 'module_emit_adrp_veneer' was present in
> vmlinux, but not in kallsyms.
>
> ...
> ERR : 0xffff00000809aa58: module_emit_adrp_veneer not on kallsyms
> ...
> test child finished with -1
> ---- end ----
> vmlinux symtab matches kallsyms: FAILED!
>
> Fix the problem by renaming module_emit_veneer_for_adrp,
> module_emit_veneer_for_adrp.
That sentence didn't come out right
> Now the test passes.
>
> Fixes: a257e02579e ("arm64/kernel: don't ban ADRP to work around Cortex-A53 erratum #843419")
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Michal Marek <[email protected]>
> Signed-off-by: Kim Phillips <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
> ---
> arch/arm64/include/asm/module.h | 2 +-
> arch/arm64/kernel/module-plts.c | 2 +-
> arch/arm64/kernel/module.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index b6dbbe3123a9..97d0ef12e2ff 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -39,7 +39,7 @@ struct mod_arch_specific {
> u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
> Elf64_Sym *sym);
>
> -u64 module_emit_adrp_veneer(struct module *mod, void *loc, u64 val);
> +u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
>
> #ifdef CONFIG_RANDOMIZE_BASE
> extern u64 module_alloc_base;
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
> index fa3637284a3d..f0690c2ca3e0 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/kernel/module-plts.c
> @@ -43,7 +43,7 @@ u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
> }
>
> #ifdef CONFIG_ARM64_ERRATUM_843419
> -u64 module_emit_adrp_veneer(struct module *mod, void *loc, u64 val)
> +u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
> {
> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
> &mod->arch.init;
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 719fde8dcc19..155fd91e78f4 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -215,7 +215,7 @@ static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val)
> insn &= ~BIT(31);
> } else {
> /* out of range for ADR -> emit a veneer */
> - val = module_emit_adrp_veneer(mod, place, val & ~0xfff);
> + val = module_emit_veneer_for_adrp(mod, place, val & ~0xfff);
> if (!val)
> return -ENOEXEC;
> insn = aarch64_insn_gen_branch_imm((u64)place, val,
> --
> 2.17.0
>
Commit a257e02579e ("arm64/kernel: don't ban ADRP to work around
Cortex-A53 erratum #843419") introduced a function whose name ends with
"_veneer".
This clashes with commit bd8b22d2888e ("Kbuild: kallsyms: ignore veneers
emitted by the ARM linker"), which removes symbols ending in "_veneer"
from kallsyms.
The problem was manifested as 'perf test -vvvvv vmlinux' failed,
correctly claiming the symbol 'module_emit_adrp_veneer' was present in
vmlinux, but not in kallsyms.
...
ERR : 0xffff00000809aa58: module_emit_adrp_veneer not on kallsyms
...
test child finished with -1
---- end ----
vmlinux symtab matches kallsyms: FAILED!
Fix the problem by renaming module_emit_adrp_veneer to
module_emit_veneer_for_adrp. Now the test passes.
Fixes: a257e02579e ("arm64/kernel: don't ban ADRP to work around Cortex-A53 erratum #843419")
Acked-by: Ard Biesheuvel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Michal Marek <[email protected]>
Signed-off-by: Kim Phillips <[email protected]>
---
v2: fixed naming and wording of last sentence in commit text.
arch/arm64/include/asm/module.h | 2 +-
arch/arm64/kernel/module-plts.c | 2 +-
arch/arm64/kernel/module.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index b6dbbe3123a9..97d0ef12e2ff 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -39,7 +39,7 @@ struct mod_arch_specific {
u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym);
-u64 module_emit_adrp_veneer(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index fa3637284a3d..f0690c2ca3e0 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -43,7 +43,7 @@ u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
}
#ifdef CONFIG_ARM64_ERRATUM_843419
-u64 module_emit_adrp_veneer(struct module *mod, void *loc, u64 val)
+u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
{
struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
&mod->arch.init;
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 719fde8dcc19..155fd91e78f4 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -215,7 +215,7 @@ static int reloc_insn_adrp(struct module *mod, __le32 *place, u64 val)
insn &= ~BIT(31);
} else {
/* out of range for ADR -> emit a veneer */
- val = module_emit_adrp_veneer(mod, place, val & ~0xfff);
+ val = module_emit_veneer_for_adrp(mod, place, val & ~0xfff);
if (!val)
return -ENOEXEC;
insn = aarch64_insn_gen_branch_imm((u64)place, val,
--
2.17.0
Commit-ID: ee05d21791db6db954bbb7b79bb18d88b5f6b7ff
Gitweb: https://git.kernel.org/tip/ee05d21791db6db954bbb7b79bb18d88b5f6b7ff
Author: Namhyung Kim <[email protected]>
AuthorDate: Mon, 19 Feb 2018 19:05:45 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 23 Apr 2018 10:52:55 -0300
perf machine: Set main kernel end address properly
map_groups__fixup_end() was called to set the end addresses of kernel
and module maps. But now since machine__create_modules() sets the end
address of modules properly, the only remaining piece is the kernel map.
We can set it with adjacent module's address directly instead of calling
map_groups__fixup_end(). If there's no module after the kernel map, the
end address will be ~0ULL.
Since it also changes the start address of the kernel map, it needs to
re-insert the map to the kmaps in order to keep a correct ordering. Kim
reported that it caused problems on ARM64.
Reported-by: Kim Phillips <[email protected]>
Tested-by: Kim Phillips <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/20180419235915.GA19067@sejong
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/machine.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 2eca8478e24f..32d50492505d 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1019,13 +1019,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
return ret;
}
-static void map_groups__fixup_end(struct map_groups *mg)
-{
- int i;
- for (i = 0; i < MAP__NR_TYPES; ++i)
- __map_groups__fixup_end(mg, i);
-}
-
static char *get_kernel_version(const char *root_dir)
{
char version[PATH_MAX];
@@ -1233,6 +1226,7 @@ int machine__create_kernel_maps(struct machine *machine)
{
struct dso *kernel = machine__get_kernel(machine);
const char *name = NULL;
+ struct map *map;
u64 addr = 0;
int ret;
@@ -1259,13 +1253,25 @@ int machine__create_kernel_maps(struct machine *machine)
machine__destroy_kernel_maps(machine);
return -1;
}
- machine__set_kernel_mmap(machine, addr, 0);
+
+ /* we have a real start address now, so re-order the kmaps */
+ map = machine__kernel_map(machine);
+
+ map__get(map);
+ map_groups__remove(&machine->kmaps, map);
+
+ /* assume it's the last in the kmaps */
+ machine__set_kernel_mmap(machine, addr, ~0ULL);
+
+ map_groups__insert(&machine->kmaps, map);
+ map__put(map);
}
- /*
- * Now that we have all the maps created, just set the ->end of them:
- */
- map_groups__fixup_end(&machine->kmaps);
+ /* update end address of the kernel map using adjacent module address */
+ map = map__next(machine__kernel_map(machine));
+ if (map)
+ machine__set_kernel_mmap(machine, addr, map->start);
+
return 0;
}