Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753129AbeAFSLC (ORCPT <rfc822;ralf@linux-mips.org> + 1 other);
        Sat, 6 Jan 2018 13:11:02 -0500
Received: from mga11.intel.com ([192.55.52.93]:33937 "EHLO mga11.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752547AbeAFSLA (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 6 Jan 2018 13:11:00 -0500
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.46,322,1511856000"; 
   d="scan'208";a="164702016"
Subject: Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable
 SPEC_CTRL feature
To: Greg KH <gregkh@linuxfoundation.org>
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
References: <cover.1515204614.git.tim.c.chen@linux.intel.com>
 <b77c0cfc2d50ebdaa721f95012fd735cc374f04c.1515204614.git.tim.c.chen@linux.intel.com>
 <20180106085410.GA4380@kroah.com>
From: Tim Chen <tim.c.chen@linux.intel.com>
Message-ID: <0de2d41c-368c-685f-ac52-cf7ce440ac60@linux.intel.com>
Date: Sat, 6 Jan 2018 10:10:59 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20180106085410.GA4380@kroah.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
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 01/06/2018 12:54 AM, Greg KH wrote:
> 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?

Right now no module will need it.  I'll get rid of it.

> 
> 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 :)

Noted.  I'll add spec_ctrl in front of those.

> 
> thanks,
> 
> greg k-h
>