lkml.org 
[lkml]   [2022]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2022-10-03 19:41    [W:2.046 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site