Re: [RFC PATCH 00/12] Support vector and more extended registers in perf

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2025-06-19 9:38 a.m., Peter Zijlstra wrote:
> On Thu, Jun 19, 2025 at 07:11:23AM -0400, Liang, Kan wrote:
> 
>> @@ -543,6 +544,24 @@ struct perf_event_attr {
>>  	__u64	sig_data;
>>
>>  	__u64	config3; /* extension of config2 */
>> +
>> +
>> +	/*
>> +	 * Defines set of SIMD registers to dump on samples.
>> +	 * The sample_simd_req_enabled !=0 implies the
>> +	 * set of SIMD registers is used to config all SIMD registers.
>> +	 * If !sample_simd_req_enabled, sample_regs_XXX may be used to
>> +	 * config some SIMD registers on X86.
>> +	 */
>> +	union {
>> +		__u16 sample_simd_reg_enabled;
>> +		__u16 sample_simd_pred_reg_qwords;
>> +	};
>> +	__u16 sample_simd_pred_reg_intr;
>> +	__u16 sample_simd_pred_reg_user;
> 
> This limits things to max 16 predicate registers. ARM will fully fill
> that with present hardware.

I think I can use __u32 for predicate registers.
It means we need one more u64 for the qwords. It should not be a problem.

> 
>> +	__u16 sample_simd_reg_qwords;
>> +	__u64 sample_simd_reg_intr;
>> +	__u64 sample_simd_reg_user;
> 
> I would perhaps make this vec_reg.

Sure.

> 
>>  };
>>
>>  /*
>> @@ -1016,7 +1035,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 {
>> +	 *	  	u16 nr_vectors;
>> +	 *	  	u16 vector_qwords;
>> +	 *	  	u16 nr_pred;
>> +	 *	  	u16 pred_qwords;
>> +	 *	  	u64 data[nr_vectors * vector_qwords + nr_pred * pred_qwords];
>> +	 *	  } && sample_simd_reg_enabled
> 
> Instead of using sample_simd_reg_enabled here I would perhaps extend
> perf_sample_regs_abi. The current abi word is woefully underused.
> 

Yes. Now I think the abi is used like a version number. I guess I can
add PERF_SAMPLE_REGS_ABI_SIMD and change it to a bitmap.
There should be no impact on the existing tool, since version and bitmap
are the same for 1 and 2.
 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_NONE               = 0x0,
+       PERF_SAMPLE_REGS_ABI_32                 = 0x1,
+       PERF_SAMPLE_REGS_ABI_64                 = 0x2,
+       PERF_SAMPLE_REGS_ABI_SIMD               = 0x4,
 };

> Also, realistically, what you want to look at here is:
> 
>   sample_simd_{pred,vec}_reg_user;
> 
> If those are empty, there will be no registers.

Sure. But I will still keep the sample_simd_reg_enabled, since it can
explicitly tell if the new format is used.

Thanks,
Kan

> 
>> +	 *	} && PERF_SAMPLE_REGS_USER
>>  	 *
>>  	 *	{ u64			size;
>>  	 *	  char			data[size];
>