Message-ID: <bc8a902f-549a-482f-bf24-04cf5f38a379@intel.com>
Date: Thu, 25 Sep 2025 08:07:08 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Dapeng Mi <dapeng1.mi@...ux.intel.com>,
 Peter Zijlstra <peterz@...radead.org>, 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>,
 Kan Liang <kan.liang@...ux.intel.com>, Andi Kleen <ak@...ux.intel.com>,
 Eranian Stephane <eranian@...gle.com>
Cc: Mark Rutland <mark.rutland@....com>, broonie@...nel.org,
 Ravi Bangoria <ravi.bangoria@....com>, linux-kernel@...r.kernel.org,
 linux-perf-users@...r.kernel.org, Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [Patch v4 03/17] x86/fpu/xstate: Add xsaves_nmi

On 9/24/25 23:11, Dapeng Mi wrote:
> From: Kan Liang <kan.liang@...ux.intel.com>
> 
> There is a hardware feature (Intel PEBS XMMs group), which can handle
> XSAVE "snapshots" from random code running. This just provides another
> XSAVE data source at a random time.
> 
> Add an interface to retrieve the actual register contents when the NMI
> hit. The interface is different from the other interfaces of FPU. The
> other mechanisms that deal with xstate try to get something coherent.
> But this interface is *in*coherent. There's no telling what was in the
> registers when a NMI hits. It writes whatever was in the registers when
> the NMI hit. It's the invoker's responsibility to make sure the contents
> are properly filtered before exposing them to the end user.
> 
> The support of the supervisor state components is required. The
> compacted storage format is preferred. So the XSAVES is used.

The changelog here is looking a bit munged from the last time I looked
at it. It's getting a bit hard to read. I'd probably run it through your
favorite LLM (and proofread it after of course) to make it more readable.

Ditto for the comments.

Also, what supervisor components are involved here? Aren't we just
talking about [XYZ]MM's?

> +/**
> + * xsaves_nmi - Save selected components to a kernel xstate buffer in NMI
> + * @xstate:	Pointer to the buffer
> + * @mask:	Feature mask to select the components to save
> + *
> + * The @xstate buffer must be 64 byte aligned.
> + *
> + * Caution: The interface is different from the other interfaces of FPU.
> + * The other mechanisms that deal with xstate try to get something coherent.
> + * But this interface is *in*coherent. There's no telling what was in the
> + * registers when a NMI hits. It writes whatever was in the registers when
> + * the NMI hit.
> + * The only user for the interface is perf_event. There is already a
> + * hardware feature (See Intel PEBS XMMs group), which can handle XSAVE
> + * "snapshots" from random code running. This just provides another XSAVE
> + * data source at a random time.
> + * This function can only be invoked in an NMI. It returns the *ACTUAL*
> + * register contents when the NMI hit.
> + */

First, please use actual paragraphs. This isn't a manpage.

But this whole comment kinda rubs me the wrong way.

For instance, I don't think we need to relitigate the XSAVE architecture
with the "The @xstate buffer must be 64 byte aligned." comment. Even if
we did, that's just silly when you could put a one-liner WARN_ON() in
the function which would be a billion times better than a comment.

I'm not sure what "interfaces of FPU" means. I know it came mostly out
of some earlier mails I wrote. But could we trim this down, please?

We basically want to scare anyone else away that might be tempted to use
this.

Powered by blists - more mailing lists