Messages in this thread | | | Date | Mon, 3 Oct 2022 10:40:47 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v2 05/39] x86/fpu/xstate: Introduce CET MSR and XSAVES supervisor states |
| |
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@chromium.org>
-- Kees Cook
| |