| Message ID | 1554326526-172295-15-git-send-email-fenghua.yu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | x86/split_lock: Enable split locked accesses detection | expand |
On Wed, 3 Apr 2019, Fenghua Yu wrote: > + > +static ssize_t > +split_lock_detect_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + return sprintf(buf, "%u\n", READ_ONCE(split_lock_detect_val)); Please stop sprinkling READ_ONCE all over the place or can you explain why this is in any way useful? You know what READ/WRITE_ONCE() is for, right? > +} > + > +static ssize_t > +split_lock_detect_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + u32 val, l, h; > + int cpu, ret; > + > + ret = kstrtou32(buf, 10, &val); > + if (ret) > + return ret; > + > + if (val != DISABLE_SPLIT_LOCK_DETECT && val != ENABLE_SPLIT_LOCK_DETECT) > + return -EINVAL; As this is really a simple boolean you can just use strtobool() and be done with it. > + > + /* > + * Since split lock could be disabled by kernel #AC handler or user > + * may directly change bit 29 in MSR_TEST_CTL, split lock setting on The user can change bit 29 in that MSR? If you talk about /dev/msr then I really do not care. That interface should die. Aside of that your usage of the term 'user' is really misleading and inconsistent all over the place. > + * each CPU may be different from global setting split_lock_detect_val > + * by now. Update MSR on each CPU, so all of CPUs will have same split > + * lock setting. That helps in which way? If #AC was detected in the kernel then 1) It's likely to be switched off again right away 2) The WARN_ONCE() already triggered and will not warn again. So what's the point here, really? If the kernel triggers #AC, game over. Fix the kernel first. If your kernel is clean, then why do you need that knob at all? > + */ > + mutex_lock(&split_lock_detect_mutex); > + > + WRITE_ONCE(split_lock_detect_val, val); Oh well. > + /* > + * Get MSR_TEST_CTL on this CPU, assuming all CPUs have same value > + * in the MSR except split lock detection bit (bit 29). And some day in the future this breaks because MRS_TEST_CTL has some other shiny bits. > + */ > + rdmsr(MSR_TEST_CTL, l, h); > + l = new_sp_test_ctl_val(l); > + /* Update the split lock detection setting on all online CPUs. */ > + for_each_online_cpu(cpu) And what exactly protects the online cpu mask? > + wrmsr_on_cpu(cpu, MSR_TEST_CTL, l, h); Oh well. Instead of just having a function which does: fun() if (ac_...enabled) msr_set_bit() else msr_clear_bit() and invoke that from cpu init code and from here via on_each_cpu() or such? > + mutex_unlock(&split_lock_detect_mutex); > + > + return count; > +} > + > +static DEVICE_ATTR_RW(split_lock_detect); > + > +static int __init split_lock_init(void) > +{ > + int ret; > + > + if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) > + return -ENODEV; > + > + ret = device_create_file(cpu_subsys.dev_root, > + &dev_attr_split_lock_detect); > + if (ret) > + return ret; > + > + return 0; What's wrong with: return device_create_file(); ??? Not hard enough to read, right? > +} > + Pointless empty line. > +subsys_initcall(split_lock_init); Thanks, tglx
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index ae3e327d5e35..166033fa8423 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -1102,3 +1102,69 @@ void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT) set_split_lock_detect(); } + +static ssize_t +split_lock_detect_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + return sprintf(buf, "%u\n", READ_ONCE(split_lock_detect_val)); +} + +static ssize_t +split_lock_detect_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + u32 val, l, h; + int cpu, ret; + + ret = kstrtou32(buf, 10, &val); + if (ret) + return ret; + + if (val != DISABLE_SPLIT_LOCK_DETECT && val != ENABLE_SPLIT_LOCK_DETECT) + return -EINVAL; + + /* + * Since split lock could be disabled by kernel #AC handler or user + * may directly change bit 29 in MSR_TEST_CTL, split lock setting on + * each CPU may be different from global setting split_lock_detect_val + * by now. Update MSR on each CPU, so all of CPUs will have same split + * lock setting. + */ + mutex_lock(&split_lock_detect_mutex); + + WRITE_ONCE(split_lock_detect_val, val); + + /* + * Get MSR_TEST_CTL on this CPU, assuming all CPUs have same value + * in the MSR except split lock detection bit (bit 29). + */ + rdmsr(MSR_TEST_CTL, l, h); + l = new_sp_test_ctl_val(l); + /* Update the split lock detection setting on all online CPUs. */ + for_each_online_cpu(cpu) + wrmsr_on_cpu(cpu, MSR_TEST_CTL, l, h); + + mutex_unlock(&split_lock_detect_mutex); + + return count; +} + +static DEVICE_ATTR_RW(split_lock_detect); + +static int __init split_lock_init(void) +{ + int ret; + + if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) + return -ENODEV; + + ret = device_create_file(cpu_subsys.dev_root, + &dev_attr_split_lock_detect); + if (ret) + return ret; + + return 0; +} + +subsys_initcall(split_lock_init);
The interface /sys/device/system/cpu/split_lock_detect is added to allow user to control split lock detection and show current split lock detection setting. Writing 1 to the file enables split lock detection and writing 0 disables split lock detection. Split lock detection is enabled or disabled on all CPUs. Reading the file returns current global split lock detection setting: 0: disabled 1: enabled Please note the interface only shows global setting. During run time, split lock detection on one CPU may be disabled if split lock in kernel code happens on the CPU. The interface doesn't show split lock detection status on individual CPU. User can run rdmsr on 0x33 to check split lock detection on each CPU. Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> --- arch/x86/kernel/cpu/intel.c | 66 +++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)