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: > > 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. > > > + "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. > > > + 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!