Re: [RFC PATCH V2 03/13] x86/fpu/xstate: Add xsaves_nmi

[Chang S. Bae]

On 6/26/2025 12:56 PM, kan.liang@xxxxxxxxxxxxxxx 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")