[MODERATED] Re: [PATCH v5 09/11] TAAv5 9

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Oct 04, 2019 at 11:34:31PM -0700, speck for Pawan Gupta wrote:
> Transactional Synchronization Extensions (TSX) is an extension to the
> x86 instruction set architecture (ISA) that adds Hardware Transactional
> Memory (HTM) support. Changing TSX state currently requires a reboot.
> This may not be desirable when rebooting imposes a huge penalty. Add
> support to control TSX feature via a new sysfs file:
> /sys/devices/system/cpu/hw_tx_mem
> 
> - Writing 0|off|N|n to this file disables TSX feature on all the CPUs.
>   This is equivalent to boot parameter tsx=off.
> - Writing 1|on|Y|y to this file enables TSX feature on all the CPUs.
>   This is equivalent to boot parameter tsx=on.
> - Reading from this returns the status of TSX feature.
> - When TSX control is not supported this interface is not visible in
>   sysfs.
> 
> Changing the TSX state from this interface also updates CPUID.RTM
> feature bit.  From the kernel side, this feature bit doesn't result in
> any ALTERNATIVE code patching.  No memory allocations are done to
> save/restore user state. No code paths in outside of the tests for
> vulnerability to TAA are dependent on the value of the feature bit.  In
> general the kernel doesn't care whether RTM is present or not.
> 
> Applications typically look at CPUID bits once at startup (or when first
> calling into a library that uses the feature).  So we have a couple of
> cases to cover:
> 
> 1) An application started and saw that RTM was enabled, so began
>    to use it. Then TSX was disabled.  Net result in this case is that
>    the application will keep trying to use RTM, but every xbegin() will
>    immediately abort the transaction. This has a performance impact to
>    the application, but it doesn't affect correctness because all users
>    of RTM must have a fallback path for when the transaction aborts. Note
>    that even if an application is in the middle of a transaction when we
>    disable RTM, we are safe. The XPI that we use to update the TSX_CTRL
>    MSR will abort the transaction (just as any interrupt would abort
>    a transaction).
> 
> 2) An application starts and sees RTM is not available. So it will
>    always use alternative paths. Even if TSX is enabled and RTM is set,
>    applications in general do not re-evaluate their choice so will
>    continue to run in non-TSX mode.
> 
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Reviewed-by: Mark Gross <mgross@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Tested-by: Neelima Krishnan <neelima.krishnan@intel.com>
> ---
>  .../ABI/testing/sysfs-devices-system-cpu      |  23 ++++
>  arch/x86/kernel/cpu/tsx.c                     | 103 ++++++++++++++++++
>  drivers/base/cpu.c                            |  32 +++++-
>  include/linux/cpu.h                           |   6 +
>  4 files changed, 163 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 5f7d7b14fa44..0c3c8c462285 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -562,3 +562,26 @@ Description:	Umwait control
>  			  or C0.2 state. The time is an unsigned 32-bit number.
>  			  Note that a value of zero means there is no limit.
>  			  Low order two bits must be zero.
> +
> +What:		/sys/devices/system/cpu/hw_tx_mem
> +Date:		August 2019
> +Contact:	Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> +		Linux kernel mailing list <linux-kernel@vger.kernel.org>
> +Description:	Hardware Transactional Memory (HTM) control.
> +
> +		Read/write interface to control HTM feature for all the CPUs in
> +		the system.  This interface is only present on platforms that
> +		support HTM control. HTM is a hardware feature to speed up the
> +		execution of multi-threaded software through lock elision. An
> +		example of HTM implementation is Intel Transactional
> +		Synchronization Extensions (TSX).
> +
> +			Read returns the status of HTM feature.
> +
> +			0: HTM is disabled
> +			1: HTM is enabled
> +
> +			Write sets the state of HTM feature.
> +
> +			0: Disables HTM
> +			1: Enables HTM
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index 73a0e5af3720..2cea038fdcba 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -10,9 +10,12 @@
>  
>  #include <linux/processor.h>
>  #include <linux/cpufeature.h>
> +#include <linux/cpu.h>
>  
>  #include "cpu.h"
>  
> +static DEFINE_MUTEX(tsx_mutex);

I think I asked this before, but in looking at the code I still can't
figure it out.  What exactly is this protecting?

It looks like you want to keep only one "writer" out of the sysfs store
function at a time, but:

> +ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	enum tsx_ctrl_states requested_state;
> +	ssize_t ret;
> +	bool val;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&tsx_mutex);
> +
> +	if (val) {
> +		tsx_user_cmd = TSX_USER_CMD_ON;
> +		requested_state = TSX_CTRL_ENABLE;
> +	} else {
> +		tsx_user_cmd = TSX_USER_CMD_OFF;
> +		requested_state = TSX_CTRL_DISABLE;
> +	}
> +
> +	/* Current state is same as the reqested state, do nothing */
> +	if (tsx_ctrl_state == requested_state)
> +		goto exit;
> +
> +	tsx_ctrl_state = requested_state;
> +
> +	/*
> +	 * Changing the TSX state from this interface also updates CPUID.RTM
> +	 * feature  bit. From the kernel side, this feature bit doesn't result
> +	 * in any ALTERNATIVE code patching.  No memory allocations are done to
> +	 * save/restore user state. No code paths in outside of the tests for
> +	 * vulnerability to TAA are dependent on the value of the feature bit.
> +	 * In general the kernel doesn't care whether RTM is present or not.
> +	 *
> +	 * From the user side it is a bit fuzzier. Applications typically look
> +	 * at CPUID bits once at startup (or when first calling into a library
> +	 * that uses the feature).  So we have a couple of cases to cover:
> +	 *
> +	 * 1) An application started and saw that RTM was enabled, so began
> +	 *    to use it. Then TSX was disabled.  Net result in this case is
> +	 *    that the application will keep trying to use RTM, but every
> +	 *    xbegin() will immediately abort the transaction. This has a
> +	 *    performance impact to the application, but it doesn't affect
> +	 *    correctness because all users of RTM must have a fallback path
> +	 *    for when the transaction aborts. Note that even if an application
> +	 *    is in the middle of a transaction when we disable RTM, we are
> +	 *    safe. The XPI that we use to update the TSX_CTRL MSR will abort
> +	 *    the transaction (just as any interrupt would abort a
> +	 *    transaction).
> +	 *
> +	 * 2) An application starts and sees RTM is not available. So it will
> +	 *    always use alternative paths. Even if TSX is enabled and RTM is
> +	 *    set, applications in general do not re-evaluate their choice so
> +	 *    will continue to run in non-TSX mode.
> +	 */
> +	tsx_update_on_each_cpu(val);
> +exit:
> +	mutex_unlock(&tsx_mutex);

What I think you want to do is just protect the tsx_update_on_each_cpu()
function, right?

So, you are locking _outside_ of a function call?  That's a sure way to
madness over time.  If this function is so special that it can not be
called multiple times at once, then put the lock _inside_ the function,
right?

Otherwise you could have other places call that function, and this
single lock is not going to protect anything :(

And why is tsx_user_cmd needed, and global?

For a "simple" turn this feature on/off function, it feels like you have
way too many different states and variables holding them here.  Why do
you need more than just one global state, and one variable showing what
was asked for here?

thanks,

greg k-h