Re: [PATCH v2 05/39] x86/fpu/xstate: Introduce CET MSR and XSAVES supervisor states

Borislav Petkov
2022-10-15T09:47:05Z

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.

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

> + 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.

--
Regards/Gruss,
Boris.