[MODERATED] Re: [PATCH v6 9/9] TAAv6 9

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Oct 09, 2019 at 04:30:57PM -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.
> 
> When the TSX state is changed from the sysfs interface, TSX Async Abort
> (TAA) mitigation state also needs to be updated. Set the TAA mitigation
> state as per TSX and VERW static branch state.
> 
> 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 ++++
>  .../admin-guide/hw-vuln/tsx_async_abort.rst   |  29 +++++
>  arch/x86/kernel/cpu/bugs.c                    |  21 +++-
>  arch/x86/kernel/cpu/cpu.h                     |   3 +-
>  arch/x86/kernel/cpu/tsx.c                     | 100 +++++++++++++++++-
>  drivers/base/cpu.c                            |  32 +++++-
>  include/linux/cpu.h                           |   6 ++
>  7 files changed, 210 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 9ef9efa4e717..09a90be9f8cc 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -563,3 +563,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/Documentation/admin-guide/hw-vuln/tsx_async_abort.rst b/Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
> index 58f24db49615..b62bc749fd8c 100644
> --- a/Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
> +++ b/Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
> @@ -207,6 +207,35 @@ buffers.  For platforms without TSX control "tsx" command line argument has no
>  effect.
>  
>  
> +.. _taa_mitigation_sysfs:
> +
> +Mitigation control using sysfs
> +------------------------------
> +
> +For those affected systems that can not be frequently rebooted to enable or
> +disable TSX, sysfs can be used as an alternative after installing the updates.
> +The possible values for the file /sys/devices/system/cpu/hw_tx_mem are:
> +
> +  ============  =============================================================
> +  0		Disable TSX. Upon entering a TSX transactional region, the code
> +                will immediately abort, before any instruction executes within
> +                the transactional region even speculatively, and continue on
> +                the fallback. Equivalent to boot parameter "tsx=off".
> +
> +  1		Enable TSX. Equivalent to boot parameter "tsx=on".
> +
> +  ============  =============================================================
> +
> +Reading from this file returns the status of TSX feature. This file is only
> +present on systems that support TSX control.
> +
> +When disabling TSX by using the sysfs mechanism, applications that are already
> +running and use TSX will see their transactional regions aborted and execution
> +flow will be redirected to the fallback, losing the benefits of the
> +non-blocking path. TSX needs fallback code to guarantee correct execution
> +without transactional regions.
> +
> +
>  Mitigation selection guide
>  --------------------------
>  
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 073687ddd06d..3bd8040ef747 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -274,7 +274,7 @@ early_param("mds", mds_cmdline);
>  #define pr_fmt(fmt)	"TAA: " fmt
>  
>  /* Default mitigation for TAA-affected CPUs */
> -static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW;
> +static enum taa_mitigations taa_mitigation = TAA_MITIGATION_VERW;
>  static bool taa_nosmt __ro_after_init;
>  
>  static const char * const taa_strings[] = {
> @@ -374,6 +374,25 @@ static int __init tsx_async_abort_cmdline(char *str)
>  }
>  early_param("tsx_async_abort", tsx_async_abort_cmdline);
>  
> +void taa_update_mitigation(bool tsx_enabled)
> +{
> +	/*
> +	 * When userspace changes the TSX state, update taa_mitigation
> +	 * so that the updated mitigation state is shown in:
> +	 * /sys/devices/system/cpu/vulnerabilities/tsx_async_abort
> +	 *
> +	 * Check if TSX is disabled.
> +	 * Check if CPU buffer clear is enabled.
> +	 * else the system is vulnerable.
> +	 */
> +	if (!tsx_enabled)
> +		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
> +	else if (static_key_count(&mds_user_clear.key))
> +		taa_mitigation = TAA_MITIGATION_VERW;
> +	else
> +		taa_mitigation = TAA_MITIGATION_OFF;
> +}
> +
>  #undef pr_fmt
>  #define pr_fmt(fmt)     "Spectre V1 : " fmt
>  
> diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
> index 38ab6e115eac..c0e2ae982692 100644
> --- a/arch/x86/kernel/cpu/cpu.h
> +++ b/arch/x86/kernel/cpu/cpu.h
> @@ -51,11 +51,12 @@ enum tsx_ctrl_states {
>  	TSX_CTRL_NOT_SUPPORTED,
>  };
>  
> -extern __ro_after_init enum tsx_ctrl_states tsx_ctrl_state;
> +extern enum tsx_ctrl_states tsx_ctrl_state;
>  
>  extern void __init tsx_init(void);
>  extern void tsx_enable(void);
>  extern void tsx_disable(void);
> +extern void taa_update_mitigation(bool tsx_enabled);
>  #else
>  static inline void tsx_init(void) { }
>  #endif /* CONFIG_CPU_SUP_INTEL */
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index e93abe6f0bb9..96320449abb7 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -10,12 +10,15 @@
>  
>  #include <linux/processor.h>
>  #include <linux/cpufeature.h>
> +#include <linux/cpu.h>
>  
>  #include <asm/cmdline.h>
>  
>  #include "cpu.h"
>  
> -enum tsx_ctrl_states tsx_ctrl_state __ro_after_init = TSX_CTRL_NOT_SUPPORTED;
> +static DEFINE_MUTEX(tsx_mutex);

I still don't know what this is trying to "protect".  Please at least
document it so I have a chance to review it...


> +
> +enum tsx_ctrl_states tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
>  
>  void tsx_disable(void)
>  {
> @@ -118,3 +121,98 @@ void __init tsx_init(void)
>  		setup_force_cpu_cap(X86_FEATURE_RTM);
>  	}
>  }
> +
> +static void tsx_update_this_cpu(void *arg)
> +{
> +	unsigned long enable = (unsigned long)arg;
> +
> +	if (enable)
> +		tsx_enable();
> +	else
> +		tsx_disable();
> +}
> +
> +/* Take tsx_mutex lock and update tsx_ctrl_state when calling this function */
> +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();
> +}

Why take the lock?  This is only called in one place.

> +ssize_t hw_tx_mem_show(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	return sprintf(buf, "%d\n", tsx_ctrl_state == TSX_CTRL_ENABLE ? 1 : 0);
> +}
> +
> +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)
> +		requested_state = TSX_CTRL_ENABLE;
> +	else
> +		requested_state = TSX_CTRL_DISABLE;

Why is lock grabbed above and not here?

And again, why a lock at all...

thanks,

greg k-h