On Sat, 2022-10-15 at 11:46 +0200, Borislav Petkov wrote:

On Thu, Sep 29, 2022 at 03:29:02PM -0700, Rick Edgecombe wrote:

quoted

Both XSAVE state components are supervisor states, even the state
controlling user-mode operation. This is a departure from earlier
features
like protection keys where the PKRU state a normal user (non-
supervisor)

^^^^^

A verb is missing in that sentence.

Oops yes.

quoted

+	"x87 floating point registers"			,
+	"SSE registers"					,
+	"AVX registers"					,
+	"MPX bounds registers"				,
+	"MPX CSR"					,
+	"AVX-512 opmask"				,
+	"AVX-512 Hi256"					,
+	"AVX-512 ZMM_Hi256"				,
+	"Processor Trace (unused)"			,
+	"Protection Keys User registers"		,
+	"PASID state"					,
+	"Control-flow User registers"			,
+	"Control-flow Kernel registers (unused)"	,
+	"unknown xstate feature"			,
+	"unknown xstate feature"			,
+	"unknown xstate feature"			,
+	"unknown xstate feature"			,
+	"AMX Tile config"				,
+	"AMX Tile data"					,
+	"unknown xstate feature"			,

What Kees said. :)

Sure, I'll adjust the comma.

quoted

+	XCHECK_SZ(&chked, sz, nr, XFEATURE_YMM,       struct
ymmh_struct);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_BNDREGS,   struct
mpx_bndreg_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_BNDCSR,    struct
mpx_bndcsr_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_OPMASK,    struct
avx_512_opmask_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_ZMM_Hi256, struct
avx_512_zmm_uppers_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_Hi16_ZMM,  struct
avx_512_hi16_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_PKRU,      struct
pkru_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_PASID,     struct
ia32_pasid_state);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_XTILE_CFG, struct
xtile_cfg);
+	XCHECK_SZ(&chked, sz, nr, XFEATURE_CET_USER,  struct
cet_user_state);

That looks silly. I wonder if you could do:

	switch (nr) {
	case XFEATURE_YMM:	XCHECK_SZ(sz, XFEATURE_YMM, struct
ymmh_struct);	  return;
	case XFEATURE_BNDREGS:	XCHECK_SZ(sz, XFEATURE_BNDREGS,
struct mpx_bndreg_state); return;
	case ...
	...
	default:
		/* that falls into the WARN etc */

and then you get rid of the if check in the macro itself and leave
the
macro be a dumb, unconditional one.

Hmmm.

Hmm yea. Another reason the actual define is passed in is that the
macro want's to stringify the XFEATURE define in order to generate the 
message like this:
XFEATURE_YMM: struct is 123 bytes, cpu state is 456 bytes

The exact format of the message is probably not too critical though. If
instead it used xfeature_names[], it could be:
[AVX registers]: struct is 123 bytes, cpu state is 456 bytes

The full block looks like (like you had):
switch (nr) {
case XFEATURE_YMM:	  return XCHECK_SZ(sz, nr, struct ymmh_struct);
case XFEATURE_BNDREGS:	  return XCHECK_SZ(sz, nr, struct
mpx_bndreg_state);
case XFEATURE_BNDCSR:	  return XCHECK_SZ(sz, nr, struct
mpx_bndcsr_state);
case XFEATURE_OPMASK:	  return XCHECK_SZ(sz, nr, struct
avx_512_opmask_state);
case XFEATURE_ZMM_Hi256:  return XCHECK_SZ(sz, nr, struct
avx_512_zmm_uppers_state);
case XFEATURE_Hi16_ZMM:	  return XCHECK_SZ(sz, nr, struct
avx_512_hi16_state);
case XFEATURE_PKRU: 	  return XCHECK_SZ(sz, nr, struct pkru_state);
case XFEATURE_PASID: 	  return XCHECK_SZ(sz, nr, struct
ia32_pasid_state);
case XFEATURE_XTILE_CFG:  return XCHECK_SZ(sz, nr, struct xtile_cfg);
case XFEATURE_CET_USER:	  return XCHECK_SZ(sz, nr, struct
cet_user_state);
case XFEATURE_XTILE_DATA: check_xtile_data_against_struct(sz); return
true;
default:
	WARN_ONCE(1, "no structure for xstate: %d\n", nr);
	XSTATE_WARN_ON(1);
	return false;
}

I like how it fits the XFEATURE_XTILE_DATA check in with the rest.

Thanks!