Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752689AbeAFIyH (ORCPT <rfc822;ralf@linux-mips.org> + 1 other);
        Sat, 6 Jan 2018 03:54:07 -0500
Received: from mail.linuxfoundation.org ([140.211.169.12]:42290 "EHLO
        mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752550AbeAFIyH (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 6 Jan 2018 03:54:07 -0500
Date: Sat, 6 Jan 2018 09:54:10 +0100
From: Greg KH <gregkh@linuxfoundation.org>
To: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
        Andy Lutomirski <luto@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andi Kleen <ak@linux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable
 SPEC_CTRL feature
Message-ID: <20180106085410.GA4380@kroah.com>
References: <cover.1515204614.git.tim.c.chen@linux.intel.com>
 <b77c0cfc2d50ebdaa721f95012fd735cc374f04c.1515204614.git.tim.c.chen@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b77c0cfc2d50ebdaa721f95012fd735cc374f04c.1515204614.git.tim.c.chen@linux.intel.com>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.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