Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Greg KH <gregkh@linuxfoundation.org>]

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