linux-kernel - Re: [PATCH v5 4/5] x86/umwait: Add sysfs interface to control umwait maximum time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Jun 2019 00:40:33 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ashok Raj <ashok.raj@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v5 4/5] x86/umwait: Add sysfs interface to control umwait
 maximum time

On Wed, 19 Jun 2019, Fenghua Yu wrote:

> Users can write an unsigned 32-bit number to
> /sys/devices/system/cpu/umwait_control/max_time to change the default

Users? Administrators can. Users NOT.

> value. Note that a value of zero means there is no limit. Low order
> two bits must be zero.

...

> +static ssize_t max_time_store(struct device *kobj,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 max_time;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max_time);
> +	if (ret)
> +		return ret;
> +
> +	/* bits[1:0] must be zero */
> +	if (max_time & ~MSR_IA32_UMWAIT_CONTROL_MAX_TIME)
> +		return -EINVAL;
> +
> +	mutex_lock(&umwait_lock);
> +
> +	if (max_time == get_umwait_ctrl_max_time())
> +		goto out_unlock;
> +
> +	WRITE_ONCE(umwait_control_cached,
> +		   UMWAIT_CTRL_VAL(max_time, get_umwait_ctrl_c02()));

Same convoluted logic with reading the cached value twice to confuse the
reader.

	ctrl = READ_ONCE(umwait_control_cached);
	if (max_time == umwait_ctrl_max_time(ctrl))
		goto out_unlock;

	ctrl = (ctrl & ~MSR_IA32_UMWAIT_CONTROL_TIME_MASK) | max_time;
	WRITE_ONCE(umwait_control_cached, ctrl);

Simple, right?

But this can be done even simpler with a shared update function:

static void umwait_update_control(u32 maxtime, bool c02_enable)
{
        u32 ctrl = maxtime & MSR_IA32_UMWAIT_CONTROL_TIME_MASK;

        if (!c02_enable)
                ctrl |= MSR_IA32_UMWAIT_CONTROL_C02_DISABLE;

        WRITE_ONCE(umwait_control_cached, ctrl);
        /* Propagate to all CPUs */
        on_each_cpu(umwait_update_control_msr, NULL, 1);
}

With that both functions become trivial and do not have duplicated code.

Thanks,

	tglx