Re: [Patch v10 04/10] perf headers: Sync perf_event.h/perf_regs.h with the kernel headers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] evsel__parse_sample() fails to parse or skip the newly appended SIMD payload, corrupting subsequent sample field parsing.
- [Medium] The condition to print the new SIMD attribute fields in `perf_event_attr__fprintf()` uses the offset of the last field instead of the first field, hiding valid intermediate fields.
- [Medium] regs_dump_abi() incorrectly reports 'unknown' for all samples with SIMD registers enabled.
--

--- Patch [4]: [Patch v10 04/10] perf headers: Sync perf_event.h/perf_regs.h with the kernel headers ---
commit 8b5325e2c0e1de39cce1b1704639159d2e039a47
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf headers: Sync perf_event.h/perf_regs.h with the kernel headers

This commit syncs UAPI header changes supporting SIMD/eGPRs/SSP sampling
into the corresponding tools UAPI headers. It also updates the print
and swap helpers to support the newly introduced perf_event_attr fields.

> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index fd10aa8d697f2..c49fc76292f79 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
[ ... ]
> @@ -314,8 +314,9 @@ enum {
>   */
>  enum perf_sample_regs_abi {
>  	PERF_SAMPLE_REGS_ABI_NONE		= 0,
> -	PERF_SAMPLE_REGS_ABI_32			= 1,
> -	PERF_SAMPLE_REGS_ABI_64			= 2,
> +	PERF_SAMPLE_REGS_ABI_32			= (1 << 0),
> +	PERF_SAMPLE_REGS_ABI_64			= (1 << 1),
> +	PERF_SAMPLE_REGS_ABI_SIMD		= (1 << 2),
>  };

[Severity: Medium]
By changing the ABI values to bitwise flags, will this break the existing
regs_dump_abi() in tools/perf/util/session.c?

When the PERF_SAMPLE_REGS_ABI_SIMD bit is set, the combined mask is at
least 4. This triggers the fallback in regs_dump_abi():

static inline const char *regs_dump_abi(struct regs_dump *d)
{
	if (d->abi > PERF_SAMPLE_REGS_ABI_64)
		return "unknown";

	return regs_abi[d->abi];
}

Should this logic be updated to handle the new bitmasks so valid SIMD
samples aren't displayed as unknown?

[ ... ]
> @@ -1020,7 +1045,15 @@ enum perf_event_type {
>  	 *      } && PERF_SAMPLE_BRANCH_STACK
>  	 *
>  	 *	{ u64			abi; # enum perf_sample_regs_abi
> -	 *	  u64			regs[weight(mask)]; } && PERF_SAMPLE_REGS_USER
> +	 *	  u64			regs[weight(mask)];
> +	 *	  struct {
> +	 *		u64 nr_vectors;		# 0 ... weight(sample_simd_vec_reg_user)
> +	 *		u64 vector_qwords;	# 0 ... sample_simd_vec_reg_qwords
> +	 *		u64 nr_pred;		# 0 ... weight(sample_simd_pred_reg_user)
> +	 *		u64 pred_qwords;	# 0 ... sample_simd_pred_reg_qwords
> +	 *		u64 data[nr_vectors * vector_qwords + nr_pred * pred_qwords];
> +	 *	  } && (abi & PERF_SAMPLE_REGS_ABI_SIMD)
> +	 *	} && PERF_SAMPLE_REGS_USER

[Severity: High]
Does evsel__parse_sample() need to be updated to parse or skip this
newly appended SIMD payload?

Looking at tools/perf/util/evsel.c:__evsel__parse_sample(), it currently
only advances the array pointer by the basic register mask weight:

		if (regs->abi) {
			u64 mask = evsel->core.attr.sample_regs_user;

			sz = hweight64(mask) * sizeof(u64);
			OVERFLOW_CHECK(array, sz, max_size);
			regs->mask = mask;
			regs->regs = (u64 *)array;
			array = (void *)array + sz;
		}

If the array pointer isn't advanced past the new SIMD data, will the
parser misinterpret the SIMD payload as subsequent sample fields and
result in parsing errors?

[ ... ]
> diff --git a/tools/perf/util/perf_event_attr_fprintf.c b/tools/perf/util/perf_event_attr_fprintf.c
> index cc817294ad3d6..2e8be9a357dce 100644
> --- a/tools/perf/util/perf_event_attr_fprintf.c
> +++ b/tools/perf/util/perf_event_attr_fprintf.c
> @@ -407,8 +407,17 @@ int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
>  	}
>  	if (attr_size >= offsetof(struct perf_event_attr, config4))
>  		PRINT_ATTRf(config3, p_hex);
> -	if (attr_size > offsetof(struct perf_event_attr, config4))
> +	if (attr_size >= offsetof(struct perf_event_attr, sample_simd_regs_enabled))
>  		PRINT_ATTRf(config4, p_hex);
> +	if (attr_size > offsetof(struct perf_event_attr, sample_simd_vec_reg_user)) {
> +		PRINT_ATTRf(sample_simd_regs_enabled, p_unsigned);
> +		PRINT_ATTRf(sample_simd_pred_reg_qwords, p_unsigned);
> +		PRINT_ATTRf(sample_simd_pred_reg_intr, p_hex);

[Severity: Medium]
Is this conditional too restrictive for the intermediate SIMD fields?

If the tools encounter an attr_size that encompasses some of the new SIMD
fields (like sample_simd_regs_enabled) but is strictly less than the offset
of sample_simd_vec_reg_user, it evaluates to false. Will this inadvertently
skip printing those intermediate fields even though they are present in the
struct?

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