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

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

On 6/2/2026 1:38 PM, Mi, Dapeng wrote:
> 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.

Just correct above words, sig_data has been printed and doesn't need to
print again. Thanks.


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