Re: [PATCH] perf tools: set kernel end address properly
On 24 April 2018 at 14:50, Kim Phillips wrote: > On Tue, 24 Apr 2018 08:13:57 +0200 > Ard Biesheuvel wrote: > >> On 23 April 2018 at 23:43, Kim Phillips 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 ?
Re: [PATCH] perf tools: set kernel end address properly
On Tue, 24 Apr 2018 08:13:57 +0200 Ard Biesheuvel wrote: > On 23 April 2018 at 23:43, Kim Phillips 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
Re: [PATCH] perf tools: set kernel end address properly
On 23 April 2018 at 23:43, Kim Phillips wrote:
> On Fri, 20 Apr 2018 10:10:32 +0200
> Ard Biesheuvel wrote:
>
>> On 20 April 2018 at 01:33, Kim Phillips wrote:
>> > On Thu, 19 Apr 2018 11:54:24 +0900
>> > Namhyung Kim 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 "
>> > 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
>
> 0540 30 30 30 30 30 20 72 20 2e 4c 31 34 34 37 32 02 |0 r .L14472.|
>
>
> 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.
Re: [PATCH] perf tools: set kernel end address properly
On Fri, 20 Apr 2018 10:10:32 +0200
Ard Biesheuvel wrote:
> On 20 April 2018 at 01:33, Kim Phillips wrote:
> > On Thu, 19 Apr 2018 11:54:24 +0900
> > Namhyung Kim 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 "
> > 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
0540 30 30 30 30 30 20 72 20 2e 4c 31 34 34 37 32 02 |0 r .L14472.|
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
Re: [PATCH] perf tools: set kernel end address properly
On Mon, 23 Apr 2018 10:52:21 -0300 Arnaldo Carvalho de Melo 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 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 > > > Signed-off-by: Namhyung Kim > > > --- > > > > Acked-by: Kim Phillips > > 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
Re: [PATCH] perf tools: set kernel end address properly
Em Fri, Apr 20, 2018 at 06:23:58PM -0500, Kim Phillips escreveu: > On Fri, 20 Apr 2018 08:59:15 +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, 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 > > Signed-off-by: Namhyung Kim > > --- > > Acked-by: Kim Phillips 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
Re: [PATCH] perf tools: set kernel end address properly
On Fri, 20 Apr 2018 08:59:15 +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, 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 > Signed-off-by: Namhyung Kim > --- Acked-by: Kim Phillips Thanks, Kim
Re: [PATCH] perf tools: set kernel end address properly
On 20 April 2018 at 01:33, Kim Phillips wrote:
> On Thu, 19 Apr 2018 11:54:24 +0900
> Namhyung Kim 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 "
> 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);
>> > +
Re: [PATCH] perf tools: set kernel end address properly
On Thu, Apr 19, 2018 at 06:33:13PM -0500, Kim Phillips wrote:
> On Thu, 19 Apr 2018 11:54:24 +0900
> Namhyung Kim 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 "
> 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);
Re: [PATCH] perf tools: set kernel end address properly
On Thu, Apr 19, 2018 at 06:20:24PM -0500, Kim Phillips wrote:
> On Thu, 19 Apr 2018 11:37:12 +0900
> Namhyung Kim 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
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
Signed-off-by: Namhyung Kim
---
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
Re: [PATCH] perf tools: set kernel end address properly
On Thu, 19 Apr 2018 11:54:24 +0900
Namhyung Kim 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 "
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);
Re: [PATCH] perf tools: set kernel end address properly
On Thu, 19 Apr 2018 11:37:12 +0900 Namhyung Kim 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 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 : 0x28081000: do_undefinstr not on kallsyms > > > > > ERR : 0x280810b8: do_sysinstr not on kallsyms > > > > > ERR : 0x28081258: do_debug_exception not on kallsyms > > > > > ERR : 0x28081648: do_mem_abort not on kallsyms > > > > > ERR : 0x280818b8: do_el0_irq_bp_hardening not on kallsyms > > > > > $ sudo ./perf test -vv 1 |& tail > > > > > ERR : 0x2a1d37c8: tramp_exit_native not on kallsyms > > > > > ERR : 0x2a1d37e8: tramp_exit_compat not on kallsyms > > > > > ERR : 0x2a1d4000: __entry_tramp_text_end not on kallsyms > > > > > WARN: Maps only in vmlinux: > > > > > 2808-28081000 1 [kernel].head.text > > > > > 2aec-2aff7548 2e5 [kernel].init.text > > > > > 2aff7548-2b0126d4 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 0x2808. > > > > > > For the vmlinux, right? > > > > yes, map__next(machine__kernel_map(machine)) has the start address > > of the single module currently loaded: > > > > 2229 t $x [arm_ccn] > > 2229 t arm_ccn_pmu_events_is_visible[arm_ccn] > > > > The beginning of the kernel is..later: > > > > 2808 t _head > > 2808 T _text > > > > and its end according to grep -w _end /proc/kallsyms is: > > > > 2d5f9000 B _end > > > > but end was assigned to the beginning of arm_ccn (0x2229), > > 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
Re: [PATCH] perf tools: set kernel end address properly
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);
Re: [PATCH] perf tools: set kernel end address properly
On Wed, Apr 18, 2018 at 07:37:59PM -0500, Kim Phillips wrote: > On Tue, 17 Apr 2018 11:27:26 +0900 > Namhyung Kim 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 : 0x28081000: do_undefinstr not on kallsyms > > > > ERR : 0x280810b8: do_sysinstr not on kallsyms > > > > ERR : 0x28081258: do_debug_exception not on kallsyms > > > > ERR : 0x28081648: do_mem_abort not on kallsyms > > > > ERR : 0x280818b8: do_el0_irq_bp_hardening not on kallsyms > > > > $ sudo ./perf test -vv 1 |& tail > > > > ERR : 0x2a1d37c8: tramp_exit_native not on kallsyms > > > > ERR : 0x2a1d37e8: tramp_exit_compat not on kallsyms > > > > ERR : 0x2a1d4000: __entry_tramp_text_end not on kallsyms > > > > WARN: Maps only in vmlinux: > > > > 2808-28081000 1 [kernel].head.text > > > > 2aec-2aff7548 2e5 [kernel].init.text > > > > 2aff7548-2b0126d4 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 0x2808. > > > > For the vmlinux, right? > > yes, map__next(machine__kernel_map(machine)) has the start address > of the single module currently loaded: > > 2229 t $x [arm_ccn] > 2229 t arm_ccn_pmu_events_is_visible [arm_ccn] > > The beginning of the kernel is..later: > > 2808 t _head > 2808 T _text > > and its end according to grep -w _end /proc/kallsyms is: > > 2d5f9000 B _end > > but end was assigned to the beginning of arm_ccn (0x2229), > 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; }
Re: [PATCH] perf tools: set kernel end address properly
'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 : 0x44eb0a6c9620: module_emit_adrp_veneer not on kallsyms
I got:
ERR : 0x44eb0a6c9620: diff addr v: module_emit_adrp_veneer k:
0x44eb0a6c9500 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 -v 1' with
no modules loaded (now passes test):
- http://paste.ubuntu.com/p/xBgpjTyjFp/
- output of the above perf, called with 'test -v 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);
Re: [PATCH] perf tools: set kernel end address properly
On Mon, Apr 16, 2018 at 05:48:11PM -0500, Kim Phillips wrote:
> On Mon, 16 Apr 2018 12:24:07 -0500
> Kim Phillips wrote:
>
> > On Mon, 16 Apr 2018 13:58:00 -0300
> > Arnaldo Carvalho de Melo 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 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 : 0x28081000: do_undefinstr not on kallsyms
> > ERR : 0x280810b8: do_sysinstr not on kallsyms
> > ERR : 0x28081258: do_debug_exception not on kallsyms
> > ERR : 0x28081648: do_mem_abort not on kallsyms
> > ERR : 0x280818b8: do_el0_irq_bp_hardening not on kallsyms
> > $ sudo ./perf test -vv 1 |& tail
> > ERR : 0x2a1d37c8: tramp_exit_native not on kallsyms
> > ERR : 0x2a1d37e8: tramp_exit_compat not on kallsyms
> > ERR : 0x2a1d4000: __entry_tramp_text_end not on kallsyms
> > WARN: Maps only in vmlinux:
> > 2808-28081000 1 [kernel].head.text
> > 2aec-2aff7548 2e5 [kernel].init.text
> > 2aff7548-2b0126d4 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 0x2808.
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);
Re: [PATCH] perf tools: set kernel end address properly
On Mon, 16 Apr 2018 12:24:07 -0500 Kim Phillips wrote: > On Mon, 16 Apr 2018 13:58:00 -0300 > Arnaldo Carvalho de Melo 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 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 : 0x28081000: do_undefinstr not on kallsyms > ERR : 0x280810b8: do_sysinstr not on kallsyms > ERR : 0x28081258: do_debug_exception not on kallsyms > ERR : 0x28081648: do_mem_abort not on kallsyms > ERR : 0x280818b8: do_el0_irq_bp_hardening not on kallsyms > $ sudo ./perf test -vv 1 |& tail > ERR : 0x2a1d37c8: tramp_exit_native not on kallsyms > ERR : 0x2a1d37e8: tramp_exit_compat not on kallsyms > ERR : 0x2a1d4000: __entry_tramp_text_end not on kallsyms > WARN: Maps only in vmlinux: > 2808-28081000 1 [kernel].head.text > 2aec-2aff7548 2e5 [kernel].init.text > 2aff7548-2b0126d4 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 0x2808. 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
Re: [PATCH] perf tools: set kernel end address properly
On Mon, 16 Apr 2018 13:58:00 -0300 Arnaldo Carvalho de Melo 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 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 : 0x28081000: do_undefinstr not on kallsyms ERR : 0x280810b8: do_sysinstr not on kallsyms ERR : 0x28081258: do_debug_exception not on kallsyms ERR : 0x28081648: do_mem_abort not on kallsyms ERR : 0x280818b8: do_el0_irq_bp_hardening not on kallsyms $ sudo ./perf test -vv 1 |& tail ERR : 0x2a1d37c8: tramp_exit_native not on kallsyms ERR : 0x2a1d37e8: tramp_exit_compat not on kallsyms ERR : 0x2a1d4000: __entry_tramp_text_end not on kallsyms WARN: Maps only in vmlinux: 2808-28081000 1 [kernel].head.text 2aec-2aff7548 2e5 [kernel].init.text 2aff7548-2b0126d4 2f87548 [kernel].exit.text test child finished with -1 end vmlinux symtab matches kallsyms: FAILED! $ Kim
Re: [PATCH] perf tools: set kernel end address properly
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 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
Re: [PATCH] perf tools: set kernel end address properly
On Mon, 16 Apr 2018 10:51:25 -0300 Arnaldo Carvalho de Melo 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
Re: [PATCH] perf tools: set kernel end address properly
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
> > Signed-off-by: Namhyung Kim
>
> looks good
>
> Acked-by: Jiri Olsa
>
> 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
> >
Re: [PATCH] perf tools: set kernel end address properly
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
> Signed-off-by: Namhyung Kim
looks good
Acked-by: Jiri Olsa
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
>
[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 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
Signed-off-by: Namhyung Kim
---
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
