Messages in this thread |  | | Date | Fri, 5 Dec 2025 14:37:24 +0800 | | Subject | Re: [Patch v5 06/19] perf/x86: Add support for XMM registers in non-PEBS and REGS_USER | | From | "Mi, Dapeng" <> |
| |
On 12/4/2025 11:47 PM, Peter Zijlstra wrote: > On Thu, Dec 04, 2025 at 04:17:35PM +0100, Peter Zijlstra wrote: >> On Wed, Dec 03, 2025 at 02:54:47PM +0800, Dapeng Mi wrote: >>> From: Kan Liang <kan.liang@linux.intel.com> >>> >>> While collecting XMM registers in a PEBS record has been supported since >>> Icelake, non-PEBS events have lacked this feature. By leveraging the >>> xsaves instruction, it is now possible to snapshot XMM registers for >>> non-PEBS events, completing the feature set. >>> >>> To utilize the xsaves instruction, a 64-byte aligned buffer is required. >>> A per-CPU ext_regs_buf is added to store SIMD and other registers, with >>> the buffer size being approximately 2K. The buffer is allocated using >>> kzalloc_node(), ensuring natural alignment and 64-byte alignment for all >>> kmalloc() allocations with powers of 2. >>> >>> The XMM sampling support is extended for both REGS_USER and REGS_INTR. >>> For REGS_USER, perf_get_regs_user() returns the registers from >>> task_pt_regs(current), which is a pt_regs structure. It needs to be >>> copied to user space secific x86_user_regs structure since kernel may >>> modify pt_regs structure later. >>> >>> For PEBS, XMM registers are retrieved from PEBS records. >>> >>> In cases where userspace tasks are trapped within kernel mode (e.g., >>> during a syscall) when an NMI arrives, pt_regs information can still be >>> retrieved from task_pt_regs(). However, capturing SIMD and other >>> xsave-based registers in this scenario is challenging. Therefore, >>> snapshots for these registers are omitted in such cases. >>> >>> The reasons are: >>> - Profiling a userspace task that requires SIMD/eGPR registers typically >>> involves NMIs hitting userspace, not kernel mode. >>> - Although it is possible to retrieve values when the TIF_NEED_FPU_LOAD >>> flag is set, the complexity introduced to handle this uncommon case in >>> the critical path is not justified. >>> - Additionally, checking the TIF_NEED_FPU_LOAD flag alone is insufficient. >>> Some corner cases, such as an NMI occurring just after the flag switches >>> but still in kernel mode, cannot be handled. >> Urgh.. Dave, Thomas, is there any reason we could not set >> TIF_NEED_FPU_LOAD *after* doing the XSAVE (clearing is already done >> after restore). >> >> That way, when an NMI sees TIF_NEED_FPU_LOAD it knows the task copy is >> consistent. >> >> I'm not at all sure this is complex, it just needs a little care. >> >> And then there is the deferred thing, just like unwind, we can defer >> REGS_USER/STACK_USER much the same, except someone went and built all >> that deferred stuff with unwind all tangled into it :/ > With something like the below, the NMI could do something like: > > struct xregs_state *xr = NULL; > > /* > * fpu code does: > * XSAVE > * set_thread_flag(TIF_NEED_FPU_LOAD) > * ... > * XRSTOR > * clear_thread_flag(TIF_NEED_FPU_LOAD) > * therefore, when TIF_NEED_FPU_LOAD, the task fpu state holds a > * whole copy. > */ > if (test_thread_flag(TIF_NEED_FPU_LOAD)) { > struct fpu *fpu = x86_task_fpu(current); > /* > * If __task_fpstate is set, it holds the right pointer, > * otherwise fpstate will. > */ > struct fpstate *fps = READ_ONCE(fpu->__task_fpstate); > if (!fps) > fps = fpu->fpstate; > xr = &fps->regs.xregs_state; > } else { > /* like fpu_sync_fpstate(), except NMI local */ > xsave_nmi(xr, mask); > } > > // frob xr into perf data > > Or did I miss something? I've not looked at this very long and the above > was very vague on the actual issues. > > > diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c > index da233f20ae6f..0f91a0d7e799 100644 > --- a/arch/x86/kernel/fpu/core.c > +++ b/arch/x86/kernel/fpu/core.c > @@ -359,18 +359,22 @@ int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest) > struct fpstate *cur_fps = fpu->fpstate; > > fpregs_lock(); > - if (!cur_fps->is_confidential && !test_thread_flag(TIF_NEED_FPU_LOAD)) > + if (!cur_fps->is_confidential && !test_thread_flag(TIF_NEED_FPU_LOAD)) { > save_fpregs_to_fpstate(fpu); > + set_thread_flag(TIF_NEED_FPU_LOAD); > + } > > /* Swap fpstate */ > if (enter_guest) { > - fpu->__task_fpstate = cur_fps; > + WRITE_ONCE(fpu->__task_fpstate, cur_fps); > + barrier(); > fpu->fpstate = guest_fps; > guest_fps->in_use = true; > } else { > guest_fps->in_use = false; > fpu->fpstate = fpu->__task_fpstate; > - fpu->__task_fpstate = NULL; > + barrier(); > + WRITE_ONCE(fpu->__task_fpstate, NULL); > } > > cur_fps = fpu->fpstate; > @@ -456,8 +460,8 @@ void kernel_fpu_begin_mask(unsigned int kfpu_mask) > > if (!(current->flags & (PF_KTHREAD | PF_USER_WORKER)) && > !test_thread_flag(TIF_NEED_FPU_LOAD)) { > - set_thread_flag(TIF_NEED_FPU_LOAD); > save_fpregs_to_fpstate(x86_task_fpu(current)); > + set_thread_flag(TIF_NEED_FPU_LOAD); > } > __cpu_invalidate_fpregs_state(); >
Ok, I would involve these changes into next version and support the SIMD registers sampling for user space registers sampling.
|  |