Messages in this thread | | | Subject | Re: [Patch v4 03/18] x86/speculation: Reorganize cpu_show_common() | From | Tim Chen <> | Date | Mon, 5 Nov 2018 11:12:15 -0800 |
| |
On 11/03/2018 11:07 AM, Thomas Gleixner wrote: > 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.
Thanks for the suggestion. Will update.
> >> 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? >
I was putting the features related to user application protection together. It was not necessary and I can leave it at the same place.
Tim
| |