CoreSight October 2024

coresight@lists.linaro.org

13 participants
29 discussions

Re: [PATCH V8 1/4] Add dso__is_pie call to identify ELF PIE

by Steve Clevenger

On 10/9/2024 2:13 PM, Leo Yan wrote: > On 9/9/2024 10:30 PM, Steve Clevenger wrote: >> >> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for >> the DF_1_PIE flag. This identifies position executable code. >> >> Signed-off-by: Steve Clevenger <scclevenger(a)os.amperecomputing.com> >> Reviewed-by: Leo Yan <leo.yan(a)arm.com> >> --- >> tools/perf/util/symbol-elf.c | 61 ++++++++++++++++++++++++++++++++++++ >> tools/perf/util/symbol.h | 1 + >> 2 files changed, 62 insertions(+) >> >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c >> index e398abfd13a0..babe47976922 100644 >> --- a/tools/perf/util/symbol-elf.c >> +++ b/tools/perf/util/symbol-elf.c >> @@ -662,6 +662,67 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, >> return err; >> } >> >> +/* >> + * Check dynamic section DT_FLAGS_1 for a Position Independent >> + * Executable (PIE). >> + */ >> +bool dso__is_pie(struct dso *dso) >> +{ >> + Elf *elf = NULL; >> + Elf_Scn *scn = NULL; >> + GElf_Ehdr ehdr; >> + GElf_Shdr shdr; >> + bool is_pie = false; >> + char dso_path[PATH_MAX]; >> + int fd = -1; >> + >> + if (!dso || (elf_version(EV_CURRENT) == EV_NONE)) >> + goto exit; // false >> + >> + dso__build_id_filename(dso, dso_path, sizeof(dso_path), false); >> + >> + fd = open(dso_path, O_RDONLY); >> + >> + if (fd < 0) { >> + pr_debug("%s: cannot read cached %s.\n", __func__, dso_path); >> + goto exit; // false >> + } >> + >> + elf = elf_begin(fd, ELF_C_READ, NULL); >> + gelf_getehdr(elf, &ehdr); >> + >> + if (ehdr.e_type == ET_DYN) { >> + Elf_Data *data; >> + GElf_Dyn *entry; >> + int n_entries = shdr.sh_size / sizeof(GElf_Dyn); > > I took time to play this series on Arm64 machine and found issue in above > sentence. As the 'shdr' strucutre is read out from elf_section_by_name() > below, but it calculates the entries before reading out the section header. > Therefore, I observed that program will not be detected as PIE executable due > to 'n_entries' is 0. > > With fixing this bug, then I observed the regression caused by patch 02. > > Below are steps: > > - Build test program with below command: > > # gcc -pie -Wl,-z,relro,-z,now -o test test.c > # readelf readelf -a test | grep FLAGS > 0x000000000000001e (FLAGS) BIND_NOW > 0x000000006ffffffb (FLAGS_1) Flags: NOW PIE > > The test program is a simple infinite loop. I run this program in a docker > container with Fedora 40. > > - Record trace data: > > # perf record -e cycles -p 149531 > # perf script > > test 149531 483168.078485: 4986 cycles: aaaacde401b4 > [unknown] (/home/test) > test 149531 483168.078564: 134207 cycles: aaaacde401b4 > [unknown] (/home/test) > test 149531 483168.079305: 1257097 cycles: aaaacde401b4 > [unknown] (/home/test) > > You can see it fails to parse symbol and print out [unknown]. > > After I reverted patch 02 in this series, then: > > # perf script > test 149531 483168.078485: 4986 cycles: aaaacde401b4 > main+0xc (/home/test) > test 149531 483168.078564: 134207 cycles: aaaacde401b4 > main+0xc (/home/test) > test 149531 483168.079305: 1257097 cycles: aaaacde401b4 > main+0xc (/home/test) > > Not sure if I miss anything for PIE executable, seems to me, we should drop > the first two patches and just pass pg_off to python script? > > Thanks, > Leo Hi Leo, I verified the perf/arm-cs-trace-disasm.py script patch results for CoreSight user and kernel trace using Fedora distros 37 and higher. I did little to test non-CoreSight trace, but I do see the dso__is_pie initialization error, and the problem revealed by your test program. As a side effect, the problem would contribute to the CoreSight user instruction test passes. The arm-cs-trace-disasm.py python script has a legacy IF block to force 'dso_vm_start' to zero for kernel code. Similarly, map_pgoff gets forced to zero here. See the adjacent comment pertaining to memory offsets for kernel dso and executable file dso. This, I think, is the root of the misunderstanding with regard to the PIE check. I see the python IF comparison includes a check for dso_start == 0x400000 but I see no information as to why this hardcoded value is relevant. Do you know? perf has (or now has) the information (e.g. map__rip_2objdump, map__objdump_2mem) needed to make all objdump address adjustments prior to calling the script. The script could even be made redundant as suggested in an earlier thread. In the meantime, Ampere needs this working so the patch implementation has to work with the current perf/script implementation. I agree the revised approach is to pass pgoff to the updated script unmodified.This means patch 3 will pass map_pgoff unmodified by MAPPING_TYPE into the dictionary. Patch 1-2 goes away, and patch 4 remains as-is. Steve > >> + >> + scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL); >> + if (!scn) >> + goto exit_close; // false >> + >> + data = (Elf_Data *) elf_getdata(scn, NULL); >> + if (!data || !data->d_buf) >> + goto exit_close; // false >> + >> + // check DT_FLAGS_1 >> + for (int i = 0; i < n_entries; i++) { >> + entry = ((GElf_Dyn *) data->d_buf) + i; >> + if (entry->d_tag == DT_FLAGS_1) { >> + if ((entry->d_un.d_val & DF_1_PIE) != 0) { >> + is_pie = true; >> + break; >> + } >> + } >> + } // end for >> + } >> + >> +exit_close: >> + elf_end(elf); >> + close(fd); >> +exit: >> + return is_pie; >> +} >> + >> /* >> * We need to check if we have a .dynsym, so that we can handle the >> * .plt, synthesizing its symbols, that aren't on the symtabs (be it >> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h >> index 3fb5d146d9b1..33ea2596ce31 100644 >> --- a/tools/perf/util/symbol.h >> +++ b/tools/perf/util/symbol.h >> @@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso, >> struct symbol *sym); >> void dso__delete_symbol(struct dso *dso, >> struct symbol *sym); >> +bool dso__is_pie(struct dso *dso); >> >> struct symbol *dso__find_symbol(struct dso *dso, u64 addr); >> struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr); >> -- >> 2.44.0 >>

2 months, 1 week

[PATCH v4 RESEND 0/3] coresight: Add static trace id support

by Mao Jinlong

Some HW has static trace id which cannot be changed via software programming. For this case, configure the trace id in device tree with "arm,static-trace-id = <xxx>", and call coresight_trace_id_get_static_system_id with the trace id value in device probe function. The id will be reserved for the HW all the time if the device is probed. Changes since V3: 1. Adda new API function int coresight_trace_id_get_system_static_id(int trace_id). 2. Use the term "static trace id" for these devices where the hardware sets a non-programmable trace ID. Changes since V2: 1. Change "trace-id" to "arm,trace-id". 2. Add trace id flag for getting preferred id or ODD id. Changes since V1: 1. Add argument to coresight_trace_id_get_system_id for preferred id instead of adding new function coresight_trace_id_reserve_system_id. 2. Add constraint to trace-id in dt-binding file. Mao Jinlong (3): dt-bindings: arm: Add arm,trace-id for coresight dummy source coresight: Add support to get static id for system trace sources coresight: dummy: Add static trace id support for dummy source .../sysfs-bus-coresight-devices-dummy-source | 15 +++++ .../arm/arm,coresight-dummy-source.yaml | 6 ++ drivers/hwtracing/coresight/coresight-dummy.c | 59 +++++++++++++++++-- .../hwtracing/coresight/coresight-platform.c | 26 ++++++++ .../hwtracing/coresight/coresight-trace-id.c | 38 ++++++++---- .../hwtracing/coresight/coresight-trace-id.h | 9 +++ include/linux/coresight.h | 1 + 7 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-dummy-source -- 2.46.0

2 months, 2 weeks

Re: [PATCH v5 RESEND 0/5] Coresight: Add Coresight TMC Control Unit driver

by Suzuki K Poulose

On 09/10/2024 10:55, Jie Gan wrote: > The Coresight TMC Control Unit(CTCU) device hosts miscellaneous configuration > registers to control various features related to TMC ETR device. > Please rebase this on the v6.12-rc1, which has the sink specific trace id allocation changes and drop the "Depends-on" tags on the patches which don't make any sense at all. Suzuki

2 months, 2 weeks

Re: [PATCH v1] Coresight: Narrow down the matching range of tpdm

by Suzuki K Poulose

On 09/10/2024 09:50, Songwei Chai wrote: > The format of tpdm's peripheral id is 1f0exx. To avoid potential > conflicts in the future, update the .id_table's id to 0x001f0e00. > This update will narrow down the matching range and prevent incorrect > matches. For example, another component's peripheral id might be > f0e00, which would incorrectly match the old id. > > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index b7d99e91ab84..8e2985d17549 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -1308,8 +1308,8 @@ static void tpdm_remove(struct amba_device *adev) > */ > static struct amba_id tpdm_ids[] = { > { > - .id = 0x000f0e00, > - .mask = 0x000fff00, > + .id = 0x001f0e00, > + .mask = 0x00ffff00, > }, > { 0, 0, NULL }, > }; > Please could you add "Fixes" tag for this ? You don't want an old kernel detect something else as TPDM, so this must go to stable kernels. Also, while at it, please could you intend the id and mask values with tabs, so it is easier to what exactly we are matching ? e.g, .id = 0x001f0e00, .mask = 0x00ffff00, Kind regards Suzuki

2 months, 2 weeks

Re: [PATCH V8 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset

by Steve Clevenger

On 10/8/2024 2:00 PM, Namhyung Kim wrote: > On Tue, Oct 08, 2024 at 11:24:45AM -0700, Steve Clevenger wrote: >> >> Hello Namhyung, >> >> Sorry your question is so late. I don't include the ELF headers here, >> but the problem can be seen with a perf.data packet dump of user >> instruction trace capture. The problem is with the non-zero pgoff. The >> arm-cs-trace-disasm.py script was never passed pgoff information to >> adjust the start/end disassemble range passed to objdump. This patch >> distributes the fix between perf and the arm-cs-trace-disasm.py script. >> >> Here's a brief excerpt from an e-mail I sent to James Clark describing >> the patch before I submitted it. > > Oh, is it applied to the coresight tree already? I don't think I got > any notification for that. Please make sure to notify perf maintainers > (and hopefullly to get Ack's) when you touch non-coresight part of the > code. > > Also I think we need to clarify how to route coresight tool patches. I > thought they are going through the perf-tools tree. But it doesn't seem > to be the case? > > Anyway, sorry about being late on this. FYI. These patch(es) were based on perf-tools-next at revision 6.11.0-rc3. All ack'd by Leo Yan. If they could get merged to perf-tools, that would be great! > >> >> ----------------------------------------------------------------- >> . >> . >> This e-mail is to document what I know and don’t know about the problem. >> The background is Fedora 38 introduced non-zero text offsets into common >> shared objects and executables (e.g. libc.so.6, etc.). >> >> If you were to ‘perf report --dump’ the perf.data of a user mode >> instruction trace on Fedora 37 and grep the PERF_RECORD_MMAP2 packets >> you’ll notice all zero values (@ 0) for shared binary and executable >> text offsets. Repeat the same for user trace collected on Fedora 38/39, >> and these text offsets show as non-zero. >> >> Fedora 37: >> >> 4294967295 18446744073709551615 0x8ac8 [0x78]: PERF_RECORD_MMAP2 >> 2389577/2389577: [0xffff85124000(0x42000) @ 0 103:04 805306555 >> 1941233998]: r-xp /usr/lib/ld-linux-aarch64.so.1 >> 4294967295 18446744073709551615 0x8b40 [0x60]: PERF_RECORD_MMAP2 >> 2389577/2389577: [0xffff85161000(0x1000) @ 0 00:00 0 0]: r-xp [vdso] >> 4294967295 18446744073709551615 0x8ba0 [0x68]: PERF_RECORD_MMAP2 >> 2389577/2389577: [0xaaaab09b0000(0x10000) @ 0 103:04 1140851919 >> 994218038]: r-xp /usr/bin/dd >> 4294967295 18446744073709551615 0x8c08 [0x70]: PERF_RECORD_MMAP2 >> 2389577/2389577: [0xffff84f70000(0x1a9000) @ 0 103:04 1677721733 >> 3891973508]: r-xp /usr/lib64/libc.so.6 >> >> Fedora 39: >> >> 4294967295 18446744073709551615 0x8ac8 [0x78]: PERF_RECORD_MMAP2 >> 18229/18229: [0xffffa5512000(0x1d000) @ 0x10000 103:04 161 4093340249]: >> r-xp /usr/lib/ld-linux-aarch64.so.1 >> 4294967295 18446744073709551615 0x8b40 [0x60]: PERF_RECORD_MMAP2 >> 18229/18229: [0xffffa554f000(0x1000) @ 0 00:00 0 0]: r-xp [vdso] >> 4294967295 18446744073709551615 0x8ba0 [0x68]: PERF_RECORD_MMAP2 >> 18229/18229: [0xaaaade7e0000(0xb000) @ 0x10000 103:04 536876423 >> 421616320]: r-xp /usr/bin/dd >> 4294967295 18446744073709551615 0x8c08 [0x70]: PERF_RECORD_MMAP2 >> 18229/18229: [0xffffa5360000(0x11f000) @ 0x30000 103:04 536873199 >> 2415801053]: r-xp /usr/lib64/libc.so.6 >> >> The arm-cs-trace-disasm.py script never gets to see the text offset into >> the dso binaries, so has no opportunity to adjust the start/end address >> range passed to objdump. This wasn’t a problem on Fedora 37 and below >> since there’s no start/end adjustment for a zero text offset. On Fedora >> 38/39 distros, the instruction trace shows unconditional branch >> instructions which do not branch to the target address, the clearest >> indication of trouble. > > Ok, thanks for sharing this. I'm ok with exposing pgoff to the python > script itself. But I'd like to make sure if it works correctly with > PIEs that have non-zero page offsets in other places (probably ok). > > Thanks, > Namhyung > > >> ----------------------------------------------------------------- >> >> On 10/7/2024 10:51 PM, Namhyung Kim wrote: >>> Hello, >>> >>> Sorry for the long delay. But can you please explain your problem in >>> more detail? Showing ELF program (or section) header would be helpful >>> as well. >>> >>> Is the problem when the text mapping has non-zero pgoff only? Is the >>> kernel symbols working correctly? Or is it just the Python script >>> broken? >>> >>> Thanks, >>> Namhyung >>> >>> On Mon, Sep 09, 2024 at 03:30:02PM -0600, Steve Clevenger wrote: >>>> Changes in V8: >>>> - in arm-cs-trace-disasm.py, ensure map_pgoff is not converted to >>>> string. >>>> - Remove map_pgoff integer conversion in dso not found print >>>> message. >>>> >>>> Changes in V7: >>>> - In arm-cs-trace-disasm.py, fix print message core dump resulting >>>> from mixed type arithmetic. >>>> - Modify CS_ETM_TRACE_ON filter to filter zero start_addr. The >>>> CS_ETM_TRACE_ON message is changed to print only in verbose mode. >>>> - Removed verbose mode only notification for start_addr/stop_addr >>>> outside of dso address range. >>>> >>>> Changes in V6: >>>> - In arm-cs-trace-disasm.py, zero map_pgoff for kernel files. Add >>>> map_pgoff to start/end address for dso not found message. >>>> - Added "Reviewed-by" trailer for patches 1-3 previously reviewed >>>> by Leo Yan in V4 and V5. >>>> >>>> Changes in V5: >>>> - In symbol-elf.c, branch to exit_close label if open file. >>>> - In trace_event_python.c, correct indentation. set_sym_in_dict >>>> call parameter "map_pgoff" renamed as "addr_map_pgoff" to >>>> match local naming. >>>> >>>> Changes in V4: >>>> - In trace-event-python.c, fixed perf-tools-next merge problem. >>>> >>>> Changes in V3: >>>> - Rebased to linux-perf-tools branch. >>>> - Squash symbol-elf.c and symbol.h into same commit. >>>> - In map.c, merge dso__is_pie() call into existing if statement. >>>> - In arm-cs-trace-disasm.py, remove debug artifacts. >>>> >>>> Changes in V2: >>>> - In dso__is_pie() (symbol-elf.c), Decrease indentation, add null pointer >>>> checks per Leo Yan review. >>>> - Updated mailing list distribution. >>>> >>>> Steve Clevenger (4): >>>> Add dso__is_pie call to identify ELF PIE >>>> Force MAPPING_TYPE__IDENTIY for PIE >>>> Add map pgoff to python dictionary based on MAPPING_TYPE >>>> Adjust objdump start/end range per map pgoff parameter >>>> >>>> .../scripts/python/arm-cs-trace-disasm.py | 17 ++++-- >>>> tools/perf/util/map.c | 4 +- >>>> .../scripting-engines/trace-event-python.c | 13 +++- >>>> tools/perf/util/symbol-elf.c | 61 +++++++++++++++++++ >>>> tools/perf/util/symbol.h | 1 + >>>> 5 files changed, 86 insertions(+), 10 deletions(-) >>>> >>>> -- >>>> 2.44.0 >>>> >>

2 months, 2 weeks

Re: [PATCH V8 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset

by Steve Clevenger

Hello Namhyung, Sorry your question is so late. I don't include the ELF headers here, but the problem can be seen with a perf.data packet dump of user instruction trace capture. The problem is with the non-zero pgoff. The arm-cs-trace-disasm.py script was never passed pgoff information to adjust the start/end disassemble range passed to objdump. This patch distributes the fix between perf and the arm-cs-trace-disasm.py script. Here's a brief excerpt from an e-mail I sent to James Clark describing the patch before I submitted it. Regards, Steve C. ----------------------------------------------------------------- . . This e-mail is to document what I know and don’t know about the problem. The background is Fedora 38 introduced non-zero text offsets into common shared objects and executables (e.g. libc.so.6, etc.). If you were to ‘perf report --dump’ the perf.data of a user mode instruction trace on Fedora 37 and grep the PERF_RECORD_MMAP2 packets you’ll notice all zero values (@ 0) for shared binary and executable text offsets. Repeat the same for user trace collected on Fedora 38/39, and these text offsets show as non-zero. Fedora 37: 4294967295 18446744073709551615 0x8ac8 [0x78]: PERF_RECORD_MMAP2 2389577/2389577: [0xffff85124000(0x42000) @ 0 103:04 805306555 1941233998]: r-xp /usr/lib/ld-linux-aarch64.so.1 4294967295 18446744073709551615 0x8b40 [0x60]: PERF_RECORD_MMAP2 2389577/2389577: [0xffff85161000(0x1000) @ 0 00:00 0 0]: r-xp [vdso] 4294967295 18446744073709551615 0x8ba0 [0x68]: PERF_RECORD_MMAP2 2389577/2389577: [0xaaaab09b0000(0x10000) @ 0 103:04 1140851919 994218038]: r-xp /usr/bin/dd 4294967295 18446744073709551615 0x8c08 [0x70]: PERF_RECORD_MMAP2 2389577/2389577: [0xffff84f70000(0x1a9000) @ 0 103:04 1677721733 3891973508]: r-xp /usr/lib64/libc.so.6 Fedora 39: 4294967295 18446744073709551615 0x8ac8 [0x78]: PERF_RECORD_MMAP2 18229/18229: [0xffffa5512000(0x1d000) @ 0x10000 103:04 161 4093340249]: r-xp /usr/lib/ld-linux-aarch64.so.1 4294967295 18446744073709551615 0x8b40 [0x60]: PERF_RECORD_MMAP2 18229/18229: [0xffffa554f000(0x1000) @ 0 00:00 0 0]: r-xp [vdso] 4294967295 18446744073709551615 0x8ba0 [0x68]: PERF_RECORD_MMAP2 18229/18229: [0xaaaade7e0000(0xb000) @ 0x10000 103:04 536876423 421616320]: r-xp /usr/bin/dd 4294967295 18446744073709551615 0x8c08 [0x70]: PERF_RECORD_MMAP2 18229/18229: [0xffffa5360000(0x11f000) @ 0x30000 103:04 536873199 2415801053]: r-xp /usr/lib64/libc.so.6 The arm-cs-trace-disasm.py script never gets to see the text offset into the dso binaries, so has no opportunity to adjust the start/end address range passed to objdump. This wasn’t a problem on Fedora 37 and below since there’s no start/end adjustment for a zero text offset. On Fedora 38/39 distros, the instruction trace shows unconditional branch instructions which do not branch to the target address, the clearest indication of trouble. ----------------------------------------------------------------- On 10/7/2024 10:51 PM, Namhyung Kim wrote: > Hello, > > Sorry for the long delay. But can you please explain your problem in > more detail? Showing ELF program (or section) header would be helpful > as well. > > Is the problem when the text mapping has non-zero pgoff only? Is the > kernel symbols working correctly? Or is it just the Python script > broken? > > Thanks, > Namhyung > > On Mon, Sep 09, 2024 at 03:30:02PM -0600, Steve Clevenger wrote: >> Changes in V8: >> - in arm-cs-trace-disasm.py, ensure map_pgoff is not converted to >> string. >> - Remove map_pgoff integer conversion in dso not found print >> message. >> >> Changes in V7: >> - In arm-cs-trace-disasm.py, fix print message core dump resulting >> from mixed type arithmetic. >> - Modify CS_ETM_TRACE_ON filter to filter zero start_addr. The >> CS_ETM_TRACE_ON message is changed to print only in verbose mode. >> - Removed verbose mode only notification for start_addr/stop_addr >> outside of dso address range. >> >> Changes in V6: >> - In arm-cs-trace-disasm.py, zero map_pgoff for kernel files. Add >> map_pgoff to start/end address for dso not found message. >> - Added "Reviewed-by" trailer for patches 1-3 previously reviewed >> by Leo Yan in V4 and V5. >> >> Changes in V5: >> - In symbol-elf.c, branch to exit_close label if open file. >> - In trace_event_python.c, correct indentation. set_sym_in_dict >> call parameter "map_pgoff" renamed as "addr_map_pgoff" to >> match local naming. >> >> Changes in V4: >> - In trace-event-python.c, fixed perf-tools-next merge problem. >> >> Changes in V3: >> - Rebased to linux-perf-tools branch. >> - Squash symbol-elf.c and symbol.h into same commit. >> - In map.c, merge dso__is_pie() call into existing if statement. >> - In arm-cs-trace-disasm.py, remove debug artifacts. >> >> Changes in V2: >> - In dso__is_pie() (symbol-elf.c), Decrease indentation, add null pointer >> checks per Leo Yan review. >> - Updated mailing list distribution. >> >> Steve Clevenger (4): >> Add dso__is_pie call to identify ELF PIE >> Force MAPPING_TYPE__IDENTIY for PIE >> Add map pgoff to python dictionary based on MAPPING_TYPE >> Adjust objdump start/end range per map pgoff parameter >> >> .../scripts/python/arm-cs-trace-disasm.py | 17 ++++-- >> tools/perf/util/map.c | 4 +- >> .../scripting-engines/trace-event-python.c | 13 +++- >> tools/perf/util/symbol-elf.c | 61 +++++++++++++++++++ >> tools/perf/util/symbol.h | 1 + >> 5 files changed, 86 insertions(+), 10 deletions(-) >> >> -- >> 2.44.0 >>

2 months, 2 weeks

Re: [PATCH] coresight: etm4x: Fix PID tracing when perf is run in an init PID namespace

by Suzuki K Poulose

On 08/10/2024 07:52, Leo Yan wrote: > On 10/7/2024 9:05 PM, Leo Yan wrote: >> >> Hi Julien, >> >> On Wed, Sep 25, 2024 at 03:13:56PM +0200, Julien Meunier wrote: >>> The previous implementation limited the tracing capabilities when perf >>> was run in the init PID namespace, making it impossible to trace >>> applications in non-init PID namespaces. >>> >>> This update improves the tracing process by verifying the event owner. >>> This allows us to determine whether the user has the necessary >>> permissions to trace the application. >> >> The original commit aab473867fed is not for constraint permission. It is >> about PID namespace mismatching issue. >> >> E.g. Perf runs in non-root namespace, thus it records process info in the >> non-root PID namespace. On the other hand, Arm CoreSight traces PID for >> root namespace, as a result, it will lead mess when decoding. >> >> With this change, I am not convinced that Arm CoreSight can trace PID for >> non-root PID namespace. Seems to me, the concerned issue is still existed >> - it might cause PID mismatching issue between hardware trace data and >> Perf's process info. > > I thought again and found I was wrong with above conclusion. This patch is a > good fixing for the perf running in root namespace to profile programs in > non-root namespace. Sorry for noise. > > Maybe it is good to improve a bit comments to avoid confusion. See below. > > [...] > >>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> index bf01f01964cf..8365307b1aec 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, >>> >>> /* Only trace contextID when runs in root PID namespace */ > > We can claim the requirement for the *tool* running in root PID namespae. > > /* Only trace contextID when the tool runs in root PID namespace */ minor nit: I wouldn't call "tool". Let keep it "event owner". /* Only trace contextID when the event owner is in root PID namespace */ Julien, Please could you respin the patch with the comments addressed. Kind regards Suzuki > > >>> if ((attr->config & BIT(ETM_OPT_CTXTID)) && >>> - task_is_in_init_pid_ns(current)) >>> + task_is_in_init_pid_ns(event->owner)) >>> /* bit[6], Context ID tracing bit */ >>> config->cfg |= TRCCONFIGR_CID; >>> >>> @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, >>> goto out; >>> } >>> /* Only trace virtual contextID when runs in root PID namespace */ > > Ditto. > > /* Only trace virtual contextID when the tool runs in root PID namespace */ > > With above change: > > Reviewed-by: Leo Yan <leo.yan(a)arm.com> > >>> - if (task_is_in_init_pid_ns(current)) >>> + if (task_is_in_init_pid_ns(event->owner)) >>> config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; >>> } >>> >>> -- >>> 2.34.1 >>> >>>

2 months, 2 weeks

Re: [PATCH] coresight: etm4x: Fix PID tracing when perf is run in an init PID namespace

by Suzuki K Poulose

On 25/09/2024 14:13, Julien Meunier wrote: > The previous implementation limited the tracing capabilities when perf > was run in the init PID namespace, making it impossible to trace > applications in non-init PID namespaces. > > This update improves the tracing process by verifying the event owner. > This allows us to determine whether the user has the necessary > permissions to trace the application. > > Cc: stable(a)vger.kernel.org > Fixes: aab473867fed ("coresight: etm4x: Don't trace PID for non-root PID namespace") > Signed-off-by: Julien Meunier <julien.meunier(a)nokia.com> Thanks for the fix, I will queue this for v6.13 Suzuki > --- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index bf01f01964cf..8365307b1aec 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -695,7 +695,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > > /* Only trace contextID when runs in root PID namespace */ > if ((attr->config & BIT(ETM_OPT_CTXTID)) && > - task_is_in_init_pid_ns(current)) > + task_is_in_init_pid_ns(event->owner)) > /* bit[6], Context ID tracing bit */ > config->cfg |= TRCCONFIGR_CID; > > @@ -710,7 +710,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, > goto out; > } > /* Only trace virtual contextID when runs in root PID namespace */ > - if (task_is_in_init_pid_ns(current)) > + if (task_is_in_init_pid_ns(event->owner)) > config->cfg |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT; > } >

2 months, 2 weeks

Re: [PATCH] perf: cs-etm: Fix the assert() to handle captured and unprocessed cpu trace

by Arnaldo Carvalho de Melo

On Wed, Sep 25, 2024 at 04:04:55PM -0700, Namhyung Kim wrote: > On Wed, Sep 25, 2024 at 10:54:31AM +0100, James Clark wrote: > > > > > > On 25/09/2024 12:39 am, Ilkka Koskinen wrote: > > > If one builds perf with DEBUG=1, captures data on multiple CPUs and > > > finally runs 'perf report -C <cpu>' for only one of the cpus, assert() > > > aborts the program. This happens because there are empty queues with > > > format set. This patch changes the condition to abort only if a queue > > > is not empty and if the format is unset. > > > > > > $ make -C tools/perf DEBUG=1 CORESIGHT=1 CSLIBS=/usr/lib CSINCLUDES=/usr/include install > > > $ perf record -o kcore --kcore -e cs_etm/timestamp/k -s -C 0-1 dd if=/dev/zero of=/dev/null bs=1M count=1 > > > $ perf report --input kcore/data --vmlinux=/home/ikoskine/projects/linux/vmlinux -C 1 > > > Aborted (core dumped) > > > > > > Fixes: 57880a7966be ("perf: cs-etm: Allocate queues for all CPUs") > > > Signed-off-by: Ilkka Koskinen <ilkka(a)os.amperecomputing.com> > > > --- > > > tools/perf/util/cs-etm.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > > > index 90f32f327b9b..40f047baef81 100644 > > > --- a/tools/perf/util/cs-etm.c > > > +++ b/tools/perf/util/cs-etm.c > > > @@ -3323,7 +3323,7 @@ static int cs_etm__create_decoders(struct cs_etm_auxtrace *etm) > > > * Don't create decoders for empty queues, mainly because > > > * etmq->format is unknown for empty queues. > > > */ > > > - assert(empty == (etmq->format == UNSET)); > > > + assert(empty || etmq->format != UNSET); > > > if (empty) > > > continue; > > > > Oops I didn't realize you could filter on CPU in report mode. Thanks for the > > fix. Adding a test to the end of test_arm_coresight.sh might be quite > > useful. Either way: > > > > Reviewed-by: James Clark <james.clark(a)linaro.org> > > Thanks, it should go to the perf-tool. Arnaldo, please pick up. Right, picking it now. Thanks, - Arnaldo

2 months, 3 weeks

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight October 2024