Re: [PATCH 23/35] x86/fpu: Add helpers for modifying supervisor xstate

[Dave Hansen <dave.hansen@intel.com>]

On 1/30/22 13:18, Rick Edgecombe wrote:
> Add helpers that can be used to modify supervisor xstate safely for the
> current task.

This should be at the end of the changelog.

> State for supervisors xstate based features can be live and
> accesses via MSR's, or saved in memory in an xsave buffer. When the
> kernel needs to modify this state it needs to be sure to operate on it
> in the right place, so the modifications don't get clobbered.

We tend to call these "supervisor xfeatures".  The "state is in the
registers" we call "active".  Maybe:

	Just like user xfeatures, supervisor xfeatures can be either
	active in the registers or inactive and present in the task FPU
	buffer.  If the registers are active, the registers can be
	modified directly.  If the registers are not active, the
	modification must be performed on the task FPU buffer.


> In the past supervisor xstate features have used get_xsave_addr()
> directly, and performed open coded logic handle operating on the saved
> state correctly. This has posed two problems:
>  1. It has logic that has been gotten wrong more than once.
>  2. To reduce code, less common path's are not optimized. Determination

			   "paths" ^


> xstate = start_update_xsave_msrs(XFEATURE_FOO);
> r = xsave_rdmsrl(state, MSR_IA32_FOO_1, &val)
> if (r)
> 	xsave_wrmsrl(state, MSR_IA32_FOO_2, FOO_ENABLE);
> end_update_xsave_msrs();

This looks OK.  I'm not thrilled about it.  The
start_update_xsave_msrs() can probably drop the "_msrs".  Maybe:

	start_xfeature_update(...);

Also, if you have to do the address lookup in xsave_rdmsrl() anyway, I
wonder if the 'xstate' should just be a full fledged 'struct xregs_state'.

The other option would be to make a little on-stack structure like:

	struct xsave_update {
		int feature;
		struct xregs_state *xregs;
	};

Then you do:

	struct xsave_update xsu;
	...
	start_update_xsave_msrs(&xsu, XFEATURE_FOO);

and then pass it along to each of the other operations:

	r = xsave_rdmsrl(xsu, MSR_IA32_FOO_1, &val)

It's slightly less likely to get type confused as a 'void *';

> +static u64 *__get_xsave_member(void *xstate, u32 msr)
> +{
> +	switch (msr) {
> +	/* Currently there are no MSR's supported */
> +	default:
> +		WARN_ONCE(1, "x86/fpu: unsupported xstate msr (%u)\n", msr);
> +		return NULL;
> +	}
> +}

Just to get an idea what this is doing, it's OK to include the shadow
stack MSRs in here.

Are you sure this should return a u64*?  We have lots of <=64-bit XSAVE
fields.

> +/*
> + * Return a pointer to the xstate for the feature if it should be used, or NULL
> + * if the MSRs should be written to directly. To do this safely, using the
> + * associated read/write helpers is required.
> + */
> +void *start_update_xsave_msrs(int xfeature_nr)
> +{
> +	void *xstate;
> +
> +	/*
> +	 * fpregs_lock() only disables preemption (mostly). So modifing state

							 modifying ^
	
> +	 * in an interrupt could screw up some in progress fpregs operation,

						^ in-progress

> +	 * but appear to work. Warn about it.
> +	 */
> +	WARN_ON_ONCE(!in_task());
> +	WARN_ON_ONCE(current->flags & PF_KTHREAD);

This might also be a good spot to check that xfeature_nr is in
fpstate.xfeatures.

> +	fpregs_lock();
> +
> +	fpregs_assert_state_consistent();
> +
> +	/*
> +	 * If the registers don't need to be reloaded. Go ahead and operate on the
> +	 * registers.
> +	 */
> +	if (!test_thread_flag(TIF_NEED_FPU_LOAD))
> +		return NULL;
> +
> +	xstate = get_xsave_addr(&current->thread.fpu.fpstate->regs.xsave, xfeature_nr);
> +
> +	/*
> +	 * If regs are in the init state, they can't be retrieved from
> +	 * init_fpstate due to the init optimization, but are not nessarily

							necessarily ^

Spell checker time.  ":set spell" in vim works for me nicely.

> +	 * zero. The only option is to restore to make everything live and
> +	 * operate on registers. This will clear TIF_NEED_FPU_LOAD.
> +	 *
> +	 * Otherwise, if not in the init state but TIF_NEED_FPU_LOAD is set,
> +	 * operate on the buffer. The registers will be restored before going
> +	 * to userspace in any case, but the task might get preempted before
> +	 * then, so this possibly saves an xsave.
> +	 */
> +	if (!xstate)
> +		fpregs_restore_userregs();

Won't fpregs_restore_userregs() end up setting TIF_NEED_FPU_LOAD=0?
Isn't that a case where a "return NULL" is needed?

In any case, this makes me think this code should start out stupid and
slow.  Keep the API as-is, but make the first patch unconditionally do
the WRMSR.  Leave the "fast" buffer modifications for a follow-on patch.