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

[Andy Lutomirski <luto@amacapital.net>]

[-- Attachment #1: Type: text/plain, Size: 5693 bytes --]

On 10/4/19 11:34 PM, 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);
> +
>  static enum tsx_ctrl_states {
>  	TSX_CTRL_ENABLE,
>  	TSX_CTRL_DISABLE,
> @@ -150,3 +153,103 @@ void tsx_init(struct cpuinfo_x86 *c)
>  		setup_force_cpu_cap(X86_FEATURE_RTM);
>  	}
>  }
> +
> +static void tsx_update_this_cpu(void *arg)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> +	unsigned long enable = (unsigned long)arg;
> +
> +	if (enable) {
> +		tsx_enable();
> +		set_cpu_cap(c, X86_FEATURE_RTM);
> +		setup_force_cpu_cap(X86_FEATURE_RTM);
> +	} else {
> +		tsx_disable();
> +		clear_cpu_cap(c, X86_FEATURE_RTM);
> +		setup_clear_cpu_cap(X86_FEATURE_RTM);
> +	}
> +}

You can't use these setup_... functions after boot like this.  Also, I'm
not sure you can safely even set_cpu_cap(...) after boot.

> +
> +static void tsx_update_on_each_cpu(bool val)
> +{
> +	get_online_cpus();
> +	on_each_cpu(tsx_update_this_cpu, (void *)val, 1);
> +	put_online_cpus();

I sure hope that on_each_cpu() is sensible enough to automatically do
the get_online_cpus() thing.  Is it not?