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

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

On 7/21/2026 3:47 PM, sashiko-bot@kernel.org wrote:
> 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?

regs_dump_abi() is extended to support these new ABI flags in the patch
05/10 "perf regs: Support x86 eGPRs/SSP sampling".


> [ ... ]
>> @@ -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?

Similarly, __evsel__parse_sample() is extended to support these new SIMD
fields in subsequent patch "perf regs: Enable dumping of SIMD registers". 


> [ ... ]
>> 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?

That would never happen since all these SIMD fields are always introduced
together. Thanks.