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

[Tim Chen]

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@xxxxxxxxxxxxxxx>
>> From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> ---
> 
> <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
>