On Thu, Sep 29, 2022 at 03:29:02PM -0700, Rick Edgecombe wrote:
> [...]
> xfeatures. So refactor these check's by having XCHECK_SZ() set a bool when
> it actually check's the xfeature. This ends up exceeding 80 chars, but was
Spelling nit through-out all patches: possessive used for plurals. E.g.
the above "check's" instances should be "checks". Please review all the
documentation and commit logs; it shows up a lot. :)
> [...]
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c8340156bfd2..5e6a4867fd05 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -39,26 +39,26 @@
> */
> static const char *xfeature_names[] =
> {
> - "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",
> - "unknown xstate feature" ,
> - "unknown xstate feature" ,
> - "unknown xstate feature" ,
> - "unknown xstate feature" ,
> - "unknown xstate feature" ,
> - "unknown xstate feature" ,
> - "AMX Tile config" ,
> - "AMX Tile data" ,
> - "unknown xstate feature" ,
> + "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 a strange style. Why not just leave the commas after the " ? Then
these kinds of multi-line updates aren't needed in the future.
> [...]
> - /*
> - * Make *SURE* to add any feature numbers in below if
> - * there are "holes" in the xsave state component
> - * numbers.
> - */
> - if ((nr < XFEATURE_YMM) ||
> - (nr >= XFEATURE_MAX) ||
> - (nr == XFEATURE_PT_UNIMPLEMENTED_SO_FAR) ||
> - ((nr >= XFEATURE_RSRVD_COMP_11) && (nr <= XFEATURE_RSRVD_COMP_16))) {
> + if (!chked) {
> WARN_ONCE(1, "no structure for xstate: %d\n", nr);
> XSTATE_WARN_ON(1);
> return false;
This clean-up feels like it should be part of a separate patch, but
okay. :)
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
--
Kees Cook
