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

[Thomas Richter <tmricht@linux.ibm.com>]

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