Received-SPF: pass (google.com: domain of linux-kernel+bounces-54199-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240202110130.3553-1-adrian.hunter@intel.com>
 <20240202110130.3553-3-adrian.hunter@intel.com> <Zb2akm1MGCv84T-7@google.com> <046baaea-217f-4ecc-b340-b5f6f1cee0b9@intel.com>
In-Reply-To: <046baaea-217f-4ecc-b340-b5f6f1cee0b9@intel.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Mon, 5 Feb 2024 18:21:46 -0800
Message-ID: <CAM9d7cgZi8jDDgHrAqJmLvz33VXkLb3KKiJ4uayEZniTnaFbBA@mail.gmail.com>
Subject: Re: [PATCH 2/2] perf symbols: Slightly improve module file executable
 section mappings
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>, 
	Like Xu <like.xu.linux@gmail.com>, linux-kernel@vger.kernel.org, 
	linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Feb 4, 2024 at 10:58=E2=80=AFPM Adrian Hunter <adrian.hunter@intel.=
com> wrote:
>
> On 3/02/24 03:44, Namhyung Kim wrote:
> > Hi Adrian,
> >
> > On Fri, Feb 02, 2024 at 01:01:30PM +0200, Adrian Hunter wrote:
> >> Currently perf does not record module section addresses except for
> >> the .text section. In general that means perf cannot get module sectio=
n
> >> mappings correct (except for .text) when loading symbols from a kernel
> >> module file. (Note using --kcore does not have this issue)
> >>
> >> Improve that situation slightly by identifying executable sections tha=
t
> >> use the same mapping as the .text section. That happens when an
> >> executable section comes directly after the .text section, both in mem=
ory
> >> and on file, something that can be determined by following the same la=
yout
> >> rules used by the kernel, refer kernel layout_sections(). Note whether
> >> that happens is somewhat arbitrary, so this is not a final solution.
> >>
> >> Example from tracing a virtual machine process:
> >>
> >>  Before:
> >>
> >>   $ perf script | grep unknown
> >>          CPU 0/KVM    1718   203.511270:     318341 cpu-cycles:P:  fff=
fffffc13e8a70 [unknown] (/lib/modules/6.7.2-local/kernel/arch/x86/kvm/kvm-i=
ntel.ko)
> >>   $ perf script -vvv 2>&1 >/dev/null | grep kvm.intel | grep 'noinstr.=
text\|ffff'
> >>   Map: 0-7e0 41430 [kvm_intel].noinstr.text
> >>   Map: ffffffffc13a7000-ffffffffc1421000 a0 /lib/modules/6.7.2-local/k=
ernel/arch/x86/kvm/kvm-intel.ko
> >>
> >>  After:
> >>
> >>   $ perf script | grep 203.511270
> >>          CPU 0/KVM    1718   203.511270:     318341 cpu-cycles:P:  fff=
fffffc13e8a70 vmx_vmexit+0x0 (/lib/modules/6.7.2-local/kernel/arch/x86/kvm/=
kvm-intel.ko)
> >>   $ perf script -vvv 2>&1 >/dev/null | grep kvm.intel | grep 'noinstr.=
text\|ffff'
> >>   Map: ffffffffc13a7000-ffffffffc1421000 a0 /lib/modules/6.7.2-local/k=
ernel/arch/x86/kvm/kvm-intel.ko
> >>
> >> Reported-by: Like Xu <like.xu.linux@gmail.com>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> ---
> >>  tools/perf/util/symbol-elf.c | 75 +++++++++++++++++++++++++++++++++++=
-
> >>  1 file changed, 73 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf=
c
> >> index 9e7eeaf616b8..98bf0881aaf6 100644
> >> --- a/tools/perf/util/symbol-elf.c
> >> +++ b/tools/perf/util/symbol-elf.c
> >> @@ -23,6 +23,7 @@
> >>  #include <linux/ctype.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/zalloc.h>
> >> +#include <linux/string.h>
> >>  #include <symbol/kallsyms.h>
> >>  #include <internal/lib.h>
> >>
> >> @@ -1329,6 +1330,58 @@ int symsrc__init(struct symsrc *ss, struct dso =
*dso, const char *name,
> >>      return -1;
> >>  }
> >>
> >> +static bool is_exe_text(int flags)
> >> +{
> >> +    return (flags & (SHF_ALLOC | SHF_EXECINSTR)) =3D=3D (SHF_ALLOC | =
SHF_EXECINSTR);
> >> +}
> >> +
> >> +/*
> >> + * Some executable module sections like .noinstr.text might be laid o=
ut with
> >> + * .text so they can use the same mapping (memory address to file off=
set).
> >> + * Check if that is the case. Refer to kernel layout_sections(). Retu=
rn the
> >> + * maximum offset.
> >> + */
> >> +static u64 max_text_section(Elf *elf, GElf_Ehdr *ehdr)
> >> +{
> >> +    Elf_Scn *sec =3D NULL;
> >> +    GElf_Shdr shdr;
> >> +    u64 offs =3D 0;
> >> +
> >> +    /* Doesn't work for some arch */
> >> +    if (ehdr->e_machine =3D=3D EM_PARISC ||
> >> +        ehdr->e_machine =3D=3D EM_ALPHA)
> >> +            return 0;
> >> +
> >> +    /* ELF is corrupted/truncated, avoid calling elf_strptr. */
> >> +    if (!elf_rawdata(elf_getscn(elf, ehdr->e_shstrndx), NULL))
> >> +            return 0;
> >> +
> >> +    while ((sec =3D elf_nextscn(elf, sec)) !=3D NULL) {
> >> +            char *sec_name;
> >> +
> >> +            if (!gelf_getshdr(sec, &shdr))
> >> +                    break;
> >> +
> >> +            if (!is_exe_text(shdr.sh_flags))
> >> +                    continue;
> >> +
> >> +            /* .init and .exit sections are not placed with .text */
> >> +            sec_name =3D elf_strptr(elf, ehdr->e_shstrndx, shdr.sh_na=
me);
> >> +            if (!sec_name ||
> >> +                strstarts(sec_name, ".init") ||
> >> +                strstarts(sec_name, ".exit"))
> >> +                    break;
> >
> > Do we really need this?  It seems my module has .init.text section
> > next to .text.
> >
> >   $ readelf -SW /lib/modules/`uname -r`/kernel/fs/ext4/ext4.ko
> >   There are 77 section headers, starting at offset 0x252e90:
> >
> >   Section Headers:
> >     [Nr] Name              Type            Address          Off    Size=
   ES Flg Lk Inf Al
> >     [ 0]                   NULL            0000000000000000 000000 0000=
00 00      0   0  0
> >     [ 1] .text             PROGBITS        0000000000000000 000040 079f=
a7 00  AX  0   0 16
> >     [ 2] .rela.text        RELA            0000000000000000 13c348 04f0=
c8 18   I 74   1  8
> >     [ 3] .init.text        PROGBITS        0000000000000000 079ff0 0006=
0c 00  AX  0   0 16
> >     ...
> >
> >
> > ALIGN(0x40 + 0x79fa7, 16) =3D 0x79ff0, right?
>
> But not in memory e.g.
>
> Section Headers:
>   [Nr] Name              Type            Address          Off    Size   E=
S Flg Lk Inf Al
>   [ 3] .text             PROGBITS        0000000000000000 0000a0 071719 0=
0  AX  0   0 16
>   [ 5] .text.unlikely    PROGBITS        0000000000000000 0717b9 000a59 0=
0  AX  0   0  1
>   [ 7] .init.text        PROGBITS        0000000000000000 072212 0004fe 0=
0  AX  0   0  1
>   [ 9] .altinstr_replacement PROGBITS    0000000000000000 072710 000004 0=
0  AX  0   0  1
>   [10] .static_call.text PROGBITS        0000000000000000 072714 000388 0=
0  AX  0   0  4
>   [12] .exit.text        PROGBITS        0000000000000000 072a9c 000078 0=
0  AX  0   0  1
>
>
> /sys/module/ext4/sections/.text:                 0xffffffffc0453000
> /sys/module/ext4/sections/.text.unlikely:        0xffffffffc04c4719
> /sys/module/ext4/sections/.init.text:            0xffffffffc053e000
> /sys/module/ext4/sections/.altinstr_replacement: 0xffffffffc04c5172
> /sys/module/ext4/sections/.static_call.text:     0xffffffffc04c5178
> /sys/module/ext4/sections/.exit.text:            0xffffffffc04c5500
>
> Need to have:
>
>         section address - offset =3D=3D .text address - .text offset
>
> perf does not record the section address, but the kernel
> layout_sections() lays out executable sections in order
> starting with .text *until* it gets to .init* or .exit*.

Ok, thanks for the explanation!

Namhyung