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.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?
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
>
