On Tue, 2022-02-08 at 09:51 +0100, Thomas Gleixner wrote:

I like the approach in principle, but you still expose the xstate
internals via the void pointer. It's just a question of time that
this
is type casted and abused in interesting ways.

Thanks for taking a look. I have to say though, these changes are
making me scratch my head a bit. Should we really design around callers
digging into mysterious pointers with magic offsets instead of using
easy helpers full of warnings about pitfalls? It should look odd in a
code review too I would think.

Something like the below untested (on top of the whole series)
preserves
the encapsulation and reduces the code at the call sites.

It loses the ability to know which MSR write actually failed. It also
loses the ability to perform read/write logic under a single
transaction. The latter is not needed for this series, but this snippet
from the IBT series does it:

int ibt_get_clear_wait_endbr(void)
{
	void *xstate;
	u64 msr_val = 0;

	if (!current->thread.shstk.ibt)
		return 0;

	xstate = start_update_xsave_msrs(XFEATURE_CET_USER);
	if (!xsave_rdmsrl(xstate, MSR_IA32_U_CET, &msr_val))
		xsave_wrmsrl(xstate, MSR_IA32_U_CET, msr_val &
~CET_WAIT_ENDBR);
	end_update_xsave_msrs();

	return msr_val & CET_WAIT_ENDBR;
}

I suppose we could just add a new function to do that logic in a single
transaction when the time comes. But inventing data structures to
describe work to be passed off to some executor always seems to break
at the first new requirement. What I usually wanted was a programming
language, and I already had it.

Not to bikeshed though, it will still get the job done.