Re: [PATCH v3] perf record: Add support for arch_sdt_arg_parse_op() on s390

From: Ian Rogers

Date: Tue Mar 17 2026 - 11:35:53 EST

On Tue, Mar 17, 2026 at 4:06 AM Thomas Richter <tmricht@xxxxxxxxxxxxx> wrote:
>
> commit e5e66adfe45a6 ("perf regs: Remove __weak attributive arch_sdt_arg_parse_op() function")
> removes arch_sdt_arg_parse_op() functions and s390 support is lost.
> The following warning is printed:

Apologies again for this breakage.

> 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@xxxxxxxxxxxxx>
> Cc: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> Cc: Ian Rogers <irogers@xxxxxxxxxx>
> Tested-by: Jan Polensky <japo@xxxxxxxxxxxxx>
> ---
> .../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..2aa70eb23311 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]|r1[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;

Sashiko's review:
https://sashiko.dev/#/patchset/20260317110641.39975-1-tmricht%40linux.ibm.com
notes that here 'initialized' will still be 1, should it be made 0 again?

It triggers an ENOMEM, so I doubt this will be very useful, but it may
make the code read better.

> + initialized = 2;
> +
> + return 0;
> +
> +free_regex1:
> + regfree(&sdt_op_regex1);
> +error:
> + pr_debug4("Regex compilation error, initialized %d\n", initialized);
> + 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)
> + return -EINVAL;
> +
> + 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);

Sashiko notes this is probably more of an issue:

The allocation size accounts for a % character, but the scnprintf format
string "%.*s" seems to omit it.

Will this output r1 instead of %r1?

> + } 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);

Similarly, does this scnprintf format string "%.*s(%.*s)" need a %% character
to produce the correct uprobe format? As written, it appears it will output
48(r1) instead of 48(%r1).

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
>