lkml.org 
[lkml]   [2019]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 14/20] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
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

\
 
 \ /
  Last update: 2019-04-04 21:12    [W:0.250 / U:44.740 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site