Re: [Patch v6 10/22] perf/x86: Enable XMM Register Sampling for Non-PEBS Events

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

On 2/16/2026 7:58 AM, Chang S. Bae wrote:
> On 2/8/2026 11:20 PM, Dapeng Mi wrote:
>> This patch supports XMM sampling for non-PEBS events in the `REGS_INTR`
> Please avoid "This patch".

Sure.


>
> ...
>
>> +static inline void __x86_pmu_sample_ext_regs(u64 mask)
>> +{
>> +	struct xregs_state *xsave = per_cpu(ext_regs_buf, smp_processor_id());
>> +
>> +	if (WARN_ON_ONCE(!xsave))
>> +		return;
>> +
>> +	xsaves_nmi(xsave, mask);
>> +}
>> +
>> +static inline void x86_pmu_update_ext_regs(struct x86_perf_regs *perf_regs,
>> +					   struct xregs_state *xsave, u64 bitmap)
>> +{
>> +	u64 mask;
>> +
>> +	if (!xsave)
>> +		return;
>> +
>> +	/* Filtered by what XSAVE really gives */
>> +	mask = bitmap & xsave->header.xfeatures;
>> +
>> +	if (mask & XFEATURE_MASK_SSE)
>> +		perf_regs->xmm_space = xsave->i387.xmm_space;
>> +}
>> +
>> +static void x86_pmu_sample_extended_regs(struct perf_event *event,
>> +					 struct perf_sample_data *data,
>> +					 struct pt_regs *regs,
>> +					 u64 ignore_mask)
>> +{
> ...
>
>> +
>> +	if (intr_mask) {
>> +		__x86_pmu_sample_ext_regs(intr_mask);
>> +		xsave = per_cpu(ext_regs_buf, smp_processor_id());
>> +		x86_pmu_update_ext_regs(perf_regs, xsave, intr_mask);
> These three lines appear to just update xcomponent's _pointers_ to an 
> xsave storage:
>
>    * Retrieve a per-cpu XSAVE buffer if valid
>    * Ensure the component's presence against XSTATE_BV
>    * Then, update pointers in pref_regs.
>
> which could be done in a function with more descriptive name. Given 
> that, I don't think__x86_pmu_sample_ext_regs() has any point. I don't 
> understand x86_pmu_sample_extended_regs() naming, either.

Hmm, partially agree, __x86_pmu_sample_ext_regs() can be dropped. The
intent of this whole function is to sample the required SIMD and APX eGPRs
registers by leveraging xsaves instruction in a NMI handler context. All
the SIMD and eGPRs registers are collectively called "extended registers"
here. Not sure if the "extended registers" is a good word, please suggest
if there is a better one.

Thanks.