lkml.org 
[lkml]   [2018]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Patch v4 03/18] x86/speculation: Reorganize cpu_show_common()
    Tim,

    On Tue, 30 Oct 2018, Tim Chen wrote:
    > Extract the logic to show IBPB, STIBP usages in cpu_show_common()
    > into helper functions.
    >
    > Later patches will add other userspace Spectre v2 mitigation modes.
    > This patch makes it easy to show IBPB and STIBP
    > usage scenario according to the mitigation mode.

    First of all, I asked you before to do:

    # git grep 'This patch' Documentation/process

    This leads you to:

    "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
    instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
    to do frotz", as if you are giving orders to the codebase to change
    its behaviour."

    Documentation is there for a reason.

    Aside of that, I'd really have a hard time to figure out what you are
    trying to say, if I didn't have the context already. Change logs need to
    make sense on their own. So something like this:

    The Spectre V2 printout in cpu_show_common() handles conditionals for the
    various mitigation methods directly in the sprintf() argument list. That's
    hard to read and will become unreadable if more complex decisions need to
    be made for a particular method.

    Move the conditionals for STIBP and IBPB string selection into helper
    functions, so they can be extended later on.

    follows the obvious ordering for change logs:

    1) Describe context and problem

    2) Describe the solution

    and is understandable without needing to know about the context in which
    this change was developed.

    Hmm? This is a suggestion, feel free to rewrite it in you own words. The
    same applies to other change logs as well. I won't comment on those.

    > static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
    > char *buf, unsigned int bug)
    > {
    > @@ -872,9 +888,8 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
    >
    > case X86_BUG_SPECTRE_V2:
    > return sprintf(buf, "%s%s%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
    > - boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
    > boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
    > - (x86_spec_ctrl_base & SPEC_CTRL_STIBP) ? ", STIBP" : "",
    > + ibpb_state(), stibp_state(),
    > boot_cpu_has(X86_FEATURE_RSB_CTXSW) ? ", RSB filling" : "",
    > spectre_v2_module_string());

    Any particular reason for changing the output ordering here? If yes, then
    the changelog should mention it. If no, why?

    Thanks,

    tglx

    \
     
     \ /
      Last update: 2018-11-03 19:08    [W:4.071 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site