Re: [PATCH V3 05/17] perf/x86: Support XMM register for non-PEBS and REGS_USER

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

On 2025-08-20 2:46 a.m., Mi, Dapeng wrote:
> 
> On 8/19/2025 11:55 PM, Liang, Kan wrote:
>>
>> On 2025-08-19 6:39 a.m., Peter Zijlstra wrote:
>>> On Fri, Aug 15, 2025 at 02:34:23PM -0700, kan.liang@linux.intel.com wrote:
>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>>
>>>> Collecting the XMM registers in a PEBS record has been supported since
>>>> the Icelake. But non-PEBS events don't support the feature. It's
>>>> possible to retrieve the XMM registers from the XSAVE for non-PEBS.
>>>> Add it to make the feature complete.
>>>>
>>>> To utilize the XSAVE, a 64-byte aligned buffer is required. Add a
>>>> per-CPU ext_regs_buf to store the vector registers. The size of the
>>>> buffer is ~2K. kzalloc_node() is used because there's a _guarantee_
>>>> that all kmalloc()'s with powers of 2 are naturally aligned and also
>>>> 64b aligned.
>>>>
>>>> Extend the support for both REGS_USER and REGS_INTR. For REGS_USER, the
>>>> perf_get_regs_user() returns the regs from the task_pt_regs(current),
>>>> which is struct pt_regs. Need to move it to local struct x86_perf_regs
>>>> x86_user_regs.
>>>> For PEBS, the HW support is still preferred. The XMM should be retrieved
>>>> from PEBS records.
>>>>
>>>> There could be more vector registers supported later. Add ext_regs_mask
>>>> to track the supported vector register group.
>>>
>>> I'm a little confused... *again* :-)
>>>
>>> Specifically, we should consider two sets of registers:
>>>
>>>  - the live set, as per the CPU (XSAVE)
>>>  - the stored set, as per x86_task_fpu()
>>>
>>> regs_intr should always get a copy of the live set; however
>>> regs_user should not. It might need a copy of the x86_task_fpu() instead
>>> of the live set, depending on TIF_NEED_FPU_LOAD (more or less, we need
>>> another variable set in kernel_fpu_begin_mask() *after*
>>> save_fpregs_to_fpstate() is completed).
>>>
>>> I don't see this code make this distinction.
>>>
>>> Consider getting a sample while the kernel is doing some avx enhanced
>>> crypto and such.
>> The regs_user only needs a set when the NMI hits the user mode
>> (user_mode(regs)) or a non-kernel thread (!(current->flags &
>> PF_KTHREAD)). The live set is good enough for both cases.
> 
> It's fine if NMI hits user mode, but if NMI hits the kernel mode
> (!(current->flags &PF_KTHREAD)), won't the kernel space SIMD/eGPR regs be
> exposed to user space for user-regs option? I'm not sure if kernel space
> really use these SIMD/eGPR regs right now, but it seems a risk.
> 
>

I don't think it's possible for the existing kernel. But I cannot
guarantee future usage.

If the kernel mode handling is still a concern, I think we should drop
the SIMD/eGPR regs for the case for now. Because
- To profile a userspace application which requires SIMD/eGPR regs, the
NMI usually hits the usersapce. It's not common to hit the kernel mode.
- The SIMD/eGPR cannot be retrieved from the task_pt_regs(). Although
it's possible to retrieve the values when the TIF_NEED_FPU_LOAD flag is
set, I don't think it's worth introducing such complexity to handle an
uncommon case in the critical path.
- Furthermore, only checking the TIF_NEED_FPU_LOAD flag cannot cure
everything. Some corner cases cannot be handled either. For example, an
NMI can happen when the flag just switched, but still in the kernel mode.

We can always add the support later if someone thinks it's important to
retrieve the user SIMD/eGPR regs during the kernel syscall.

Thanks,
Kan
>>
>> I think the kernel crypto should be to a kernel thread (current->flags &
>> PF_KTHREAD). If so, the regs_user should return NULL.
>>
>> Thanks,
>> Kan
>>
>