lkml.org 
[lkml]   [2021]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH v10 02/28] x86/fpu/xstate: Modify the initialization helper to handle both static and dynamic buffers
    Date
    On Oct 1, 2021, at 05:45, Thomas Gleixner <tglx@linutronix.de> wrote:
    > On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
    >> Have the function initializing the XSTATE buffer take a struct fpu *
    >> pointer in preparation for dynamic state buffer support.
    >>
    >> init_fpstate is a special case, which is indicated by a null pointer
    >> parameter to fpstate_init().
    >>
    >> Also, fpstate_init_xstate() now accepts the state component bitmap to
    >> customize the compacted format.
    >
    > That's not a changelog. Changelogs have to explain the WHY not the WHAT.
    >
    > I can see the WHY when I look at the later changes, but that's not how
    > it works.

    The same feedback was raised before [1]. I thought this changelog has been
    settled down with Boris [2].

    How about:

    “To prepare dynamic features, change fpstate_init()’s argument to a struct
    fpu * pointer instead of a struct fpregs_state * pointer. A struct fpu
    will have new fields to handle dynamic features."

    With fpstate_init_xstate() changes in a separate patch and defining init_fpu,
    the last two sentences shall be removed.

    > Also the subject of this patch is just wrong. It does not make the
    > functions handle dynamic buffers, it prepares them to add support for
    > that later.

    How about “Prepare fpstate_init() to handle dynamic features"

    >> +static inline void fpstate_init_xstate(struct xregs_state *xsave, u64 mask)
    >> +{
    >> + /*
    >> + * XRSTORS requires these bits set in xcomp_bv, or it will
    >> + * trigger #GP:
    >> + */
    >> + xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | mask;
    >> +}
    >
    > This wants to be a separate cleanup patch which replaces the open coded
    > variant here:

    Okay, maybe the change becomes to be the new patch1.

    >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
    >> index fc1d529547e6..0fed7fbcf2e8 100644
    >> --- a/arch/x86/kernel/fpu/xstate.c
    >> +++ b/arch/x86/kernel/fpu/xstate.c
    >> @@ -395,8 +395,7 @@ static void __init setup_init_fpu_buf(void)
    >> print_xstate_features();
    >>
    >> if (boot_cpu_has(X86_FEATURE_XSAVES))
    >> - init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
    >> - xfeatures_mask_all;
    >> + fpstate_init_xstate(&init_fpstate.xsave, xfeatures_mask_all);

    Thanks,
    Chang

    [1] https://lore.kernel.org/lkml/20201207171251.GB16640@zn.tnic/
    [2] https://lore.kernel.org/lkml/20210115124038.GA11337@zn.tnic/


    \
     
     \ /
      Last update: 2021-10-04 00:36    [W:4.103 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site