[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3e95a8f7-88aa-4732-b38b-ccef74634819@intel.com>
Date: Tue, 1 Jul 2025 17:18:01 -0700
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: <kan.liang@...ux.intel.com>
CC: <peterz@...radead.org>, <mingo@...hat.com>, <acme@...nel.org>,
<namhyung@...nel.org>, <tglx@...utronix.de>, <dave.hansen@...ux.intel.com>,
<irogers@...gle.com>, <adrian.hunter@...el.com>, <jolsa@...nel.org>,
<alexander.shishkin@...ux.intel.com>, <linux-kernel@...r.kernel.org>,
<dapeng1.mi@...ux.intel.com>, <ak@...ux.intel.com>, <zide.chen@...el.com>,
<mark.rutland@....com>, <broonie@...nel.org>, <ravi.bangoria@....com>
Subject: Re: [RFC PATCH V2 03/13] x86/fpu/xstate: Add xsaves_nmi
On 6/26/2025 12:56 PM, kan.liang@...ux.intel.com wrote:
<snip>> 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.
<snip>
>
> +/**
> + * 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.
> + */
> +void xsaves_nmi(struct xregs_state *xstate, u64 mask)
> +{
> + int err;
> +
> + if (!in_nmi())
> + return;
> +
> + XSTATE_OP(XSAVES, xstate, (u32)mask, (u32)(mask >> 32), err);
> + WARN_ON_ONCE(err);
> +}
> +
There are xsaves()/xrstors() functions, already narrowed to the
"independent" feature set only. So, adding a new xsaves_yyy() variant
for a different use case -- without renaming the existing helpers to
something like xsaves_xxx() -- would make the naming scheme appear
inconsistent at a glance.
But looking back at history:
1. These helpers were established with "independent" in the name (though
they were initially described as for “dynamic” features):
copy_kernel_to_independent_supervisor()/
copy_independent_supervisor_to_kernel()
2. Later, Thomas reworked them, renaming and simplifying them to
xsaves()/xrstors(), and adding a refactored validator:
validate_xsaves_xrstors() [1]. At that point, their usage was
*relaxed* and not strictly limited to independent features.
3. Subsequently, in preparation for dynamic feature support, the helpers
were restricted again to independent features only [2]. This involved
renaming and enforcing stricter validation via
validate_independent_components().
Given that, rather than introducing a new wrapper for every additional
use case, another option could be to retain xsaves() naming but modestly
expand its scope. That would mean to add another allowance: features in
tightly constrained contexts (e.g., NMI). Perhaps, this approach can
keep the API simple while still expanding usage.
[1] a75c52896b6d ("x86/fpu/xstate: Sanitize handling of independent
features")
[2] f5daf836f292 ("x86/fpu: Restrict xsaves()/xrstors() to independent
states")
Powered by blists - more mailing lists