On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
> From: Tim Chen <tim.c.c...@linux.intel.com>
> From: Andrea Arcangeli <aarca...@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.c...@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
