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

[sashiko-bot@kernel.org]

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.

> 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?

> 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?

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