Re: [Patch v7 21/24] perf/x86/intel: Enable PERF_PMU_CAP_SIMD_REGS capability

On 4/25/2026 10:01 AM, [email protected] wrote:
> Sashiko AI review found 1 potential issue(s):
> - [Medium] When sample_simd_regs_enabled is true, large PEBS incorrectly remains enabled if unsupported legacy standard registers (such as segment registers) are requested.
> --
>
> commit add2d25bc2536ced31489740914a43c7ed8e02eb
> Author: Kan Liang <[email protected]>
>
> perf/x86/intel: Enable PERF_PMU_CAP_SIMD_REGS capability
>
> This commit enables the PERF_PMU_CAP_SIMD_REGS capability when XSAVES
> support is available. It also temporarily disables large PEBS sampling
> for these new registers until arch-PEBS sampling support is added.
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -4424,11 +4424,33 @@ static unsigned long intel_pmu_large_pebs_flags(struct perf_event *event)
>>  		flags &= ~PERF_SAMPLE_TIME;
>>  	if (!event->attr.exclude_kernel)
>>  		flags &= ~PERF_SAMPLE_REGS_USER;
>> -	if (event->attr.sample_regs_user & ~PEBS_GP_REGS)
>> -		flags &= ~PERF_SAMPLE_REGS_USER;
>> -	if (event->attr.sample_regs_intr &
>> -	    ~(PEBS_GP_REGS | PERF_REG_EXTENDED_MASK))
>> -		flags &= ~PERF_SAMPLE_REGS_INTR;
>> +	if (event->attr.sample_simd_regs_enabled) {
>> +		u64 nolarge = PERF_X86_EGPRS_MASK | BIT_ULL(PERF_REG_X86_SSP);
>> +
>> +		/*
>> +		 * PEBS HW can only collect the XMM0-XMM15 for now.
>> +		 * Disable large PEBS for other vector registers, predicate
>> +		 * registers, eGPRs, and SSP.
>> +		 */
>> +		if (event->attr.sample_regs_user & nolarge ||
>> +		    fls64(event->attr.sample_simd_vec_reg_user) > PERF_X86_H16ZMM_BASE ||
>> +		    event->attr.sample_simd_pred_reg_user)
>> +			flags &= ~PERF_SAMPLE_REGS_USER;
>> +
>> +		if (event->attr.sample_regs_intr & nolarge ||
>> +		    fls64(event->attr.sample_simd_vec_reg_intr) > PERF_X86_H16ZMM_BASE ||
>> +		    event->attr.sample_simd_pred_reg_intr)
>> +			flags &= ~PERF_SAMPLE_REGS_INTR;
> When sample_simd_regs_enabled is true, nolarge is used to disable large PEBS
> if eGPRs or SSP are requested. However, it looks like the pre-existing
> ~PEBS_GP_REGS check was dropped in this branch.
>
> If unsupported legacy registers like segment registers (e.g. PERF_REG_X86_CS)
> are requested alongside SIMD sampling, will this bypass the check and
> incorrectly leave PERF_SAMPLE_REGS_USER or PERF_SAMPLE_REGS_INTR enabled?
>
> Since PEBS hardware batches samples and does not natively capture segment
> registers, wouldn't the delayed PMI overflow populate these missing segment
> registers using the interrupt context?
>
> Could this result in stale or delayed segment register values being provided
> for the entire batch instead of precise per-sample values?

That's right. We needs to check the bitmask of all supported registers
including eGPRs when sample_simd_regs_enabled is set. 

Besides, I would swap this patch with the next patch, then we don't need to
introduce the code and then delete them.

Thanks.


>