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
