Date:   Thu, 4 Apr 2019 21:11:18 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Fenghua Yu <fenghua.yu@intel.com>
cc:     Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Ashok Raj <ashok.raj@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Kalle Valo <kvalo@codeaurora.org>,
        Xiaoyao Li <xiaoyao.li@intel.com>,
        Michael Chan <michael.chan@broadcom.com>,
        Ravi V Shankar <ravi.v.shankar@intel.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        x86 <x86@kernel.org>, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH v6 14/20] x86/split_lock: Add a sysfs interface to
 enable/disable split lock detection during run time
In-Reply-To: <1554326526-172295-15-git-send-email-fenghua.yu@intel.com>
Message-ID: <alpine.DEB.2.21.1904042039150.1802@nanos.tec.linutronix.de>
References: <1554326526-172295-1-git-send-email-fenghua.yu@intel.com> <1554326526-172295-15-git-send-email-fenghua.yu@intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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