* [PATCH v2] perf record: Add support for arch_sdt_arg_parse_op() on s390
@ 2026-03-13 13:23 Thomas Richter
2026-03-13 20:50 ` Ian Rogers
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Richter @ 2026-03-13 13:23 UTC (permalink / raw)
To: linux-kernel, linux-s390, linux-perf-users, acme, namhyung,
dapeng1.mi
Cc: agordeev, gor, sumanthk, hca, japo, Thomas Richter
commit e5e66adfe45a6 ("perf regs: Remove __weak attributive arch_sdt_arg_parse_op() function")
removes arch_sdt_arg_parse_op() functions. s390 support is missing.
The following warning is printed:
Unknown ELF machine 22, standard arguments parse will be skipped.
ELF machine 22 is the EM_S390 host. This happens with command
# ./perf record -v -- stress-ng -t 1s --matrix 0
on a z/VM system when the event is not specified.
Add s390 specific __perf_sdt_arg_parse_op_s390() function to support
-architecture calls to arch_sdt_arg_parse_op() for s390.
The warning disappears.
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>
Tested-by: Jan Polensky <japo@linux.ibm.com>
---
.../perf/util/perf-regs-arch/perf_regs_s390.c | 89 +++++++++++++++++++
tools/perf/util/perf_regs.c | 3 +
tools/perf/util/perf_regs.h | 1 +
3 files changed, 93 insertions(+)
diff --git a/tools/perf/util/perf-regs-arch/perf_regs_s390.c b/tools/perf/util/perf-regs-arch/perf_regs_s390.c
index c61df24edf0f..c830aeae606e 100644
--- a/tools/perf/util/perf-regs-arch/perf_regs_s390.c
+++ b/tools/perf/util/perf-regs-arch/perf_regs_s390.c
@@ -1,7 +1,13 @@
// SPDX-License-Identifier: GPL-2.0
+#include <errno.h>
+#include <regex.h>
#include "../perf_regs.h"
#include "../../arch/s390/include/perf_regs.h"
+#include "debug.h"
+
+#include <linux/zalloc.h>
+#include <linux/kernel.h>
uint64_t __perf_reg_mask_s390(bool intr __maybe_unused)
{
@@ -95,3 +101,86 @@ uint64_t __perf_reg_sp_s390(void)
{
return PERF_REG_S390_R15;
}
+
+/* %rXX */
+#define SDT_OP_REGEX1 "^%r([0-9]|1[0-5])$"
+/* -###(%rXX) */
+#define SDT_OP_REGEX2 "^(-?[0-9]+)\\(%r([0-9]|1[0-5])\\)$"
+static regex_t sdt_op_regex1, sdt_op_regex2;
+
+static int sdt_init_op_regex(void)
+{
+ static int initialized;
+ int ret = 0;
+
+ if (initialized)
+ return 0;
+
+ ret = regcomp(&sdt_op_regex1, SDT_OP_REGEX1, REG_EXTENDED);
+ if (ret)
+ goto error;
+ initialized = 1;
+
+ ret = regcomp(&sdt_op_regex2, SDT_OP_REGEX2, REG_EXTENDED);
+ if (ret)
+ goto free_regex1;
+ initialized = 2;
+
+ return 0;
+
+free_regex1:
+ regfree(&sdt_op_regex1);
+error:
+ pr_debug4("Regex compilation error, initialized %d\n", initialized);
+ initialized = 0;
+ return ret;
+}
+
+/*
+ * Parse OP and convert it into uprobe format, which is, +/-NUM(%gprREG).
+ * Possible variants of OP are:
+ * Format Example
+ * -------------------------
+ * NUM(%rREG) 48(%r1)
+ * -NUM(%rREG) -48(%r1)
+ * %rREG %r1
+ */
+int __perf_sdt_arg_parse_op_s390(char *old_op, char **new_op)
+{
+ int ret, new_len;
+ regmatch_t rm[6];
+ unsigned long i;
+
+ *new_op = NULL;
+ ret = sdt_init_op_regex();
+ if (ret < 0)
+ return ret;
+
+ if (!regexec(&sdt_op_regex1, old_op, 3, rm, 0)) {
+ /* Extract %rX */
+ new_len = 2; /* % NULL */
+ new_len += (int)(rm[1].rm_eo - rm[1].rm_so);
+ *new_op = zalloc(new_len);
+ if (!*new_op)
+ return -ENOMEM;
+
+ scnprintf(*new_op, new_len, "%%%.*s",
+ (int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so);
+ } else if (!regexec(&sdt_op_regex2, old_op, ARRAY_SIZE(rm), rm, 0)) {
+ /* Extract #(%rX) */
+ new_len = 4; /* (%)NULL */
+ for (i = 1; i < ARRAY_SIZE(rm) && rm[i].rm_so != -1; ++i)
+ new_len += (int)(rm[i].rm_eo - rm[i].rm_so);
+ *new_op = zalloc(new_len);
+ if (!*new_op)
+ return -ENOMEM;
+
+ scnprintf(*new_op, new_len, "%.*s(%%%.*s)",
+ (int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so,
+ (int)(rm[2].rm_eo - rm[2].rm_so), old_op + rm[2].rm_so);
+ } else {
+ pr_debug4("Skipping unsupported SDT argument: %s\n", old_op);
+ return SDT_ARG_SKIP;
+ }
+
+ return SDT_ARG_VALID;
diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
index 5b8f34beb24e..f52b0e1f7fc7 100644
--- a/tools/perf/util/perf_regs.c
+++ b/tools/perf/util/perf_regs.c
@@ -23,6 +23,9 @@ int perf_sdt_arg_parse_op(uint16_t e_machine, char *old_op, char **new_op)
case EM_X86_64:
ret = __perf_sdt_arg_parse_op_x86(old_op, new_op);
break;
+ case EM_S390:
+ ret = __perf_sdt_arg_parse_op_s390(old_op, new_op);
+ break;
default:
pr_debug("Unknown ELF machine %d, standard arguments parse will be skipped.\n",
e_machine);
diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
index 7c04700bf837..573f0d1dfe04 100644
--- a/tools/perf/util/perf_regs.h
+++ b/tools/perf/util/perf_regs.h
@@ -62,6 +62,7 @@ uint64_t __perf_reg_mask_s390(bool intr);
const char *__perf_reg_name_s390(int id);
uint64_t __perf_reg_ip_s390(void);
uint64_t __perf_reg_sp_s390(void);
+int __perf_sdt_arg_parse_op_s390(char *old_op, char **new_op);
int __perf_sdt_arg_parse_op_x86(char *old_op, char **new_op);
uint64_t __perf_reg_mask_x86(bool intr);
--
2.53.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] perf record: Add support for arch_sdt_arg_parse_op() on s390 2026-03-13 13:23 [PATCH v2] perf record: Add support for arch_sdt_arg_parse_op() on s390 Thomas Richter @ 2026-03-13 20:50 ` Ian Rogers 2026-03-17 8:40 ` Thomas Richter 0 siblings, 1 reply; 3+ messages in thread From: Ian Rogers @ 2026-03-13 20:50 UTC (permalink / raw) To: Thomas Richter Cc: linux-kernel, linux-s390, linux-perf-users, acme, namhyung, dapeng1.mi, agordeev, gor, sumanthk, hca, japo On Fri, Mar 13, 2026 at 6:33 AM Thomas Richter <tmricht@linux.ibm.com> wrote: > > commit e5e66adfe45a6 ("perf regs: Remove __weak attributive arch_sdt_arg_parse_op() function") > removes arch_sdt_arg_parse_op() functions. s390 support is missing. > The following warning is printed: > > Unknown ELF machine 22, standard arguments parse will be skipped. > > ELF machine 22 is the EM_S390 host. This happens with command > # ./perf record -v -- stress-ng -t 1s --matrix 0 > on a z/VM system when the event is not specified. > > Add s390 specific __perf_sdt_arg_parse_op_s390() function to support > -architecture calls to arch_sdt_arg_parse_op() for s390. > The warning disappears. > > Signed-off-by: Thomas Richter <tmricht@linux.ibm.com> > Cc: Dapeng Mi <dapeng1.mi@linux.intel.com> > Tested-by: Jan Polensky <japo@linux.ibm.com> > --- > .../perf/util/perf-regs-arch/perf_regs_s390.c | 89 +++++++++++++++++++ > tools/perf/util/perf_regs.c | 3 + > tools/perf/util/perf_regs.h | 1 + > 3 files changed, 93 insertions(+) > > diff --git a/tools/perf/util/perf-regs-arch/perf_regs_s390.c b/tools/perf/util/perf-regs-arch/perf_regs_s390.c > index c61df24edf0f..c830aeae606e 100644 > --- a/tools/perf/util/perf-regs-arch/perf_regs_s390.c > +++ b/tools/perf/util/perf-regs-arch/perf_regs_s390.c > @@ -1,7 +1,13 @@ > // SPDX-License-Identifier: GPL-2.0 > > +#include <errno.h> > +#include <regex.h> > #include "../perf_regs.h" > #include "../../arch/s390/include/perf_regs.h" > +#include "debug.h" > + > +#include <linux/zalloc.h> > +#include <linux/kernel.h> > > uint64_t __perf_reg_mask_s390(bool intr __maybe_unused) > { > @@ -95,3 +101,86 @@ uint64_t __perf_reg_sp_s390(void) > { > return PERF_REG_S390_R15; > } > + > +/* %rXX */ > +#define SDT_OP_REGEX1 "^%r([0-9]|1[0-5])$" > +/* -###(%rXX) */ > +#define SDT_OP_REGEX2 "^(-?[0-9]+)\\(%r([0-9]|1[0-5])\\)$" > +static regex_t sdt_op_regex1, sdt_op_regex2; > + > +static int sdt_init_op_regex(void) > +{ > + static int initialized; > + int ret = 0; > + > + if (initialized) > + return 0; > + > + ret = regcomp(&sdt_op_regex1, SDT_OP_REGEX1, REG_EXTENDED); > + if (ret) > + goto error; > + initialized = 1; > + > + ret = regcomp(&sdt_op_regex2, SDT_OP_REGEX2, REG_EXTENDED); > + if (ret) > + goto free_regex1; > + initialized = 2; > + > + return 0; > + > +free_regex1: > + regfree(&sdt_op_regex1); > +error: > + pr_debug4("Regex compilation error, initialized %d\n", initialized); > + initialized = 0; > + return ret; > +} > + > +/* > + * Parse OP and convert it into uprobe format, which is, +/-NUM(%gprREG). > + * Possible variants of OP are: > + * Format Example > + * ------------------------- > + * NUM(%rREG) 48(%r1) > + * -NUM(%rREG) -48(%r1) > + * %rREG %r1 > + */ > +int __perf_sdt_arg_parse_op_s390(char *old_op, char **new_op) > +{ > + int ret, new_len; > + regmatch_t rm[6]; > + unsigned long i; > + > + *new_op = NULL; > + ret = sdt_init_op_regex(); > + if (ret < 0) > + return ret; Some AI feedback: POSIX regcomp() returns 0 on success and a positive error code on failure (like REG_ESPACE). Since sdt_init_op_regex() returns this positive code, will ret < 0 evaluate to false on compilation failure? If so, this would allow execution to proceed to regexec() using uninitialized or freed regex structs, which could crash the tool. > + > + if (!regexec(&sdt_op_regex1, old_op, 3, rm, 0)) { > + /* Extract %rX */ > + new_len = 2; /* % NULL */ > + new_len += (int)(rm[1].rm_eo - rm[1].rm_so); > + *new_op = zalloc(new_len); > + if (!*new_op) > + return -ENOMEM; > + > + scnprintf(*new_op, new_len, "%%%.*s", > + (int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so); Does this formatting correctly preserve the 'r' prefix for s390 registers? The regex SDT_OP_REGEX1 is defined as ^%r([0-9]|1[0-5])$, meaning rm[1] captures the numeric digits, not the 'r'. So an input like %r15 will be formatted as %15. However, the s390 kernel's regs_query_register_offset() strictly requires the register name to start with 'r', otherwise it returns -EINVAL. Will the kernel's parse_probe_arg() reject the uprobe definition without the 'r' prefix? > + } else if (!regexec(&sdt_op_regex2, old_op, ARRAY_SIZE(rm), rm, 0)) { > + /* Extract #(%rX) */ > + new_len = 4; /* (%)NULL */ > + for (i = 1; i < ARRAY_SIZE(rm) && rm[i].rm_so != -1; ++i) > + new_len += (int)(rm[i].rm_eo - rm[i].rm_so); > + *new_op = zalloc(new_len); > + if (!*new_op) > + return -ENOMEM; > + > + scnprintf(*new_op, new_len, "%.*s(%%%.*s)", > + (int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so, > + (int)(rm[2].rm_eo - rm[2].rm_so), old_op + rm[2].rm_so); Similar to the above, rm[2] isolates the digits without the 'r' prefix, creating an argument like 48(%15). Additionally, does this string translation handle positive memory displacements correctly? The kernel's parse_probe_arg() in kernel/trace/trace_probe.c parses memory dereferences by matching the case '+': or case '-': prefix switch cases. If an argument starts with a digit rather than a + or -, it falls through to the default case and is rejected with -EINVAL. Should positive memory offsets be translated to explicitly include the + prefix (e.g., +48(%r15)) so they are accepted by the uprobe parser? Thanks, Ian > + } else { > + pr_debug4("Skipping unsupported SDT argument: %s\n", old_op); > + return SDT_ARG_SKIP; > + } > + > + return SDT_ARG_VALID; > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index 5b8f34beb24e..f52b0e1f7fc7 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -23,6 +23,9 @@ int perf_sdt_arg_parse_op(uint16_t e_machine, char *old_op, char **new_op) > case EM_X86_64: > ret = __perf_sdt_arg_parse_op_x86(old_op, new_op); > break; > + case EM_S390: > + ret = __perf_sdt_arg_parse_op_s390(old_op, new_op); > + break; > default: > pr_debug("Unknown ELF machine %d, standard arguments parse will be skipped.\n", > e_machine); > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h > index 7c04700bf837..573f0d1dfe04 100644 > --- a/tools/perf/util/perf_regs.h > +++ b/tools/perf/util/perf_regs.h > @@ -62,6 +62,7 @@ uint64_t __perf_reg_mask_s390(bool intr); > const char *__perf_reg_name_s390(int id); > uint64_t __perf_reg_ip_s390(void); > uint64_t __perf_reg_sp_s390(void); > +int __perf_sdt_arg_parse_op_s390(char *old_op, char **new_op); > > int __perf_sdt_arg_parse_op_x86(char *old_op, char **new_op); > uint64_t __perf_reg_mask_x86(bool intr); > -- > 2.53.0 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] perf record: Add support for arch_sdt_arg_parse_op() on s390 2026-03-13 20:50 ` Ian Rogers @ 2026-03-17 8:40 ` Thomas Richter 0 siblings, 0 replies; 3+ messages in thread From: Thomas Richter @ 2026-03-17 8:40 UTC (permalink / raw) To: Ian Rogers Cc: linux-kernel, linux-s390, linux-perf-users, acme, namhyung, dapeng1.mi, agordeev, gor, sumanthk, hca, japo On 3/13/26 21:50, Ian Rogers wrote: > On Fri, Mar 13, 2026 at 6:33 AM Thomas Richter <tmricht@linux.ibm.com> wrote: >> >> commit e5e66adfe45a6 ("perf regs: Remove __weak attributive arch_sdt_arg_parse_op() function") >> removes arch_sdt_arg_parse_op() functions. s390 support is missing. >> The following warning is printed: >> >> Unknown ELF machine 22, standard arguments parse will be skipped. >> >> ELF machine 22 is the EM_S390 host. This happens with command >> # ./perf record -v -- stress-ng -t 1s --matrix 0 >> on a z/VM system when the event is not specified. >> >> Add s390 specific __perf_sdt_arg_parse_op_s390() function to support >> -architecture calls to arch_sdt_arg_parse_op() for s390. >> The warning disappears. >> >> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com> >> Cc: Dapeng Mi <dapeng1.mi@linux.intel.com> >> Tested-by: Jan Polensky <japo@linux.ibm.com> >> --- >> .../perf/util/perf-regs-arch/perf_regs_s390.c | 89 +++++++++++++++++++ >> tools/perf/util/perf_regs.c | 3 + >> tools/perf/util/perf_regs.h | 1 + >> 3 files changed, 93 insertions(+) >> >> diff --git a/tools/perf/util/perf-regs-arch/perf_regs_s390.c b/tools/perf/util/perf-regs-arch/perf_regs_s390.c >> index c61df24edf0f..c830aeae606e 100644 >> --- a/tools/perf/util/perf-regs-arch/perf_regs_s390.c >> +++ b/tools/perf/util/perf-regs-arch/perf_regs_s390.c >> @@ -1,7 +1,13 @@ >> // SPDX-License-Identifier: GPL-2.0 >> >> +#include <errno.h> >> +#include <regex.h> >> #include "../perf_regs.h" >> #include "../../arch/s390/include/perf_regs.h" >> +#include "debug.h" >> + >> +#include <linux/zalloc.h> >> +#include <linux/kernel.h> >> >> uint64_t __perf_reg_mask_s390(bool intr __maybe_unused) >> { >> @@ -95,3 +101,86 @@ uint64_t __perf_reg_sp_s390(void) >> { >> return PERF_REG_S390_R15; >> } >> + >> +/* %rXX */ >> +#define SDT_OP_REGEX1 "^%r([0-9]|1[0-5])$" >> +/* -###(%rXX) */ >> +#define SDT_OP_REGEX2 "^(-?[0-9]+)\\(%r([0-9]|1[0-5])\\)$" >> +static regex_t sdt_op_regex1, sdt_op_regex2; >> + >> +static int sdt_init_op_regex(void) >> +{ >> + static int initialized; >> + int ret = 0; >> + >> + if (initialized) >> + return 0; >> + >> + ret = regcomp(&sdt_op_regex1, SDT_OP_REGEX1, REG_EXTENDED); >> + if (ret) >> + goto error; >> + initialized = 1; >> + >> + ret = regcomp(&sdt_op_regex2, SDT_OP_REGEX2, REG_EXTENDED); >> + if (ret) >> + goto free_regex1; >> + initialized = 2; >> + >> + return 0; >> + >> +free_regex1: >> + regfree(&sdt_op_regex1); >> +error: >> + pr_debug4("Regex compilation error, initialized %d\n", initialized); >> + initialized = 0; >> + return ret; >> +} >> + >> +/* >> + * Parse OP and convert it into uprobe format, which is, +/-NUM(%gprREG). >> + * Possible variants of OP are: >> + * Format Example >> + * ------------------------- >> + * NUM(%rREG) 48(%r1) >> + * -NUM(%rREG) -48(%r1) >> + * %rREG %r1 >> + */ >> +int __perf_sdt_arg_parse_op_s390(char *old_op, char **new_op) >> +{ >> + int ret, new_len; >> + regmatch_t rm[6]; >> + unsigned long i; >> + >> + *new_op = NULL; >> + ret = sdt_init_op_regex(); >> + if (ret < 0) >> + return ret; > > Some AI feedback: > > POSIX regcomp() returns 0 on success and a positive error code on failure > (like REG_ESPACE). Since sdt_init_op_regex() returns this positive code, > will ret < 0 evaluate to false on compilation failure? > > If so, this would allow execution to proceed to regexec() using uninitialized > or freed regex structs, which could crash the tool. Thanks for the finding, you are correct. I simply copy and pasted most part of the code. So we should also fix util/perf-regs-arch/perf_regs_powerpc.c, line 86 util/perf-regs-arch/perf_regs_aarch64.c, line 65 Then then return code of functions __perf_sdt_arg_parse_op_s390() should be negative on error. Otherwise synthesize_sdt_probe_arg() +--> perf_sdt_arg_parse_op() +--> __perf_sdt_arg_parse_op_s390() and synthesize_sdt_probe_arg() does not handle positive value as error: ret = perf_sdt_arg_parse_op(EM_HOST, op, &new_op); if (ret < 0) goto error; >> + >> + if (!regexec(&sdt_op_regex1, old_op, 3, rm, 0)) { >> + /* Extract %rX */ >> + new_len = 2; /* % NULL */ >> + new_len += (int)(rm[1].rm_eo - rm[1].rm_so); >> + *new_op = zalloc(new_len); >> + if (!*new_op) >> + return -ENOMEM; >> + >> + scnprintf(*new_op, new_len, "%%%.*s", >> + (int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so); > > Does this formatting correctly preserve the 'r' prefix for s390 registers? > The regex SDT_OP_REGEX1 is defined as ^%r([0-9]|1[0-5])$, meaning rm[1] > captures the numeric digits, not the 'r'. So an input like %r15 will be > formatted as %15. > > However, the s390 kernel's regs_query_register_offset() strictly requires > the register name to start with 'r', otherwise it returns -EINVAL. Will > the kernel's parse_probe_arg() reject the uprobe definition without the > 'r' prefix? Ok will fix this > >> + } else if (!regexec(&sdt_op_regex2, old_op, ARRAY_SIZE(rm), rm, 0)) { >> + /* Extract #(%rX) */ >> + new_len = 4; /* (%)NULL */ >> + for (i = 1; i < ARRAY_SIZE(rm) && rm[i].rm_so != -1; ++i) >> + new_len += (int)(rm[i].rm_eo - rm[i].rm_so); >> + *new_op = zalloc(new_len); >> + if (!*new_op) >> + return -ENOMEM; >> + >> + scnprintf(*new_op, new_len, "%.*s(%%%.*s)", >> + (int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so, >> + (int)(rm[2].rm_eo - rm[2].rm_so), old_op + rm[2].rm_so); > > Similar to the above, rm[2] isolates the digits without the 'r' prefix, > creating an argument like 48(%15). > > Additionally, does this string translation handle positive memory > displacements correctly? > > The kernel's parse_probe_arg() in kernel/trace/trace_probe.c parses memory > dereferences by matching the case '+': or case '-': prefix switch cases. > If an argument starts with a digit rather than a + or -, it falls through > to the default case and is rejected with -EINVAL. > > Should positive memory offsets be translated to explicitly include the + > prefix (e.g., +48(%r15)) so they are accepted by the uprobe parser? > Ok will fix this and send v2 Thanks a lot -- Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany -- IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-03-17 8:40 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-13 13:23 [PATCH v2] perf record: Add support for arch_sdt_arg_parse_op() on s390 Thomas Richter 2026-03-13 20:50 ` Ian Rogers 2026-03-17 8:40 ` Thomas Richter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox