lkml.org 
[lkml]   [2018]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> From: Tim Chen <tim.c.chen@linux.intel.com>
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> There are 2 ways to control IBRS
>
> 1. At boot time
> noibrs kernel boot parameter will disable IBRS usage
>
> Otherwise if the above parameters are not specified, the system
> will enable ibrs and ibpb usage if the cpu supports it.
>
> 2. At run time
> echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
> echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
> echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
>
> The implementation was updated with input and suggestions from Andrea Arcangeli.
>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---

<snip>

> index 0000000..4fda38b
> --- /dev/null
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Thank you for these markings.

> --- /dev/null
> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
> @@ -0,0 +1,160 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/mutex.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/spec_ctrl.h>
> +#include <asm/cpufeature.h>
> +
> +unsigned int dynamic_ibrs __read_mostly;
> +EXPORT_SYMBOL_GPL(dynamic_ibrs);

What kernel module needs this symbol? A symbol can be "global" and not
be exported, those are two different things.

And again, horrible name for a global symbol, if this really is being
exported to modules (i.e. the world.)

> +
> +enum {
> + IBRS_DISABLED,

As you are making a user/kernel API here, shouldn't you enforce the
values this enum has "just to be sure"?

> + /* in host kernel, disabled in guest and userland */
> + IBRS_ENABLED,
> + /* in host kernel and host userland, disabled in guest */
> + IBRS_ENABLED_USER,
> + IBRS_MAX = IBRS_ENABLED_USER,
> +};

Also, this should be documented somewhere, Documentation/ABI/ perhaps?

> +static unsigned int ibrs_enabled;
> +static bool ibrs_admin_disabled;
> +
> +/* mutex to serialize IBRS control changes */
> +DEFINE_MUTEX(spec_ctrl_mutex);

static?

> +
> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
> +{
> + if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
> + if (!ibrs_admin_disabled) {
> + dynamic_ibrs = 1;
> + ibrs_enabled = IBRS_ENABLED;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature);

Again, what module needs this?

Same for all the exports in this file, and again, if they are needed in
modules, you need to get a better naming scheme. I'm not trying to
bikeshed, it really matters, our global symbol namespace is a mess,
don't make it worse :)

thanks,

greg k-h

\
 
 \ /
  Last update: 2018-01-06 09:54    [W:0.202 / U:4.448 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site