Re: [Patch v8 3/5] perf regs: Support x86 SIMD registers sampling

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 5/30/2026 1:41 AM, Ian Rogers wrote:
> On Fri, May 29, 2026 at 3:51 AM <sashiko-bot@kernel.org> wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> - [Medium] Parsing of `--intr-regs` and `--user-regs` with multiple options is order-dependent, preventing intended combination of General Purpose (GP) and SIMD registers.
>> - [Medium] Specifying only SIMD registers (e.g., `--intr-regs=xmm` without GP registers) leaves the general-purpose register mask as 0, causing the kernel to reject the event configuration with `-EINVAL`.
>> - [Low] The `sample_simd_regs_enabled` field is omitted from `perf_event_attr__fprintf()`.
>> --
>>
>> commit 03a81994cf381ee1d67a41deb452833fc73a023e
>> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>
>> perf regs: Support x86 SIMD registers sampling
>>
>> This patch adds support for the newly introduced SIMD register sampling
>> format by adding functions to support parsing and sampling x86 SIMD/PRED
>> registers.
>>
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index 1c856a2ecc6ec..cd62af14a4f54 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>> [ ... ]
>>
>>> @@ -1594,6 +1595,16 @@ void evsel__config(struct evsel *evsel, const struct record_opts *opts,
>>>               evsel__set_sample_bit(evsel, REGS_INTR);
>>>       }
>>>
>>> +     if ((opts->sample_intr_vec_regs || opts->sample_intr_pred_regs) &&
>>> +         !evsel->no_aux_samples && !evsel__is_dummy_event(evsel)) {
>>> +             attr->sample_simd_regs_enabled = !!opts->sample_simd_regs_enabled;
>>> +             attr->sample_simd_vec_reg_intr = opts->sample_intr_vec_regs;
>>> +             attr->sample_simd_vec_reg_qwords = opts->sample_vec_reg_qwords;
>>> +             attr->sample_simd_pred_reg_intr = opts->sample_intr_pred_regs;
>>> +             attr->sample_simd_pred_reg_qwords = opts->sample_pred_reg_qwords;
>>> +             evsel__set_sample_bit(evsel, REGS_INTR);
>>> +     }
>> [Severity: Medium]
>> Will specifying only SIMD registers (e.g., perf record --intr-regs=xmm)
>> leave the general-purpose register mask as 0?
>>
>> If opts->sample_intr_regs is 0, attr->sample_regs_intr remains 0. However,
>> setting the REGS_INTR bit here causes the kernel validation function
>> (perf_reg_validate()) to unconditionally return -EINVAL because the sample
>> type bit is set but the corresponding GP register mask is 0.
> It is an interesting corner case, we could extend this test to cover it:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/record.sh?h=perf-tools-next#n106

No, this is not a real issue. The perf_reg_validate() has been leased to
allow 0 mask bit if simd_enabled is true. So no -EINVAL would be returned,
like below,

$ sudo ./perf record -e cycles:p -Ixmm -c 10000 sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.111 MB perf.data (262 samples) ]


>
>>> diff --git a/tools/perf/util/parse-regs-options.c b/tools/perf/util/parse-regs-options.c
>>> index 70a1cc90b2c13..26d560a486c85 100644
>>> --- a/tools/perf/util/parse-regs-options.c
>>> +++ b/tools/perf/util/parse-regs-options.c
>> [ ... ]
>>
>>> @@ -53,22 +106,96 @@ name_to_perf_reg_mask(const char *to_match, uint64_t mask, int abi)
>> [ ... ]
>>
>>>       /*
>>> -      * cannot set it twice
>>> +      * Non-SIMD registers cannot be set twice.
>>> +      * SIMD registers can be set multiple times, but only the register
>>> +      * class with largest length (qwords) is sampled.
>>>        */
>>>       if (*mode)
>>>               return -1;
>> [Severity: Medium]
>> Does the order of options affect whether they are parsed successfully?
>>
>> If SIMD registers are parsed first (e.g., -I xmm), *mode is left as 0,
>> which allows a subsequent GP register option like -I ax to be parsed
>> successfully.
>>
>> However, if GP registers are parsed first (e.g., -I ax), *mode is set to
>> a non-zero value, which triggers this failure check when -I xmm is
>> evaluated. Does this prevent intended combinations of GP and SIMD registers
>> from working consistently?
> It's probably worth adding coverage for this too. In this case having
> a C unit test may make most sense.

Ok, It's indeed a corner issue. The simplest way may be remove this check.
Would fix it in next version.

Sure. Would extend current perf record test to cover the SIMD registers
sampling.


>
>>> diff --git a/tools/perf/util/perf_event_attr_fprintf.c b/tools/perf/util/perf_event_attr_fprintf.c
>>> index 741c3d657a8b6..c6b8e53e06fd1 100644
>>> --- a/tools/perf/util/perf_event_attr_fprintf.c
>>> +++ b/tools/perf/util/perf_event_attr_fprintf.c
>>> @@ -362,6 +362,12 @@ int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
>>>       PRINT_ATTRf(aux_start_paused, p_unsigned);
>>>       PRINT_ATTRf(aux_pause, p_unsigned);
>>>       PRINT_ATTRf(aux_resume, p_unsigned);
>>> +     PRINT_ATTRf(sample_simd_pred_reg_qwords, p_unsigned);
>>> +     PRINT_ATTRf(sample_simd_pred_reg_intr, p_hex);
>>> +     PRINT_ATTRf(sample_simd_pred_reg_user, p_hex);
>>> +     PRINT_ATTRf(sample_simd_vec_reg_qwords, p_unsigned);
>>> +     PRINT_ATTRf(sample_simd_vec_reg_intr, p_hex);
>>> +     PRINT_ATTRf(sample_simd_vec_reg_user, p_hex);
>>>
>>>       return ret;
>>>  }
>> [Severity: Low]
>> Does this also need to print the sample_simd_regs_enabled field?
>>
>> This patch adds several new SIMD-related fields to struct perf_event_attr,
>> but sample_simd_regs_enabled is missing here. Could this prevent developers
>> from verifying whether the tool correctly enabled SIMD sampling when inspecting
>> configurations via debug commands like perf record -vvv?
> It looks like this uncovers some technical debt. I don't see config3
> or config4 being printed either :-( Could you help to address, if not
> remind me :-)

It looks like sig_data, config3 and config4 are missed to print here. I
would add a patch to print them before printing these new SIMD fields.

Thanks.


>
> Thanks,
> Ian
>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260529082451.591783-1-dapeng1.mi@linux.intel.com?part=3
>>