From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (193.142.43.55:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 10 Oct 2019 06:54:29 -0000 Received: from mail.kernel.org ([198.145.29.99]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iISL8-0000yv-9Y for speck@linutronix.de; Thu, 10 Oct 2019 08:54:28 +0200 Date: Thu, 10 Oct 2019 08:54:12 +0200 From: Greg KH Subject: [MODERATED] Re: [PATCH v6 9/9] TAAv6 9 Message-ID: <20191010065412.GD256999@kroah.com> References: <5d9e6f13.1c69fb81.d7036.be99SMTPIN_ADDED_BROKEN@mx.google.com> MIME-Version: 1.0 In-Reply-To: <5d9e6f13.1c69fb81.d7036.be99SMTPIN_ADDED_BROKEN@mx.google.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: 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 > Reviewed-by: Mark Gross > Reviewed-by: Tony Luck > Tested-by: Neelima Krishnan > --- > .../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 > + Linux kernel mailing list > +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 > #include > +#include > > #include > > #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