Message-ID: <20251204154721.GB2619703@noisy.programming.kicks-ass.net>
Date: Thu, 4 Dec 2025 16:47:21 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Dapeng Mi <dapeng1.mi@...ux.intel.com>
Cc: Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Eranian Stephane <eranian@...gle.com>,
	Mark Rutland <mark.rutland@....com>, broonie@...nel.org,
	Ravi Bangoria <ravi.bangoria@....com>, linux-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org, Zide Chen <zide.chen@...el.com>,
	Falcon Thomas <thomas.falcon@...el.com>,
	Dapeng Mi <dapeng1.mi@...el.com>, Xudong Hao <xudong.hao@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [Patch v5 06/19] perf/x86: Add support for XMM registers in
 non-PEBS and REGS_USER

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@...ux.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();
 

Powered by blists - more mailing lists