[PATCH v3 00/10] x86/split_lock: Enable #AC exception for split locked accesses

[PATCH v3 00/10] x86/split_lock: Enable #AC exception for split locked accesses

[Fenghua Yu]

==Introduction==

A split lock is any atomic operation whose operand crosses two cache
lines. Since the operand spans two cache lines and the operation must
be atomic, the system locks the bus while the CPU accesses the two cache
lines.

During bus locking, request from other CPUs or bus agents for control
of the bus are blocked. Blocking bus access from other CPUs plus
overhead of configuring bus locking protocol degrade not only the
performance of one CPU but overall system performance.

If operand is cacheable and completely contained in one cache line, atomic
operation is optimized by less expensive cache locking on Intel P6 and
recent processors. If split lock is detected and the two cache lines in the
operand can be merged into one cache line, cache locking instead of
more expensive bus locking will be used for atomic operation. Removing
split lock can improve overall performance.

Instructions that may cause split lock issue include lock add, lock btc,
xchg, lsl, far call, ltr, etc.

More information about split lock, bus locking, and cache locking can be
found in the latest Intel 64 and IA-32 Architecture Software Developer's
Manual.

==#AC for split lock==

Currently we can trace split lock event counter sq_misc.split_lock
for debug purpose. But for system deployed in the field, this event
counter after the fact is insufficient. We need a mechanism that
detects split lock before it happens to ensure that bus lock is never
incurred due to split lock.

Intel introduces a mechanism to detect split lock via Alignment Check
(#AC) exception before badly aligned atomic instructions might impact
whole system performance in Tremont and other future processors. 

This capability is critical for real time system designers who build
consolidated real time systems. These systems run hard real time
code on some cores and run "untrusted" user processes on some
other cores. The hard real time cannot afford to have any bus lock from
the untrusted processes to hurt real time performance. To date the
designers have been unable to deploy these solutions as they have no
way to prevent the "untrusted" user code from generating split lock and
bus lock to block the hard real time code to access memory during bus
locking.

This capability may also find usage in cloud. A user process with split
lock running in one guest can block other cores from accessing shared
memory during its split locked memory access. That may cause overall
system performance degradation.

Split lock may open a security hole where malicious user code may slow
down overall system by executing instructions with split lock.

==Enable #AC for Split Lock==

A control bit (bit 29) in TEST_CTL MSR 0x33 will be introduced in
future x86 processors. When the bit 29 is set, the processor causes
#AC exception for split locked accesses at all CPL.

User uses "setcpuid=ac_split_lock" or "setcpuid=255" to enable the feature.

Please note: The feature could be enumerated through
MSR IA32_CORE_CAPABILITY (0xCF). But the enumeration is not completely
published yet. So this patch set doesn't include the method.

The bit 29 specification in MSR TEST_CTL is published in the latest
Intel Architecture Instruction Set Extensions and Future Features
Programming Reference.

==Handle Split Lock===

There may be different considerations to handle split lock, e.g. how
to handle split lock issue in firmware after kernel enables #AC for
split lock.

But we use a simple way to handle split lock which is suggested
by Thomas Gleixner and Dave Hansen:

- If split lock happens in kernel, a warning is issued and #AC for
split lock is disabled on the local CPU. The split lock issue should
be fixed in kernel.

- If split lock happens in user process, the process is killed by
SIGBUS. Unless the issue is fixed, the process cannot run in the
system.

- If split lock happens in firmware, system may hang in firmware. The
issue should be fixed in firmware.

- User can force to enable the feature if he knows the feature is
available.

==Patches==
Patch 1-4: Fix a few split lock issues.
Patch 5-7: Extend kernel option "clearcpuid=" to support multiple
options and CPU cap flags.
Patch 8: Add a new kernel option "setcpuid=" to force enable features.
Patch 9-10: Handle split lock

==Changelog==
v3:
- Handle split lock as suggested by Thomas Gleixner.
- Fix a few potential spit lock issues suggested by Thomas Gleixner.
- Support kernel option "setcpuid=" suggested by Dave Hanson and Thomas
Gleixner.
- Support flag string in "clearcpuid=" suggested by Dave Hanson and
Thomas Gleixner.

v2:
- Remove code that handles split lock issue in firmware and fix
x86_capability issue mainly based on comments from Thomas Gleixner and
Peter Zijlstra.

In previous version:
Comments from Dave Hansen:
- Enumerate feature in X86_FEATURE_SPLIT_LOCK_AC
- Separate #AC handler from do_error_trap
- Use CONFIG to configure inherit BIOS setting, enable, or disable split
  lock. Remove kernel parameter "split_lock_ac="
- Change config interface to debugfs from sysfs
- Fix a few bisectable issues
- Other changes.

Comment from Tony Luck and Dave Hansen:
- Dump right information in #AC handler

Comment from Alan Cox and Dave Hansen:
- Description of split lock in patch 0

Others:
- Remove tracing because we can trace split lock in existing
  sq_misc.split_lock.
- Add CONFIG to configure either panic or re-execute faulting instruction
  for split lock in kernel.
- other minor changes.

Fenghua Yu (10):
  x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long
  drivers/net/b44: Align pwol_mask to unsigned long for better
    performance
  wlcore: Align reg_ch_conf_pending and tmp_ch_bitmap to unsigned long
    for better performance
  x86/split_lock: Align x86_capability to unsigned long to avoid split
    locked access
  x86/clearcpuid: Support multiple clearcpuid options
  x86/clearcpuid: Support feature flag string in kernel option
    clearcpuid
  Change document for kernel option clearcpuid
  x86/setcpuid: Add kernel option setcpuid
  x86/split_lock: Define #AC for split lock feature
  x86/split_lock: Handle #AC exception for split lock

 Documentation/admin-guide/kernel-parameters.txt | 21 +++++++-
 arch/x86/include/asm/cmdline.h                  |  3 ++
 arch/x86/include/asm/cpu.h                      |  2 +
 arch/x86/include/asm/cpufeature.h               |  2 +
 arch/x86/include/asm/cpufeatures.h              |  1 +
 arch/x86/include/asm/msr-index.h                |  4 ++
 arch/x86/include/asm/processor.h                |  4 +-
 arch/x86/kernel/cpu/common.c                    |  5 +-
 arch/x86/kernel/cpu/cpuid-deps.c                | 68 +++++++++++++++++++++----
 arch/x86/kernel/cpu/intel.c                     | 25 +++++++++
 arch/x86/kernel/fpu/init.c                      | 40 +++++++++++----
 arch/x86/kernel/setup.c                         |  3 ++
 arch/x86/kernel/smpboot.c                       |  3 ++
 arch/x86/kernel/traps.c                         | 30 ++++++++++-
 arch/x86/lib/cmdline.c                          | 17 ++++++-
 drivers/net/ethernet/broadcom/b44.c             |  3 +-
 drivers/net/wireless/ti/wlcore/cmd.c            |  3 +-
 drivers/net/wireless/ti/wlcore/wlcore.h         |  6 ++-
 18 files changed, 208 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH v3 01/10] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long

[Fenghua Yu]

cpu_caps_cleared and cpu_caps_set may not be aligned to unsigned long.
Atomic operations (i.e. set_bit and clear_bit) on the bitmaps may access
two cache lines (a.k.a. split lock) and lock bus to block all memory
accesses from other processors to ensure atomicity.

To avoid the overall performance degradation from the bus locking, align
the two variables to unsigned long.

Defining the variables as unsigned long may also fix the issue because
they are naturally aligned to unsigned long. But that needs additional
code changes. Adding __aligned(unsigned long) are simpler fixes.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cb28e98a0659..51ab37ba5f64 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -488,8 +488,9 @@ static const char *table_lookup_model(struct cpuinfo_x86 *c)
 	return NULL;		/* Not found */
 }
 
-__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS];
-__u32 cpu_caps_set[NCAPINTS + NBUGINTS];
+/* Unsigned long alignment to avoid split lock in atomic bitmap ops */
+__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
+__u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
 
 void load_percpu_segment(int cpu)
 {
-- 
2.7.4

[PATCH v3 02/10] drivers/net/b44: Align pwol_mask to unsigned long for better performance

[Fenghua Yu]

A bit in pwol_mask is set in b44_magic_pattern automatically by set_bit.
set_bit sets the bit in a single unsigned long location. Since pwol_mask
may not be aligned to unsigned long, the location may cross two cache
lines and accessing the location degradates performance. On x86, accessing
two cache lines in locked instruction in set_bit is called split lock and
can cause overall performance degradation.

To avoid to impact performance by accessing two cache lines in set_bit,
align pwol_mask to unsigned long.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 drivers/net/ethernet/broadcom/b44.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 97ab0dd25552..bc544b6b9c3a 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -1547,7 +1547,8 @@ static void b44_setup_pseudo_magicp(struct b44 *bp)
 	u32 val;
 	int plen0, plen1, plen2;
 	u8 *pwol_pattern;
-	u8 pwol_mask[B44_PMASK_SIZE];
+	/* Align to unsigned long for better performance in set_bit() */
+	u8 pwol_mask[B44_PMASK_SIZE] __aligned(sizeof(unsigned long));
 
 	pwol_pattern = kzalloc(B44_PATTERN_SIZE, GFP_KERNEL);
 	if (!pwol_pattern)
-- 
2.7.4

[PATCH v3 03/10] wlcore: Align reg_ch_conf_pending and tmp_ch_bitmap to unsigned long for better performance

[Fenghua Yu]

A bit in reg_ch_conf_pending in wl271 and tmp_ch_bitmap is set atomically
by set_bit(). set_bit() sets the bit in a single unsigned long location. If
the variables are not aligned to unsigned long, set_bit() accesses two
cache lines and thus causes slower performance. On x86, this scenario is
called split lock and can cause overall performance degradation due to
locked BTSL instruction in set_bit() locks bus.

To avoid performance degradation, the two variables are aligned to
unsigned long.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 drivers/net/wireless/ti/wlcore/cmd.c    | 3 ++-
 drivers/net/wireless/ti/wlcore/wlcore.h | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c
index 903968735a74..8d15a6307d44 100644
--- a/drivers/net/wireless/ti/wlcore/cmd.c
+++ b/drivers/net/wireless/ti/wlcore/cmd.c
@@ -1707,7 +1707,8 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
 {
 	struct wl12xx_cmd_regdomain_dfs_config *cmd = NULL;
 	int ret = 0, i, b, ch_bit_idx;
-	u32 tmp_ch_bitmap[2];
+	/* Align to unsigned long for better performance in set_bit() */
+	u32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));
 	struct wiphy *wiphy = wl->hw->wiphy;
 	struct ieee80211_supported_band *band;
 	bool timeout = false;
diff --git a/drivers/net/wireless/ti/wlcore/wlcore.h b/drivers/net/wireless/ti/wlcore/wlcore.h
index dd14850b0603..92d878f01fa5 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore.h
@@ -321,8 +321,10 @@ struct wl1271 {
 
 	/* Reg domain last configuration */
 	u32 reg_ch_conf_last[2]  __aligned(8);
-	/* Reg domain pending configuration */
-	u32 reg_ch_conf_pending[2];
+	/* Reg domain pending configuration. Aligned to unsigned long for
+	 * better performane in set_bit().
+	 */
+	u32 reg_ch_conf_pending[2] __aligned(sizeof(unsigned long));
 
 	/* Pointer that holds DMA-friendly block for the mailbox */
 	void *mbox;
-- 
2.7.4

[PATCH v3 04/10] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access

[Fenghua Yu]

set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
operate on bitmap defined in x86_capability.

Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
the location is at:
base address of x86_capability + (bit offset in x86_capability % 64) * 8

Since base address of x86_capability may not aligned to unsigned long,
the single unsigned long location may cross two cache lines and
accessing the location by locked BTS/BTR introductions will trigger #AC.

To fix the split lock issue, align x86_capability to unsigned long so that
the location will be always within one cache line.

Changing x86_capability[]'s type to unsigned long may also fix the issue
because x86_capability[] will be naturally aligned to unsigned long. But
this needs additional code changes. So we choose the simpler solution
by enforcing alignment using __aligned(unsigned long).

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/processor.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 33051436c864..eb8ae701ef65 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -93,7 +93,9 @@ struct cpuinfo_x86 {
 	__u32			extended_cpuid_level;
 	/* Maximum supported CPUID level, -1=no CPUID: */
 	int			cpuid_level;
-	__u32			x86_capability[NCAPINTS + NBUGINTS];
+	/* Unsigned long alignment to avoid split lock in atomic bitmap ops */
+	__u32			x86_capability[NCAPINTS + NBUGINTS]
+				__aligned(sizeof(unsigned long));
 	char			x86_vendor_id[16];
 	char			x86_model_id[64];
 	/* in KB - valid for CPUS which support this call: */
-- 
2.7.4

[PATCH v3 05/10] x86/clearcpuid: Support multiple clearcpuid options

[Fenghua Yu]

Currently only one kernel option "clearcpuid=" can be picked up by
kernel during boot time.

In some cases, user may want to clear a few cpu caps. This may be
useful to replace endless (new) kernel options like nosmep, nosmap,
etc.

We add support of multiple clearcpuid options to allow user to clear
multiple cpu caps.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cmdline.h |  3 +++
 arch/x86/kernel/fpu/init.c     | 35 +++++++++++++++++++++++++----------
 arch/x86/lib/cmdline.c         | 17 +++++++++++++++--
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/cmdline.h b/arch/x86/include/asm/cmdline.h
index 6faaf27e8899..059e29558bb3 100644
--- a/arch/x86/include/asm/cmdline.h
+++ b/arch/x86/include/asm/cmdline.h
@@ -5,5 +5,8 @@
 int cmdline_find_option_bool(const char *cmdline_ptr, const char *option);
 int cmdline_find_option(const char *cmdline_ptr, const char *option,
 			char *buffer, int bufsize);
+int cmdline_find_option_in_range(const char *cmdline_ptr, int cmdline_size,
+				 const char *option, char *buffer, int bufsize,
+				 char **arg_pos);
 
 #endif /* _ASM_X86_CMDLINE_H */
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 6abd83572b01..da92f7f340c1 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -243,16 +243,36 @@ static void __init fpu__init_system_ctx_switch(void)
 	WARN_ON_FPU(current->thread.fpu.initialized);
 }
 
+static void __init setup_cpuid_option(char *setup_cpuid_option)
+{
+	void (*setup_cpu_cap)(unsigned int feature);
+	char arg[32], *argptr, *option_pos;
+	int bit, cmdline_size;
+
+	if (!strcmp(setup_cpuid_option, "clearcpuid"))
+		setup_cpu_cap = setup_clear_cpu_cap;
+		return;
+
+	/* Find each option in boot_command_line and clear specified cpu cap. */
+	cmdline_size = COMMAND_LINE_SIZE;
+	while (cmdline_find_option_in_range(boot_command_line, cmdline_size,
+					    setup_cpuid_option, arg,
+					    sizeof(arg), &option_pos) >= 0) {
+		/* Chang command line range for next search. */
+		cmdline_size = option_pos - boot_command_line + 1;
+		argptr = arg;
+		if (get_option(&argptr, &bit) &&
+		    bit >= 0 && bit < NCAPINTS * 32)
+			setup_cpu_cap(bit);
+	}
+}
+
 /*
  * We parse fpu parameters early because fpu__init_system() is executed
  * before parse_early_param().
  */
 static void __init fpu__init_parse_early_param(void)
 {
-	char arg[32];
-	char *argptr = arg;
-	int bit;
-
 	if (cmdline_find_option_bool(boot_command_line, "no387"))
 		setup_clear_cpu_cap(X86_FEATURE_FPU);
 
@@ -271,12 +291,7 @@ static void __init fpu__init_parse_early_param(void)
 	if (cmdline_find_option_bool(boot_command_line, "noxsaves"))
 		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 
-	if (cmdline_find_option(boot_command_line, "clearcpuid", arg,
-				sizeof(arg)) &&
-	    get_option(&argptr, &bit) &&
-	    bit >= 0 &&
-	    bit < NCAPINTS * 32)
-		setup_clear_cpu_cap(bit);
+	setup_cpuid_option("clearcpuid");
 }
 
 /*
diff --git a/arch/x86/lib/cmdline.c b/arch/x86/lib/cmdline.c
index 3261abb21ef4..9cf1a0773877 100644
--- a/arch/x86/lib/cmdline.c
+++ b/arch/x86/lib/cmdline.c
@@ -114,13 +114,15 @@ __cmdline_find_option_bool(const char *cmdline, int max_cmdline_size,
  * @option: option string to look for
  * @buffer: memory buffer to return the option argument
  * @bufsize: size of the supplied memory buffer
+ * @option_pos: pointer to the option if found
  *
  * Returns the length of the argument (regardless of if it was
  * truncated to fit in the buffer), or -1 on not found.
  */
 static int
 __cmdline_find_option(const char *cmdline, int max_cmdline_size,
-		      const char *option, char *buffer, int bufsize)
+		      const char *option, char *buffer, int bufsize,
+		      char **arg_pos)
 {
 	char c;
 	int pos = 0, len = -1;
@@ -164,6 +166,9 @@ __cmdline_find_option(const char *cmdline, int max_cmdline_size,
 				len = 0;
 				bufptr = buffer;
 				state = st_bufcpy;
+				if (arg_pos)
+					*arg_pos = (char *)cmdline -
+						   strlen(option) - 1;
 				break;
 			} else if (c == *opptr++) {
 				/*
@@ -211,5 +216,13 @@ int cmdline_find_option(const char *cmdline, const char *option, char *buffer,
 			int bufsize)
 {
 	return __cmdline_find_option(cmdline, COMMAND_LINE_SIZE, option,
-				     buffer, bufsize);
+				     buffer, bufsize, NULL);
+}
+
+int cmdline_find_option_in_range(const char *cmdline, int cmdline_size,
+				 char *option, char *buffer, int bufsize,
+				 char **arg_pos)
+{
+	return __cmdline_find_option(cmdline, cmdline_size, option, buffer,
+				     bufsize, arg_pos);
 }
-- 
2.7.4

[PATCH v3 06/10] x86/clearcpuid: Support feature flag string in kernel option clearcpuid

[Fenghua Yu]

The kernel option clearcpuid currently only takes feature bit which
can be changed from kernel to kernel.

Extend clearcpuid to use cap flag string, which is defined in
x86_cap_flags[] and won't be changed from kernel to kernel.
And user can easily get the cap flag string from /proc/cpuinfo.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/cpuid-deps.c  | 26 ++++++++++++++++++++++++++
 arch/x86/kernel/fpu/init.c        |  3 ++-
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index ce95b8cbd229..6792088525e3 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -132,6 +132,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 
 extern void setup_clear_cpu_cap(unsigned int bit);
 extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
+bool find_cpu_cap(char *cap_flag, unsigned int *pfeature);
 
 #define setup_force_cpu_cap(bit) do { \
 	set_cpu_cap(&boot_cpu_data, bit);	\
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 2c0bd38a44ab..19bfab0858ac 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -119,3 +119,29 @@ void setup_clear_cpu_cap(unsigned int feature)
 {
 	do_clear_cpu_cap(NULL, feature);
 }
+
+/**
+ * find_cpu_cap - Given a cap flag string, find its corresponding feature bit.
+ * @cap_flag:	cap flag string as defined in x86_cap_flags[]
+ * @pfeature:	feature bit
+ *
+ * Return: true if the feature is found. false if not found
+ */
+bool find_cpu_cap(char *cap_flag, unsigned int *pfeature)
+{
+#ifdef CONFIG_X86_FEATURE_NAMES
+	unsigned int feature;
+
+	for (feature = 0; feature < NCAPINTS * 32; feature++) {
+		if (!x86_cap_flags[feature])
+			continue;
+
+		if (strcmp(cap_flag, x86_cap_flags[feature]) == 0) {
+			*pfeature = feature;
+
+			return true;
+		}
+	}
+#endif
+	return false;
+}
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index da92f7f340c1..a9a67645a607 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -261,7 +261,8 @@ static void __init setup_cpuid_option(char *setup_cpuid_option)
 		/* Chang command line range for next search. */
 		cmdline_size = option_pos - boot_command_line + 1;
 		argptr = arg;
-		if (get_option(&argptr, &bit) &&
+		/* cpu cap can be specified by either feature bit or string */
+		if ((get_option(&argptr, &bit) || find_cpu_cap(arg, &bit)) &&
 		    bit >= 0 && bit < NCAPINTS * 32)
 			setup_cpu_cap(bit);
 	}
-- 
2.7.4

[PATCH v3 07/10] Change document for kernel option clearcpuid

[Fenghua Yu]

Since kernel option clearcpuid now supports multiple options and CPU
capability flags, the document needs to be changed.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b799bcf67d7b..13bf223c7739 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -558,10 +558,12 @@
 			loops can be debugged more effectively on production
 			systems.
 
-	clearcpuid=BITNUM [X86]
+	clearcpuid=BITNUM | FLAG [X86]
 			Disable CPUID feature X for the kernel. See
 			arch/x86/include/asm/cpufeatures.h for the valid bit
-			numbers. Note the Linux specific bits are not necessarily
+			numbers or /proc/cpuinfo for valid CPU flags.
+			Multiple options can be used to disable a few features.
+			Note the Linux specific bits are not necessarily
 			stable over kernel options, but the vendor specific
 			ones should be.
 			Also note that user programs calling CPUID directly
-- 
2.7.4

[PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Fenghua Yu]

On some platforms, a feature (e.g. #AC for split lock) may not be
enumerated by CPUID or non architectural way in IA32_CORE_CAPABILITY.
To enable the feature on the platforms, a new kernel option setcpuid
is added.

The feature is defined in cpufeatures.h. The kernel option setcpuid
takes either feature bit or cpu cap flag corresponding to the feature.
The format of the option:
setcpuid=<feature bit>
setcpuid=<feature capability flag>

Check cpufeatures.h for valid feature bit numbers and check capflags.c
or /proc/cpuinfo for valid feature capability flags.

Enabling multiple features are supported.

Please note kernel may malfunction if some features are enabled by
the option.

This option behaves like existing kernel option clearcpuid.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 15 +++++++++
 arch/x86/include/asm/cpufeature.h               |  1 +
 arch/x86/kernel/cpu/cpuid-deps.c                | 42 +++++++++++++++++++------
 arch/x86/kernel/fpu/init.c                      |  4 +++
 4 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 13bf223c7739..5de364078c69 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4083,6 +4083,21 @@
 			incurs a small amount of overhead in the scheduler
 			but is useful for debugging and performance tuning.
 
+	setcpuid=BITNUM | FLAG [X86]
+			Enable CPUID feature X for the kernel. See
+			arch/x86/include/asm/cpufeatures.h for the valid bit
+			numbers or /proc/cpuinfo for the valid CPU flags.
+			Multiple options can be used to enable a few features.
+			Note the Linux specific bits are not necessarily
+			stable over kernel options, but the vendor specific
+			ones should be.
+			Also note that user programs calling CPUID directly
+			or using the feature without checking anything
+			may still disable it.
+			Also note the kernel might malfunction if you enable
+			some critical bits.
+			Please refer to clearcpuid for disabling CPUID feature.
+
 	skew_tick=	[KNL] Offset the periodic timer tick per cpu to mitigate
 			xtime_lock contention on larger systems, and/or RCU lock
 			contention on all systems with CONFIG_MAXSMP set.
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 6792088525e3..6b580687f261 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -131,6 +131,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 #define set_cpu_cap(c, bit)	set_bit(bit, (unsigned long *)((c)->x86_capability))
 
 extern void setup_clear_cpu_cap(unsigned int bit);
+void setup_set_cpu_cap(unsigned int bit);
 extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
 bool find_cpu_cap(char *cap_flag, unsigned int *pfeature);
 
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 19bfab0858ac..ae534b5d0b4d 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -62,25 +62,39 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{}
 };
 
-static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
+static inline void
+setup_feature(struct cpuinfo_x86 *c, unsigned int feature, bool enable)
 {
 	/*
 	 * Note: This could use the non atomic __*_bit() variants, but the
 	 * rest of the cpufeature code uses atomics as well, so keep it for
 	 * consistency. Cleanup all of it separately.
 	 */
-	if (!c) {
-		clear_cpu_cap(&boot_cpu_data, feature);
-		set_bit(feature, (unsigned long *)cpu_caps_cleared);
+	if (enable) {
+		/* Set the feature */
+		if (!c) {
+			set_cpu_cap(&boot_cpu_data, feature);
+			clear_bit(feature, (unsigned long *)cpu_caps_cleared);
+			setup_force_cpu_cap(feature);
+		} else {
+			set_bit(feature, (unsigned long *)c->x86_capability);
+		}
 	} else {
-		clear_bit(feature, (unsigned long *)c->x86_capability);
+		/* Clear the feature */
+		if (!c) {
+			clear_cpu_cap(&boot_cpu_data, feature);
+			set_bit(feature, (unsigned long *)cpu_caps_cleared);
+		} else {
+			clear_bit(feature, (unsigned long *)c->x86_capability);
+		}
 	}
 }
 
 /* Take the capabilities and the BUG bits into account */
 #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)
 
-static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
+static void
+do_setup_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature, bool enable)
 {
 	DECLARE_BITMAP(disable, MAX_FEATURE_BITS);
 	const struct cpuid_dep *d;
@@ -89,7 +103,7 @@ static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 	if (WARN_ON(feature >= MAX_FEATURE_BITS))
 		return;
 
-	clear_feature(c, feature);
+	setup_feature(c, feature, enable);
 
 	/* Collect all features to disable, handling dependencies */
 	memset(disable, 0, sizeof(disable));
@@ -105,19 +119,27 @@ static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 				continue;
 
 			changed = true;
-			clear_feature(c, d->feature);
+			setup_feature(c, d->feature, enable);
 		}
 	} while (changed);
 }
 
 void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 {
-	do_clear_cpu_cap(c, feature);
+	/* Disable the feature. */
+	do_setup_cpu_cap(c, feature, false);
 }
 
 void setup_clear_cpu_cap(unsigned int feature)
 {
-	do_clear_cpu_cap(NULL, feature);
+	/* Disable the feature. */
+	do_setup_cpu_cap(NULL, feature, false);
+}
+
+void setup_set_cpu_cap(unsigned int feature)
+{
+	/* Enable the feature. */
+	do_setup_cpu_cap(NULL, feature, true);
 }
 
 /**
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index a9a67645a607..cb1e2f8129a5 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -251,6 +251,9 @@ static void __init setup_cpuid_option(char *setup_cpuid_option)
 
 	if (!strcmp(setup_cpuid_option, "clearcpuid"))
 		setup_cpu_cap = setup_clear_cpu_cap;
+	else if (!strcmp(setup_cpuid_option, "setcpuid"))
+		setup_cpu_cap = setup_set_cpu_cap;
+	else
 		return;
 
 	/* Find each option in boot_command_line and clear specified cpu cap. */
@@ -293,6 +296,7 @@ static void __init fpu__init_parse_early_param(void)
 		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 
 	setup_cpuid_option("clearcpuid");
+	setup_cpuid_option("setcpuid");
 }
 
 /*
-- 
2.7.4

[PATCH v3 09/10] x86/split_lock: Define #AC for split lock feature

[Fenghua Yu]

The feature is Linux specific. If the feature is enumerated or
enabled by "setcpuid=ac_split_lock", the bit is set.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 6d6122524711..125a490e4182 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -221,6 +221,7 @@
 #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
 #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
 #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
+#define X86_FEATURE_AC_SPLIT_LOCK	( 7*32+31) /* #AC for split lock */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */
-- 
2.7.4

[PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[Fenghua Yu]

There may be different considerations on how to handle #AC for split lock,
e.g. how to handle system hang caused by split lock issue in firmware,
how to emulate faulting instruction, etc. We use a simple method to
handle user and kernel split lock and may extend the method in the future.

When #AC exception for split lock is triggered from user process, the
process is killed by SIGBUS. To execute the process properly, user
application developer needs to fix the split lock issue.

When #AC exception for split lock is triggered from a kernel instruction,
disable #AC for split lock on local CPU and warn the split lock issue.
After the exception, the faulting instruction will be executed and kernel
execution continues. #AC for split lock is only disabled on the local CPU
not globally. It will be re-enabled if the CPU is offline and then online.

Kernel developer should check the warning, which contains helpful faulting
address, context, and callstack info, and fix the split lock issue
one by one. Then further split lock may be captured and fixed.

After bit 29 in MSR_TEST_CTL is set as one in kernel, firmware inherits
the setting when firmware is executed in S4, S5, run time services, SMI,
etc. Split lock issue in firmware triggers #AC and may hang the system
depending on how firmware handles the #AC. It's up to firmware developer
to fix the split lock issues in firmware.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpu.h       |  2 ++
 arch/x86/include/asm/msr-index.h |  4 ++++
 arch/x86/kernel/cpu/intel.c      | 25 +++++++++++++++++++++++++
 arch/x86/kernel/setup.c          |  3 +++
 arch/x86/kernel/smpboot.c        |  3 +++
 arch/x86/kernel/traps.c          | 30 +++++++++++++++++++++++++++++-
 6 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index adc6cc86b062..21792b295a61 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -40,4 +40,6 @@ int mwait_usable(const struct cpuinfo_x86 *);
 unsigned int x86_family(unsigned int sig);
 unsigned int x86_model(unsigned int sig);
 unsigned int x86_stepping(unsigned int sig);
+void set_ac_split_lock(void);
+bool do_ac_split_lock(struct pt_regs *regs);
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 8e40c2446fd1..74cf8a0f1512 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -39,6 +39,10 @@
 
 /* Intel MSRs. Some also available on other CPUs */
 
+#define MSR_TEST_CTL				0x00000033
+#define TEST_CTL_ENABLE_AC_SPLIT_LOCK_SHIFT	29
+#define TEST_CTL_ENABLE_AC_SPLIT_LOCK		BIT(29)
+
 #define MSR_IA32_SPEC_CTRL		0x00000048 /* Speculation Control */
 #define SPEC_CTRL_IBRS			(1 << 0)   /* Indirect Branch Restricted Speculation */
 #define SPEC_CTRL_STIBP_SHIFT		1	   /* Single Thread Indirect Branch Predictor (STIBP) bit */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fc3c07fe7df5..d813fd64ecef 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1029,3 +1029,28 @@ static const struct cpu_dev intel_cpu_dev = {
 
 cpu_dev_register(intel_cpu_dev);
 
+/* #AC handler for split lock is called by generic #AC handler. */
+bool do_ac_split_lock(struct pt_regs *regs)
+{
+	/* Generic #AC handler will handle split lock in user. */
+	if (user_mode(regs))
+		return false;
+
+	/*
+	 * On split lock in kernel, warn and disable #AC for split lock on
+	 * current CPU.
+	 */
+	msr_clear_bit(MSR_TEST_CTL, TEST_CTL_ENABLE_AC_SPLIT_LOCK_SHIFT);
+
+	WARN_ONCE(1, "A split lock issue is detected.\n");
+
+	return true;
+}
+
+void set_ac_split_lock(void)
+{
+	if (boot_cpu_has(X86_FEATURE_AC_SPLIT_LOCK)) {
+		msr_set_bit(MSR_TEST_CTL, TEST_CTL_ENABLE_AC_SPLIT_LOCK_SHIFT);
+		pr_info_once("#AC for split lock is enabled\n");
+	}
+}
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 3d872a527cd9..7517debc8f61 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -961,6 +961,9 @@ void __init setup_arch(char **cmdline_p)
 
 	parse_early_param();
 
+	/* Set up #AC for split lock at the earliest phase. */
+	set_ac_split_lock();
+
 	if (efi_enabled(EFI_BOOT))
 		efi_memblock_x86_reserve_range();
 #ifdef CONFIG_MEMORY_HOTPLUG
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ccd1f2a8e557..3ef843045564 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -231,6 +231,9 @@ static void notrace start_secondary(void *unused)
 #endif
 	load_current_idt();
 	cpu_init();
+
+	set_ac_split_lock();
+
 	x86_cpuinit.early_percpu_clock_init();
 	preempt_disable();
 	smp_callin();
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9b7c4ca8f0a7..f3cadf2d177f 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -61,6 +61,7 @@
 #include <asm/mpx.h>
 #include <asm/vm86.h>
 #include <asm/umip.h>
+#include <asm/cpu.h>
 
 #ifdef CONFIG_X86_64
 #include <asm/x86_init.h>
@@ -292,9 +293,36 @@ DO_ERROR(X86_TRAP_OLD_MF, SIGFPE,           0, NULL, "coprocessor segment overru
 DO_ERROR(X86_TRAP_TS,     SIGSEGV,          0, NULL, "invalid TSS",         invalid_TSS)
 DO_ERROR(X86_TRAP_NP,     SIGBUS,           0, NULL, "segment not present", segment_not_present)
 DO_ERROR(X86_TRAP_SS,     SIGBUS,           0, NULL, "stack segment",       stack_segment)
-DO_ERROR(X86_TRAP_AC,     SIGBUS,  BUS_ADRALN, NULL, "alignment check",     alignment_check)
 #undef IP
 
+dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
+{
+	unsigned int trapnr = X86_TRAP_AC;
+	char str[] = "alignment check";
+	int signr = SIGBUS;
+	int ret;
+
+	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
+
+	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
+			NOTIFY_STOP) {
+		/* #AC exception could be handled by split lock handler. */
+		ret = do_ac_split_lock(regs);
+		if (ret) {
+			cond_local_irq_enable(regs);
+
+			return;
+		}
+
+		cond_local_irq_enable(regs);
+		/*
+		 * If not processed by split lock handler, go to generic
+		 * #AC handler.
+		 */
+		do_trap(trapnr, signr, str, regs, error_code, BUS_ADRALN, NULL);
+	}
+}
+
 #ifdef CONFIG_VMAP_STACK
 __visible void __noreturn handle_stack_overflow(const char *message,
 						struct pt_regs *regs,
-- 
2.7.4

Re: [PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[kbuild test robot]

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

Hi Fenghua,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fenghua-Yu/x86-split_lock-Enable-AC-exception-for-split-locked-accesses/20190204-162843
config: i386-randconfig-a2-02040849 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   ld: arch/x86/kernel/traps.o: in function `do_alignment_check':
   arch/x86/kernel/traps.c:310: undefined reference to `do_ac_split_lock'
   ld: arch/x86/kernel/setup.o: in function `setup_arch':
>> arch/x86/kernel/setup.c:965: undefined reference to `set_ac_split_lock'

vim +965 arch/x86/kernel/setup.c

   949	
   950		strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
   951		*cmdline_p = command_line;
   952	
   953		/*
   954		 * x86_configure_nx() is called before parse_early_param() to detect
   955		 * whether hardware doesn't support NX (so that the early EHCI debug
   956		 * console setup can safely call set_fixmap()). It may then be called
   957		 * again from within noexec_setup() during parsing early parameters
   958		 * to honor the respective command line option.
   959		 */
   960		x86_configure_nx();
   961	
   962		parse_early_param();
   963	
   964		/* Set up #AC for split lock at the earliest phase. */
 > 965		set_ac_split_lock();
   966	
   967		if (efi_enabled(EFI_BOOT))
   968			efi_memblock_x86_reserve_range();
   969	#ifdef CONFIG_MEMORY_HOTPLUG
   970		/*
   971		 * Memory used by the kernel cannot be hot-removed because Linux
   972		 * cannot migrate the kernel pages. When memory hotplug is
   973		 * enabled, we should prevent memblock from allocating memory
   974		 * for the kernel.
   975		 *
   976		 * ACPI SRAT records all hotpluggable memory ranges. But before
   977		 * SRAT is parsed, we don't know about it.
   978		 *
   979		 * The kernel image is loaded into memory at very early time. We
   980		 * cannot prevent this anyway. So on NUMA system, we set any
   981		 * node the kernel resides in as un-hotpluggable.
   982		 *
   983		 * Since on modern servers, one node could have double-digit
   984		 * gigabytes memory, we can assume the memory around the kernel
   985		 * image is also un-hotpluggable. So before SRAT is parsed, just
   986		 * allocate memory near the kernel image to try the best to keep
   987		 * the kernel away from hotpluggable memory.
   988		 */
   989		if (movable_node_is_enabled())
   990			memblock_set_bottom_up(true);
   991	#endif
   992	
   993		x86_report_nx();
   994	
   995		/* after early param, so could get panic from serial */
   996		memblock_x86_reserve_range_setup_data();
   997	
   998		if (acpi_mps_check()) {
   999	#ifdef CONFIG_X86_LOCAL_APIC
  1000			disable_apic = 1;
  1001	#endif
  1002			setup_clear_cpu_cap(X86_FEATURE_APIC);
  1003		}
  1004	
  1005		e820__reserve_setup_data();
  1006		e820__finish_early_params();
  1007	
  1008		if (efi_enabled(EFI_BOOT))
  1009			efi_init();
  1010	
  1011		dmi_scan_machine();
  1012		dmi_memdev_walk();
  1013		dmi_set_dump_stack_arch_desc();
  1014	
  1015		/*
  1016		 * VMware detection requires dmi to be available, so this
  1017		 * needs to be done after dmi_scan_machine(), for the boot CPU.
  1018		 */
  1019		init_hypervisor_platform();
  1020	
  1021		tsc_early_init();
  1022		x86_init.resources.probe_roms();
  1023	
  1024		/* after parse_early_param, so could debug it */
  1025		insert_resource(&iomem_resource, &code_resource);
  1026		insert_resource(&iomem_resource, &data_resource);
  1027		insert_resource(&iomem_resource, &bss_resource);
  1028	
  1029		e820_add_kernel_range();
  1030		trim_bios_range();
  1031	#ifdef CONFIG_X86_32
  1032		if (ppro_with_ram_bug()) {
  1033			e820__range_update(0x70000000ULL, 0x40000ULL, E820_TYPE_RAM,
  1034					  E820_TYPE_RESERVED);
  1035			e820__update_table(e820_table);
  1036			printk(KERN_INFO "fixed physical RAM map:\n");
  1037			e820__print_table("bad_ppro");
  1038		}
  1039	#else
  1040		early_gart_iommu_check();
  1041	#endif
  1042	
  1043		/*
  1044		 * partially used pages are not usable - thus
  1045		 * we are rounding upwards:
  1046		 */
  1047		max_pfn = e820__end_of_ram_pfn();
  1048	
  1049		/* update e820 for memory not covered by WB MTRRs */
  1050		mtrr_bp_init();
  1051		if (mtrr_trim_uncached_memory(max_pfn))
  1052			max_pfn = e820__end_of_ram_pfn();
  1053	
  1054		max_possible_pfn = max_pfn;
  1055	
  1056		/*
  1057		 * This call is required when the CPU does not support PAT. If
  1058		 * mtrr_bp_init() invoked it already via pat_init() the call has no
  1059		 * effect.
  1060		 */
  1061		init_cache_modes();
  1062	
  1063		/*
  1064		 * Define random base addresses for memory sections after max_pfn is
  1065		 * defined and before each memory section base is used.
  1066		 */
  1067		kernel_randomize_memory();
  1068	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25142 bytes --]

Re: [PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[kbuild test robot]

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

Hi Fenghua,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v5.0-rc4 next-20190204]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fenghua-Yu/x86-split_lock-Enable-AC-exception-for-split-locked-accesses/20190204-162843
config: x86_64-randconfig-k3-02041323 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   ld: arch/x86/kernel/traps.o: in function `do_alignment_check':
   arch/x86/kernel/traps.c:310: undefined reference to `do_ac_split_lock'
   ld: arch/x86/kernel/setup.o: in function `setup_arch':
   arch/x86/kernel/setup.c:965: undefined reference to `set_ac_split_lock'
   ld: arch/x86/kernel/smpboot.o: in function `start_secondary':
>> arch/x86/kernel/smpboot.c:235: undefined reference to `set_ac_split_lock'

vim +235 arch/x86/kernel/smpboot.c

   206	
   207	static int cpu0_logical_apicid;
   208	static int enable_start_cpu0;
   209	/*
   210	 * Activate a secondary processor.
   211	 */
   212	static void notrace start_secondary(void *unused)
   213	{
   214		/*
   215		 * Don't put *anything* except direct CPU state initialization
   216		 * before cpu_init(), SMP booting is too fragile that we want to
   217		 * limit the things done here to the most necessary things.
   218		 */
   219		if (boot_cpu_has(X86_FEATURE_PCID))
   220			__write_cr4(__read_cr4() | X86_CR4_PCIDE);
   221	
   222	#ifdef CONFIG_X86_32
   223		/* switch away from the initial page table */
   224		load_cr3(swapper_pg_dir);
   225		/*
   226		 * Initialize the CR4 shadow before doing anything that could
   227		 * try to read it.
   228		 */
   229		cr4_init_shadow();
   230		__flush_tlb_all();
   231	#endif
   232		load_current_idt();
   233		cpu_init();
   234	
 > 235		set_ac_split_lock();
   236	
   237		x86_cpuinit.early_percpu_clock_init();
   238		preempt_disable();
   239		smp_callin();
   240	
   241		enable_start_cpu0 = 0;
   242	
   243		/* otherwise gcc will move up smp_processor_id before the cpu_init */
   244		barrier();
   245		/*
   246		 * Check TSC synchronization with the boot CPU:
   247		 */
   248		check_tsc_sync_target();
   249	
   250		speculative_store_bypass_ht_init();
   251	
   252		/*
   253		 * Lock vector_lock, set CPU online and bring the vector
   254		 * allocator online. Online must be set with vector_lock held
   255		 * to prevent a concurrent irq setup/teardown from seeing a
   256		 * half valid vector space.
   257		 */
   258		lock_vector_lock();
   259		set_cpu_online(smp_processor_id(), true);
   260		lapic_online();
   261		unlock_vector_lock();
   262		cpu_set_state_online(smp_processor_id());
   263		x86_platform.nmi_init();
   264	
   265		/* enable local interrupts */
   266		local_irq_enable();
   267	
   268		/* to prevent fake stack check failure in clock setup */
   269		boot_init_stack_canary();
   270	
   271		x86_cpuinit.setup_percpu_clockev();
   272	
   273		wmb();
   274		cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
   275	}
   276	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30168 bytes --]

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Thomas Gleixner]

On Fri, 1 Feb 2019, Fenghua Yu wrote:

> On some platforms, a feature (e.g. #AC for split lock) may not be
> enumerated by CPUID or non architectural way in IA32_CORE_CAPABILITY.
> To enable the feature on the platforms, a new kernel option setcpuid
> is added.
> 
> The feature is defined in cpufeatures.h. The kernel option setcpuid
> takes either feature bit or cpu cap flag corresponding to the feature.
> The format of the option:
> setcpuid=<feature bit>
> setcpuid=<feature capability flag>
> 
> Check cpufeatures.h for valid feature bit numbers and check capflags.c
> or /proc/cpuinfo for valid feature capability flags.
> 
> Enabling multiple features are supported.
> 
> Please note kernel may malfunction if some features are enabled by
> the option.
> 
> This option behaves like existing kernel option clearcpuid.

No it does NOT. clearcpuid allows to disable things.

This allows to enable random CPUID bits without any sanity checking. Not
going to happen. We made it clear in the past that functionality needs to
be detectable by enumeration. We do quirks for broken crap, but this is
just not how it works.

Thanks,

	tglx

Re: [PATCH v3 09/10] x86/split_lock: Define #AC for split lock feature

[Dave Hansen]

On 2/1/19 9:14 PM, Fenghua Yu wrote:
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -221,6 +221,7 @@
>  #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
>  #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
>  #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
> +#define X86_FEATURE_AC_SPLIT_LOCK	( 7*32+31) /* #AC for split lock */

The last time this was posted, we (Intel) promised to go get the proper
(CPUID or MSR-based) enumeration of this feature documented.  Did we do
that?  If so, where is that documentation?

Re: [PATCH v3 09/10] x86/split_lock: Define #AC for split lock feature

[Fenghua Yu]

On Mon, Feb 04, 2019 at 10:41:40AM -0800, Dave Hansen wrote:
> On 2/1/19 9:14 PM, Fenghua Yu wrote:
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -221,6 +221,7 @@
> >  #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
> >  #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
> >  #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
> > +#define X86_FEATURE_AC_SPLIT_LOCK	( 7*32+31) /* #AC for split lock */
> 
> The last time this was posted, we (Intel) promised to go get the proper
> (CPUID or MSR-based) enumeration of this feature documented.  Did we do
> that?  If so, where is that documentation?

As said in the cover patch:
"Please note: The feature could be enumerated through
MSR IA32_CORE_CAPABILITY (0xCF). But the enumeration is not completely
published yet. So this patch set doesn't include the method."

In SDM and ISE, bit 5 in IA32_CORE_CAPABILITY is for enumeration for
the split lock feature. But how to enumerate IA32_CORE_CAPABILITY MSR
itself is not public yet.

That's why I didn't include the enumeration patches for IA32_CORE_CAPABILITY
and the split lock feature. The enumeration patches will be published
once the complete enumerations for both IA32_CORE_CAPABILITY and the split
lock feature is publicly released.

This patch set only uses "setcpuid=ac_split_lock" to enable the feature.

Thanks.

-Fenghua

Re: [PATCH v3 09/10] x86/split_lock: Define #AC for split lock feature

[Dave Hansen]

On 2/4/19 10:45 AM, Fenghua Yu wrote:
> On Mon, Feb 04, 2019 at 10:41:40AM -0800, Dave Hansen wrote:
>> On 2/1/19 9:14 PM, Fenghua Yu wrote:
>>> --- a/arch/x86/include/asm/cpufeatures.h
>>> +++ b/arch/x86/include/asm/cpufeatures.h
>>> @@ -221,6 +221,7 @@
>>>  #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
>>>  #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
>>>  #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
>>> +#define X86_FEATURE_AC_SPLIT_LOCK	( 7*32+31) /* #AC for split lock */
>>
>> The last time this was posted, we (Intel) promised to go get the proper
>> (CPUID or MSR-based) enumeration of this feature documented.  Did we do
>> that?  If so, where is that documentation?
> 
> As said in the cover patch:
> "Please note: The feature could be enumerated through
> MSR IA32_CORE_CAPABILITY (0xCF). But the enumeration is not completely
> published yet. So this patch set doesn't include the method."

FWIW, I think we (Intel) promised to get this documented _before_ we
came back asking our intrepid x86 maintainers to accept something like
setcpuid=.  While I generally applaud the post-early-post-often
attitude, we might have been a bit trigger-happy on this one.

This is awfully crucial information to bury in a massive cover letter,
especially when the patch description is a rather anemic two lines.

Re: [PATCH v3 09/10] x86/split_lock: Define #AC for split lock feature

[Fenghua Yu]

On Mon, Feb 04, 2019 at 11:00:03AM -0800, Dave Hansen wrote:
> On 2/4/19 10:45 AM, Fenghua Yu wrote:
> > On Mon, Feb 04, 2019 at 10:41:40AM -0800, Dave Hansen wrote:
> >> On 2/1/19 9:14 PM, Fenghua Yu wrote:
> >>> --- a/arch/x86/include/asm/cpufeatures.h
> >>> +++ b/arch/x86/include/asm/cpufeatures.h
> >>> @@ -221,6 +221,7 @@
> >>>  #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
> >>>  #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
> >>>  #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
> >>> +#define X86_FEATURE_AC_SPLIT_LOCK	( 7*32+31) /* #AC for split lock */
> >>
> >> The last time this was posted, we (Intel) promised to go get the proper
> >> (CPUID or MSR-based) enumeration of this feature documented.  Did we do
> >> that?  If so, where is that documentation?
> > 
> > As said in the cover patch:
> > "Please note: The feature could be enumerated through
> > MSR IA32_CORE_CAPABILITY (0xCF). But the enumeration is not completely
> > published yet. So this patch set doesn't include the method."
> 
> FWIW, I think we (Intel) promised to get this documented _before_ we
> came back asking our intrepid x86 maintainers to accept something like
> setcpuid=.  While I generally applaud the post-early-post-often
> attitude, we might have been a bit trigger-happy on this one.
> 
> This is awfully crucial information to bury in a massive cover letter,
> especially when the patch description is a rather anemic two lines.
> 
Hi, Dave,

I do have two pathes available to enumerate IA32_CORE_CAPABILITY itself
and to enumerate #AC for split lock by bit 5 in the MSR. But because the
enumeration for the MSR is not published, I cannot release the two
patches in this patch set.

Hopefully this patch set without the two enumeration patches is
a meaningful set.

Thanks.

-Fenghua

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/4/19 9:49 AM, Thomas Gleixner wrote:
> On Fri, 1 Feb 2019, Fenghua Yu wrote:
>> This option behaves like existing kernel option clearcpuid.
> 
> No it does NOT. clearcpuid allows to disable things.
> 
> This allows to enable random CPUID bits without any sanity checking. Not
> going to happen. We made it clear in the past that functionality needs to
> be detectable by enumeration. We do quirks for broken crap, but this is
> just not how it works.

Hi Thomas,

I think we are trying persuade you like mentioned here:

> http://lkml.kernel.org/r/alpine.DEB.2.21.1807122153170.1597@nanos.tec.linutronix.de

But, we're not being very persuasive because we kinda forgot about the
"if and only if" condition that you mentioned.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Borislav Petkov]

On Mon, Feb 04, 2019 at 11:05:52AM -0800, Dave Hansen wrote:
> But, we're not being very persuasive because we kinda forgot about the
> "if and only if" condition that you mentioned.

But why does it have to be a cmdline parameter instead of
being an automatic thing which sets the proper bits in
arch/x86/kernel/cpu/intel.c based on f/m/s or MSR or whatever ?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/4/19 11:57 AM, Borislav Petkov wrote:
> On Mon, Feb 04, 2019 at 11:05:52AM -0800, Dave Hansen wrote:
>> But, we're not being very persuasive because we kinda forgot about the
>> "if and only if" condition that you mentioned.
> But why does it have to be a cmdline parameter instead of
> being an automatic thing which sets the proper bits in
> arch/x86/kernel/cpu/intel.c based on f/m/s or MSR or whatever ?

It doesn't have to be a cmdline parameter.

Intel can obviously add or remove enumeration for a feature after
silicon ships.  But, that eats up microcode "patch" space which is an
even more valuable resource than the microcode "ROM" space.  That patch
space is a very constrained resource when creating things like the
side-channel mitigations.  The way I read this situation is that this
feature fills a bit small of a niche to justify consuming patch space.

So, the compromise we reached in this case is that Intel will fully
document the future silicon architecture, and then write the kernel
implementation to _that_.  Then, for the weirdo deployments where this
feature is not enumerated, we have the setcpuid= to fake the enumeration
in software.

The reason I'm pushing for setcpuid= instead of a one-off is that I
don't expect this to be the last time Intel does this.  I'd rather have
one setcpuid= than a hundred things like "ac_split_lock_disable".

The other alternative is that folks will run custom (non-mainline)
kernels with small patches to override the lack of enumeration.  Doing
setcpuid= keeps folks on mainline.  (BTW, we should probably taint the
kernel on setcpuid=....).

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Fenghua Yu]

On Mon, Feb 04, 2019 at 11:05:52AM -0800, Dave Hansen wrote:
> On 2/4/19 9:49 AM, Thomas Gleixner wrote:
> > On Fri, 1 Feb 2019, Fenghua Yu wrote:
> >> This option behaves like existing kernel option clearcpuid.
> > 
> > No it does NOT. clearcpuid allows to disable things.
> > 
> > This allows to enable random CPUID bits without any sanity checking. Not
> > going to happen. We made it clear in the past that functionality needs to
> > be detectable by enumeration. We do quirks for broken crap, but this is
> > just not how it works.
> 
> Hi Thomas,
> 
> I think we are trying persuade you like mentioned here:
> 
> > http://lkml.kernel.org/r/alpine.DEB.2.21.1807122153170.1597@nanos.tec.linutronix.de
> 
> But, we're not being very persuasive because we kinda forgot about the
> "if and only if" condition that you mentioned.

Intel SDM published TODAY does have IA32_CORE_CAPABILITY MSR enumerateion
bit CPUID.0x7.0:EDX[30] now. Please check today's SDM for the bit:
https://software.intel.com/en-us/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4

I will add the two patches that enumerate the MSR and #AC for split lock
in the next version.

Thanks.

-Fenghua

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Borislav Petkov]

On Mon, Feb 04, 2019 at 12:46:30PM -0800, Dave Hansen wrote:
> Intel can obviously add or remove enumeration for a feature after
> silicon ships.  But, that eats up microcode "patch" space which is an
> even more valuable resource than the microcode "ROM" space.  That patch
> space is a very constrained resource when creating things like the
> side-channel mitigations.  The way I read this situation is that this
> feature fills a bit small of a niche to justify consuming patch space.

Yap, makes sense. I've heard that argumentation before, btw.

> So, the compromise we reached in this case is that Intel will fully
> document the future silicon architecture, and then write the kernel
> implementation to _that_.

Yap.

> Then, for the weirdo deployments where this feature is not enumerated,
> we have the setcpuid= to fake the enumeration in software.
>
> The reason I'm pushing for setcpuid= instead of a one-off is that I
> don't expect this to be the last time Intel does this. I'd rather have
> one setcpuid= than a hundred things like "ac_split_lock_disable".

So my only issue with this is the user having to type this in in order
to get the feature.

VS

automatically enabling it during boot in early_init_intel() or so. No
need for any user intervention. It'll be just like a forgotten CPUID bit
and we've done those before.

The disable chicken bits you have for all those features which are
enumerated in CPUID anyway so there'll be no difference.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Fenghua Yu]

On Mon, Feb 04, 2019 at 10:40:45PM +0100, Borislav Petkov wrote:
> On Mon, Feb 04, 2019 at 12:46:30PM -0800, Dave Hansen wrote:
> > Intel can obviously add or remove enumeration for a feature after
> > silicon ships.  But, that eats up microcode "patch" space which is an
> > even more valuable resource than the microcode "ROM" space.  That patch
> > space is a very constrained resource when creating things like the
> > side-channel mitigations.  The way I read this situation is that this
> > feature fills a bit small of a niche to justify consuming patch space.
> 
> Yap, makes sense. I've heard that argumentation before, btw.
> 
> > So, the compromise we reached in this case is that Intel will fully
> > document the future silicon architecture, and then write the kernel
> > implementation to _that_.
> 
> Yap.
> 
> > Then, for the weirdo deployments where this feature is not enumerated,
> > we have the setcpuid= to fake the enumeration in software.
> >
> > The reason I'm pushing for setcpuid= instead of a one-off is that I
> > don't expect this to be the last time Intel does this. I'd rather have
> > one setcpuid= than a hundred things like "ac_split_lock_disable".
> 
> So my only issue with this is the user having to type this in in order
> to get the feature.

With "setcpuid=", there is no additional code to add as long as
enumeration code is available.

> 
> VS
> 
> automatically enabling it during boot in early_init_intel() or so. No
> need for any user intervention. It'll be just like a forgotten CPUID bit
> and we've done those before.

Every time a new feature like this case, the early_init_intel() needs
to be changed for FMS etc.

I guess that's a reason we want to use "setcpuid=" to deal with different
cases withou changing code.

Thanks.

-Fenghua

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/4/19 1:40 PM, Borislav Petkov wrote:
>> Then, for the weirdo deployments where this feature is not enumerated,
>> we have the setcpuid= to fake the enumeration in software.
>>
>> The reason I'm pushing for setcpuid= instead of a one-off is that I
>> don't expect this to be the last time Intel does this. I'd rather have
>> one setcpuid= than a hundred things like "ac_split_lock_disable".
> So my only issue with this is the user having to type this in in order
> to get the feature.
> 
> VS
> 
> automatically enabling it during boot in early_init_intel() or so. No
> need for any user intervention. It'll be just like a forgotten CPUID bit
> and we've done those before.

Actually, there's one part of all this that I forgot.  Will split lock
detection be enumerated _widely_?  IOW, will my laptop in 5 years
enumerate support for it?  If so, we surely don't want to enable this
everyhwhere: it will break old apps.  Doesn't that mean that we need
both feature detection and another separate bit for folks to opt-in?

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Borislav Petkov]

On Mon, Feb 04, 2019 at 02:14:30PM -0800, Fenghua Yu wrote:
> With "setcpuid=", there is no additional code to add as long as
> enumeration code is available.

Wait, are you saying that all the other enablement of new features is
easy and the only problem is patching {early_,}init_intel() so you'd
prefer not to patch it each time and use a cmdline param instead which
is error prone and really user-unfriendly?!

Usually, the patch adding the CPUID flag and checking is the easiest
one.

Also, you do realize that even if it gets applied, it will need
to sanity-check everything passed in, which means, it will accept
*only* the leafs which you guys don't have in CPUID?! It won't be a
lets-enable-this-random-cpuid-bits-and-see-what-happens deal.

Because I don't think anyone will be willing to debug reports from such
random enablements. The qemu+kvm "experiments" are already painful
enough.

By then you're better off simply patching {early_,}init_intel() I'd say.

> Every time a new feature like this case, the early_init_intel() needs
> to be changed for FMS etc.

Yes, as part of the enablement. You really seldomly - if ever at all -
have a new feature which only needs CPUID enablement. Unless it is some
feature flag to show support for new insns but that gets applied almost
immediately and I doubt userspace even uses it through /proc/cpuinfo -
they do their own querying of CPUID.

Because if they do the latter, setcpuid= will give you nothing unless we
enforce CPUID faulting. Except *that* is not present everywhere...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Borislav Petkov]

On Mon, Feb 04, 2019 at 03:24:23PM -0800, Dave Hansen wrote:
> Actually, there's one part of all this that I forgot.  Will split lock
> detection be enumerated _widely_?

You never know what users will do. The moment it gets out, it better be
designed properly, along with the chicken bits.

> IOW, will my laptop in 5 years enumerate support for it?

Don't tell me this is going to be another MPX fiasco :-\

Or is this something along the lines of "let's see whether it takes off
and if yes, we'll commit to it or otherwise remove it and not even waste
a CPUID leaf"?

> If so, we surely don't want to enable this everyhwhere: it will break
> old apps. Doesn't that mean that we need both feature detection and
> another separate bit for folks to opt-in?

Well, if it breaks old apps, it probably needs to be opt-in anyway.

And for that you don't need setcpuid either - you simply boot with
"split_lock_ac" or whatever and the kernel pokes that MSR_TEST_CTL or
whatever else it needs to detect in hw for split lock and sets the
X86_FEATURE bits if detection is successful.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Mon, Feb 04, 2019 at 12:46:30PM -0800, Dave Hansen wrote:
> So, the compromise we reached in this case is that Intel will fully
> document the future silicon architecture, and then write the kernel
> implementation to _that_.  Then, for the weirdo deployments where this
> feature is not enumerated, we have the setcpuid= to fake the enumeration
> in software.

What user is _EVER_ going to use this? Nobody, I expect the answer to
be.

Is this some transient state; where a few (early) models will not have
the enumeration sorted but all later models will have it all neat and
tidy?

If so, we can easily do the FMS solution for this.

But a cmdline features thing is not something I can see anybody but
a limited set of developers ever using.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Mon, Feb 04, 2019 at 01:09:12PM -0800, Fenghua Yu wrote:

> Intel SDM published TODAY does have IA32_CORE_CAPABILITY MSR enumerateion
> bit CPUID.0x7.0:EDX[30] now. Please check today's SDM for the bit:
> https://software.intel.com/en-us/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4

When I click that link I get Nov'18, not Feb'19.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Mon, Feb 04, 2019 at 03:24:23PM -0800, Dave Hansen wrote:

> Actually, there's one part of all this that I forgot.  Will split lock
> detection be enumerated _widely_?  IOW, will my laptop in 5 years
> enumerate support for it?

I would bloody hope so. Just for giggles, create an little program that
does LOCK prefix across a line or page boundary in a while(1) loop and
'enjoy' your laptop experience.

> If so, we surely don't want to enable this
> everyhwhere: it will break old apps.  Doesn't that mean that we need
> both feature detection and another separate bit for folks to opt-in?

No, we very much do want to default enable this everywhere.

We might want to provide some chicken bits, like a (inheritable) PRCTL
or ELF flag to disable it for those broken apps.

But realistically, any app that triggers this is non-portable already,
most (if not all) the RISC architecture would already kill it with
SIGBUS for this.

We very much want the kernel _AND_ firmware to be #AC clean, always,
everywhere.

Heck, I'd love for #AC to be even stronger and not only trigger on
cross-line.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Tue, Feb 05, 2019 at 09:57:50AM +0100, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 03:24:23PM -0800, Dave Hansen wrote:
> 
> > Actually, there's one part of all this that I forgot.  Will split lock
> > detection be enumerated _widely_?  IOW, will my laptop in 5 years
> > enumerate support for it?
> 
> I would bloody hope so. Just for giggles, create an little program that
> does LOCK prefix across a line or page boundary in a while(1) loop and
> 'enjoy' your laptop experience.
> 
> > If so, we surely don't want to enable this
> > everyhwhere: it will break old apps.  Doesn't that mean that we need
> > both feature detection and another separate bit for folks to opt-in?
> 
> No, we very much do want to default enable this everywhere.
> 
> We might want to provide some chicken bits, like a (inheritable) PRCTL
> or ELF flag to disable it for those broken apps.

Combined with a sysctl that disables the chickens; such that
administrators can configure their system to be #AC pure.

> But realistically, any app that triggers this is non-portable already,
> most (if not all) the RISC architecture would already kill it with
> SIGBUS for this.
> 
> We very much want the kernel _AND_ firmware to be #AC clean, always,
> everywhere.
> 
> Heck, I'd love for #AC to be even stronger and not only trigger on
> cross-line.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/5/19 12:48 AM, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 12:46:30PM -0800, Dave Hansen wrote:
>> So, the compromise we reached in this case is that Intel will fully
>> document the future silicon architecture, and then write the kernel
>> implementation to _that_.  Then, for the weirdo deployments where this
>> feature is not enumerated, we have the setcpuid= to fake the enumeration
>> in software.
> 
> What user is _EVER_ going to use this? Nobody, I expect the answer to
> be.

This is one of the few times that we're pretty confident that folks will
use this.  The reason we're going to this trouble is that the split lock
detection is wanted by actual customers, and they want it before it's
implemented on a processor with real enumeration.

This isn't something we want everybody and their grandma to turn on;
it's a rather specialized feature.  It's really only for folks that care
about the latency incurred across the entire system on split lock
operations.

> Is this some transient state; where a few (early) models will not have
> the enumeration sorted but all later models will have it all neat and
> tidy?

From my understanding, it's not just an early stepping.  It's a
generational thing.  The current generation lacks the enumeration and
the next generation will get it.  Both have the silicon to implement the
feature itself.

> If so, we can easily do the FMS solution for this.

Yeah, we can.  I honestly forget why we didn't do FMS. :)

Fenghua?

> But a cmdline features thing is not something I can see anybody but
> a limited set of developers ever using.

It's not for developers.  This really is for (somewhat niche) end users
that want split lock detection in production.  This is all really an
effort to get them running mainline or real distro kernels.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/5/19 12:51 AM, Peter Zijlstra wrote:
> On Mon, Feb 04, 2019 at 01:09:12PM -0800, Fenghua Yu wrote:
> 
>> Intel SDM published TODAY does have IA32_CORE_CAPABILITY MSR enumerateion
>> bit CPUID.0x7.0:EDX[30] now. Please check today's SDM for the bit:
>> https://software.intel.com/en-us/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4
> When I click that link I get Nov'18, not Feb'19.

Try starting from intel.com/sdm.

It just got released yesterday, so it's entirely possible that some
caches were out of date when you tried.

I see "Order Number: 325462-069US January 2019" at the bottom of the
first page.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Tue, Feb 05, 2019 at 07:21:59AM -0800, Dave Hansen wrote:
> On 2/5/19 12:51 AM, Peter Zijlstra wrote:
> > On Mon, Feb 04, 2019 at 01:09:12PM -0800, Fenghua Yu wrote:
> > 
> >> Intel SDM published TODAY does have IA32_CORE_CAPABILITY MSR enumerateion
> >> bit CPUID.0x7.0:EDX[30] now. Please check today's SDM for the bit:
> >> https://software.intel.com/en-us/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4
> > When I click that link I get Nov'18, not Feb'19.
> 
> Try starting from intel.com/sdm.
> 
> It just got released yesterday, so it's entirely possible that some
> caches were out of date when you tried.
> 
> I see "Order Number: 325462-069US January 2019" at the bottom of the
> first page.

Still Nov'18. I suppose I'll have to wait moar...

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Borislav Petkov]

On Tue, Feb 05, 2019 at 07:19:16AM -0800, Dave Hansen wrote:
> This is one of the few times that we're pretty confident that folks will
> use this.  The reason we're going to this trouble is that the split lock
> detection is wanted by actual customers, and they want it before it's
> implemented on a processor with real enumeration.
>
> This isn't something we want everybody and their grandma to turn on;
> it's a rather specialized feature.  It's really only for folks that care
> about the latency incurred across the entire system on split lock
> operations.

...

> It's not for developers.  This really is for (somewhat niche) end users
> that want split lock detection in production.  This is all really an
> effort to get them running mainline or real distro kernels.

This all sounds to me like it shouldn't even be mainline but in a
special, evaluation kernel. If anything, it should be default off and be
opted-in by a cmdline switch. None of that *cpuid=* toggling dance.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/4/19 10:18 PM, Borislav Petkov wrote:
> On Mon, Feb 04, 2019 at 03:24:23PM -0800, Dave Hansen wrote:
>> Actually, there's one part of all this that I forgot.  Will split lock
>> detection be enumerated _widely_?
> 
> You never know what users will do. The moment it gets out, it better be
> designed properly, along with the chicken bits.

Sure.  I think this was just the simplest implementation we could come
up with.  There was more complexity before, and Thomas suggested
stripping it back to the bare-bones like we have here.

>> IOW, will my laptop in 5 years enumerate support for it?
> 
> Don't tell me this is going to be another MPX fiasco :-\
> 
> Or is this something along the lines of "let's see whether it takes off
> and if yes, we'll commit to it or otherwise remove it and not even waste
> a CPUID leaf"?

"Is Intel serious enough to put in a CPUID leaf" is a pretty good litmus
test INMHO.  I think it's one of the reasons that Thomas said he would
consider this if Intel was willing to go to the trouble of adding proper
enumeration.

>> If so, we surely don't want to enable this everyhwhere: it will break
>> old apps. Doesn't that mean that we need both feature detection and
>> another separate bit for folks to opt-in?
> 
> Well, if it breaks old apps, it probably needs to be opt-in anyway.

Yes, this was my assumption.

> And for that you don't need setcpuid either - you simply boot with
> "split_lock_ac" or whatever and the kernel pokes that MSR_TEST_CTL or
> whatever else it needs to detect in hw for split lock and sets the
> X86_FEATURE bits if detection is successful.

That's actually what we did in the last set.

Anyway...  There are a few branches of this discussion.  Let's wait for
Fenghua to tell us how universal this feature is and if
family/model/stepping detection will work.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Tue, Feb 05, 2019 at 07:19:16AM -0800, Dave Hansen wrote:
> On 2/5/19 12:48 AM, Peter Zijlstra wrote:
> > On Mon, Feb 04, 2019 at 12:46:30PM -0800, Dave Hansen wrote:
> >> So, the compromise we reached in this case is that Intel will fully
> >> document the future silicon architecture, and then write the kernel
> >> implementation to _that_.  Then, for the weirdo deployments where this
> >> feature is not enumerated, we have the setcpuid= to fake the enumeration
> >> in software.
> > 
> > What user is _EVER_ going to use this? Nobody, I expect the answer to
> > be.
> 
> This is one of the few times that we're pretty confident that folks will
> use this.  The reason we're going to this trouble is that the split lock
> detection is wanted by actual customers, and they want it before it's
> implemented on a processor with real enumeration.

That's big customers that do magic stuff not users.

> This isn't something we want everybody and their grandma to turn on;
> it's a rather specialized feature.  It's really only for folks that care
> about the latency incurred across the entire system on split lock
> operations.

That really should be everyone. That split lock stuff is horrible. There
is no real down-side to having it always enabled. Code that breaks is
bad code you want fixed anyway.

Like I said elsewhere, I wish it would #AC for any unaligned LOCK
prefix, not just cross-line. I see why we'd not want to traditional RISC
#AC for every load/store, but atomics really had better be aligned.

> > Is this some transient state; where a few (early) models will not have
> > the enumeration sorted but all later models will have it all neat and
> > tidy?
> 
> From my understanding, it's not just an early stepping.  It's a
> generational thing.  The current generation lacks the enumeration and
> the next generation will get it.  Both have the silicon to implement the
> feature itself.

I never said stepping, in fact I explicitly said model.

> > If so, we can easily do the FMS solution for this.
> 
> Yeah, we can.  I honestly forget why we didn't do FMS. :)

Right so FMS is fairly horrible; but when it is a stop-gap for a limited
number of models it's waaay better than dodgy cmdline things.

We could of course try to wrmsr_safe() detect the feature; but that
might be a problem is the MSR exists on any other models and has a
different meaning.

> > But a cmdline features thing is not something I can see anybody but
> > a limited set of developers ever using.
> 
> It's not for developers.  This really is for (somewhat niche) end users
> that want split lock detection in production.  This is all really an
> effort to get them running mainline or real distro kernels.

Yes, because cloud service providers and RT are niche products.. *sigh*.

Nobody wants to do live audio on their laptops.. oh wait..

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Tue, Feb 05, 2019 at 08:46:23AM -0800, Dave Hansen wrote:
> On 2/4/19 10:18 PM, Borislav Petkov wrote:

> > Well, if it breaks old apps, it probably needs to be opt-in anyway.
> 
> Yes, this was my assumption.

It _should_ not break portable code (because RISC has much stronger #AC
requirements).

That said; I really do think we want it default enabled and allow
individual apps to opt-out with a system-wide override to disable the
opt-out.

The cloud vendors and RT thingies will want to disable the opt-out for
example.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Fenghua Yu]

On Tue, Feb 05, 2019 at 04:43:09PM +0100, Borislav Petkov wrote:
> On Tue, Feb 05, 2019 at 07:19:16AM -0800, Dave Hansen wrote:
> > This is one of the few times that we're pretty confident that folks will
> > use this.  The reason we're going to this trouble is that the split lock
> > detection is wanted by actual customers, and they want it before it's
> > implemented on a processor with real enumeration.
> >
> > This isn't something we want everybody and their grandma to turn on;
> > it's a rather specialized feature.  It's really only for folks that care
> > about the latency incurred across the entire system on split lock
> > operations.
> 
> ...
> 
> > It's not for developers.  This really is for (somewhat niche) end users
> > that want split lock detection in production.  This is all really an
> > effort to get them running mainline or real distro kernels.
> 
> This all sounds to me like it shouldn't even be mainline but in a
> special, evaluation kernel. If anything, it should be default off and be
> opted-in by a cmdline switch. None of that *cpuid=* toggling dance

In the cover patch, we gave three real usage cases for this feature:

This capability is critical for real time system designers who build
consolidated real time systems. These systems run hard real time
code on some cores and run "untrusted" user processes on some
other cores. The hard real time cannot afford to have any bus lock from
the untrusted processes to hurt real time performance. To date the
designers have been unable to deploy these solutions as they have no
way to prevent the "untrusted" user code from generating split lock and
bus lock to block the hard real time code to access memory during bus
locking.

This capability may also find usage in cloud. A user process with split
lock running in one guest can block other cores from accessing shared
memory during its split locked memory access. That may cause overall
system performance degradation.

Split lock may open a security hole where malicious user code may slow
down overall system by executing instructions with split lock.

In the usage cases, end users (not developers) want to turn on the
feature in their production lines (not evaluation) to detect split lock
issues which can come not only from legacy apps but also from
malicious code or newly started apps any time during run time.

Thanks.

-Fenghua

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Thomas Gleixner]

On Tue, 5 Feb 2019, Peter Zijlstra wrote:
> On Tue, Feb 05, 2019 at 07:19:16AM -0800, Dave Hansen wrote:
> > On 2/5/19 12:48 AM, Peter Zijlstra wrote:
> > This isn't something we want everybody and their grandma to turn on;
> > it's a rather specialized feature.  It's really only for folks that care
> > about the latency incurred across the entire system on split lock
> > operations.
> 
> That really should be everyone. That split lock stuff is horrible. There
> is no real down-side to having it always enabled. Code that breaks is
> bad code you want fixed anyway.
> 
> Like I said elsewhere, I wish it would #AC for any unaligned LOCK
> prefix, not just cross-line. I see why we'd not want to traditional RISC
> #AC for every load/store, but atomics really had better be aligned.

Right, we should really make this default enabled.

> > > Is this some transient state; where a few (early) models will not have
> > > the enumeration sorted but all later models will have it all neat and
> > > tidy?
> > 
> > From my understanding, it's not just an early stepping.  It's a
> > generational thing.  The current generation lacks the enumeration and
> > the next generation will get it.  Both have the silicon to implement the
> > feature itself.
> 
> I never said stepping, in fact I explicitly said model.
> 
> > > If so, we can easily do the FMS solution for this.
> > 
> > Yeah, we can.  I honestly forget why we didn't do FMS. :)
> 
> Right so FMS is fairly horrible; but when it is a stop-gap for a limited
> number of models it's waaay better than dodgy cmdline things.

One or two is fine. And _IF_ we get the enumeration sorted before we merge
that, then we can declare the FM list as immutable :)
 
> We could of course try to wrmsr_safe() detect the feature; but that
> might be a problem is the MSR exists on any other models and has a
> different meaning.

Well, yes, but that would be pretty stupid.

Thanks,

	tglx

Re: [PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[Ingo Molnar]

* Fenghua Yu <fenghua.yu@intel.com> wrote:

> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -61,6 +61,7 @@
>  #include <asm/mpx.h>
>  #include <asm/vm86.h>
>  #include <asm/umip.h>
> +#include <asm/cpu.h>
>  
>  #ifdef CONFIG_X86_64
>  #include <asm/x86_init.h>
> @@ -292,9 +293,36 @@ DO_ERROR(X86_TRAP_OLD_MF, SIGFPE,           0, NULL, "coprocessor segment overru
>  DO_ERROR(X86_TRAP_TS,     SIGSEGV,          0, NULL, "invalid TSS",         invalid_TSS)
>  DO_ERROR(X86_TRAP_NP,     SIGBUS,           0, NULL, "segment not present", segment_not_present)
>  DO_ERROR(X86_TRAP_SS,     SIGBUS,           0, NULL, "stack segment",       stack_segment)
> -DO_ERROR(X86_TRAP_AC,     SIGBUS,  BUS_ADRALN, NULL, "alignment check",     alignment_check)
>  #undef IP
>  
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> +{
> +	unsigned int trapnr = X86_TRAP_AC;
> +	char str[] = "alignment check";
> +	int signr = SIGBUS;
> +	int ret;
> +
> +	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> +	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
> +			NOTIFY_STOP) {
> +		/* #AC exception could be handled by split lock handler. */
> +		ret = do_ac_split_lock(regs);
> +		if (ret) {
> +			cond_local_irq_enable(regs);
> +
> +			return;
> +		}
> +
> +		cond_local_irq_enable(regs);
> +		/*
> +		 * If not processed by split lock handler, go to generic
> +		 * #AC handler.
> +		 */
> +		do_trap(trapnr, signr, str, regs, error_code, BUS_ADRALN, NULL);
> +	}
> +}

Is there any experience with how frequently this signal is killing 
user-space processes on a modern distro? Any expectation of how frequent 
such SIGBUS task terminations are going to be in the field?

Thanks,

	Ingo

Re: [PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[Fenghua Yu]

On Mon, Feb 11, 2019 at 11:53:39AM +0100, Ingo Molnar wrote:
> > +		do_trap(trapnr, signr, str, regs, error_code, BUS_ADRALN, NULL);
> > +	}
> > +}
> 
> Is there any experience with how frequently this signal is killing 
> user-space processes on a modern distro? Any expectation of how frequent 
> such SIGBUS task terminations are going to be in the field?

We did pretty intensive testing internally (zero day tests, many engineers
and testers use the patches in their dailly work, etc). So far I'm not
aware of any user space process hiting split lock issue. I guess GCC or
other compilers takes care of the issue already. Inline assembly code may
hit the issue if code is not right, but there are fewer inline assembly
code in user space.

So far we only find two kernel split lock issues and fix them in the first
two patches in this patch set. We also find one BIOS split lock issue
internally which has been fixed in production BIOS.

Thanks.

-Fenghua

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Fenghua Yu]

On Sun, Feb 10, 2019 at 08:20:20PM +0100, Thomas Gleixner wrote:
> On Tue, 5 Feb 2019, Peter Zijlstra wrote:
> > On Tue, Feb 05, 2019 at 07:19:16AM -0800, Dave Hansen wrote:
> > > On 2/5/19 12:48 AM, Peter Zijlstra wrote:
> > > This isn't something we want everybody and their grandma to turn on;
> > > it's a rather specialized feature.  It's really only for folks that care
> > > about the latency incurred across the entire system on split lock
> > > operations.
> > 
> > That really should be everyone. That split lock stuff is horrible. There
> > is no real down-side to having it always enabled. Code that breaks is
> > bad code you want fixed anyway.
> > 
> > Like I said elsewhere, I wish it would #AC for any unaligned LOCK
> > prefix, not just cross-line. I see why we'd not want to traditional RISC
> > #AC for every load/store, but atomics really had better be aligned.
> 
> Right, we should really make this default enabled.

Yes, I agree.

> 
> > > > Is this some transient state; where a few (early) models will not have
> > > > the enumeration sorted but all later models will have it all neat and
> > > > tidy?
> > > 
> > > From my understanding, it's not just an early stepping.  It's a
> > > generational thing.  The current generation lacks the enumeration and
> > > the next generation will get it.  Both have the silicon to implement the
> > > feature itself.
> > 
> > I never said stepping, in fact I explicitly said model.
> > 
> > > > If so, we can easily do the FMS solution for this.
> > > 
> > > Yeah, we can.  I honestly forget why we didn't do FMS. :)
> > 
> > Right so FMS is fairly horrible; but when it is a stop-gap for a limited
> > number of models it's waaay better than dodgy cmdline things.
> 
> One or two is fine. And _IF_ we get the enumeration sorted before we merge
> that, then we can declare the FM list as immutable :)

There will be about 8 models that have the split lock feature but
don't have the IA32_CORE_CAPABILITY enumeration. The models will NOT
have the IA32_CORE_CAPABILITY enumerate in the future as planned. All
of other models than these ones will have the IA32_CORE_CAPABILITY
enumeration.

I don't see any of the model numbers are public yet as of now.

In the next version of patches, I will do the following changes from
v3:

1. Remove patch #11 that implements "setcpuid=".
2. But I will not implement the patch that enables the split lock feature
based on FMS because there is no public FMS numbers of those models. The
patch/patches will be implemented only after the FMS numbers are public.
3. The new patches enables the feature once it enumerates the feature
by bit 5 in IA32_CORE_CAPABILITY.
4. The feature can be disabled by kernel option
"clearcpuid=split_lock_detection" during early boot time.
5. The feature can be disabled/enabled during run time by
sysfs interface "/sys/kernel/split_lock_detection"

Does that make sense?

Thanks.

-Fenghua

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Mon, Feb 11, 2019 at 11:16:43AM -0800, Fenghua Yu wrote:
> 4. The feature can be disabled by kernel option
> "clearcpuid=split_lock_detection" during early boot time.

IFF clearcpuid lives, it should also employ CPUID faulting and clear it
for userspace too.

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Thomas Gleixner]

On Tue, 12 Feb 2019, Peter Zijlstra wrote:

> On Mon, Feb 11, 2019 at 11:16:43AM -0800, Fenghua Yu wrote:
> > 4. The feature can be disabled by kernel option
> > "clearcpuid=split_lock_detection" during early boot time.
> 
> IFF clearcpuid lives, it should also employ CPUID faulting and clear it
> for userspace too.

We have it already, and aside of clearcpuid there are enough things which
the kernel disables in the kernel view of CPUID, but user space still can
see them. That's inconsistent, so we really should use CPUID faulting when
its available.

That won't solve the problem of user space ignoring CPUID alltogether and
just probing crap, but you can't prevent that at all.

For that we'd need a CPUID mask facility in the hardware, which would
default to 0xFFFFFFFFFF and the kernel could clear bits in the mask to turn
of both the CPUID bit _AND_ the connected functionality. IOW, if you mask a
bit and user space probes the functionality brute force, it'll #GP and we
can just kill it.

Thanks,

	tglx

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Peter Zijlstra]

On Tue, Feb 12, 2019 at 02:51:00PM +0100, Thomas Gleixner wrote:
> On Tue, 12 Feb 2019, Peter Zijlstra wrote:
> 
> > On Mon, Feb 11, 2019 at 11:16:43AM -0800, Fenghua Yu wrote:
> > > 4. The feature can be disabled by kernel option
> > > "clearcpuid=split_lock_detection" during early boot time.
> > 
> > IFF clearcpuid lives, it should also employ CPUID faulting and clear it
> > for userspace too.
> 
> We have it already,

D'0h right, I thought that was introduced here, but that's just
extending it to multiple arguments.

> and aside of clearcpuid there are enough things which
> the kernel disables in the kernel view of CPUID, but user space still can
> see them. That's inconsistent, so we really should use CPUID faulting when
> its available.

How about something terrible like this then? _completely_ untested, not
been near a compiler.

---
 arch/x86/include/asm/cpufeature.h |  4 +++
 arch/x86/kernel/cpu/common.c      |  2 ++
 arch/x86/kernel/cpu/cpuid-deps.c  | 52 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/intel.c       | 47 ++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/scattered.c   | 15 +++++++++++
 arch/x86/kernel/process.c         |  3 +++
 arch/x86/kernel/traps.c           | 11 +++++++++
 7 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index ce95b8cbd229..a41d147af110 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -226,5 +226,9 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
 #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
 					boot_cpu_data.x86_model
 
+extern int cpuid_fault;
+extern u32 scattered_cpuid_mask(u32 leaf, u32 count, enum cpuid_regs_idx reg);
+extern u32 cpuid_cap_mask(u32 leaf, u32 count, enum cpuid_regs_idx reg)
+
 #endif /* defined(__KERNEL__) && !defined(__ASSEMBLY__) */
 #endif /* _ASM_X86_CPUFEATURE_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cb28e98a0659..5abc0ef725a7 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1499,6 +1499,8 @@ void print_cpu_info(struct cpuinfo_x86 *c)
 		pr_cont(")\n");
 }
 
+int cpuid_fault;
+
 /*
  * clearcpuid= was already parsed in fpu__init_parse_early_param.
  * But we need to keep a dummy __setup around otherwise it would
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 2c0bd38a44ab..3c5a590508a5 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -117,5 +117,57 @@ void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 
 void setup_clear_cpu_cap(unsigned int feature)
 {
+	if (boot_cpu_has(feature))
+		cpuid_fault = 1;
 	do_clear_cpu_cap(NULL, feature);
 }
+
+u32 cpuid_cap_mask(u32 leaf, u32 count, enum cpuid_regs_idx reg)
+{
+	switch (leaf) {
+	case 0x1:
+		if (reg == CPUID_EDX)
+			return ~cpu_caps_cleared[CPUID_1_EDX];
+		if (reg == CPUID_ECX)
+			return ~cpu_caps_cleared[CPUID_1_ECX];
+		break;
+
+	case 0x6:
+		if (reg == CPUID_EAX)
+			return ~cpu_caps_cleared[CPUID_6_EAX];
+		break;
+
+	case 0x7:
+		if (reg == CPUID_EDX)
+			return ~cpu_caps_cleared[CPUID_7_EDX];
+		if (reg == CPUID_ECX)
+			return ~cpu_caps_cleared[CPUID_7_ECX];
+		if (reg == CPUID_EBX && count == 0)
+			return ~cpu_caps_cleared[CPUID_7_0_EBX];
+		break;
+
+
+	case 0xD:
+		if (reg == CPUID_EAX)
+			return ~cpu_caps_cleared[CPUID_D_EAX];
+		break;
+
+	case 0xF:
+		if (reg == CPUID_EDX) {
+			if (count == 0)
+				return ~cpu_caps_cleared[CPUID_F_0_EDX];
+			if (count == 1)
+				return ~cpu_caps_cleared[CPUID_F_0_EDX];
+		}
+		break;
+
+	case 0x80000007:
+		if (reg == CPUID_EDX) {
+			if (test_bit(X86_FEATURE_CONSTANT_TSC, cpu_caps_cleared))
+				return ~BIT(8);
+		}
+		break;
+	}
+
+	return scattered_cpuid_mask(leaf, count, reg);
+}
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fc3c07fe7df5..85a37239d009 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -626,13 +626,58 @@ static void intel_bsp_resume(struct cpuinfo_x86 *c)
 	init_intel_energy_perf(c);
 }
 
+bool fixup_cpuid_exception(struct pt_regs *regs)
+{
+	unsigned int leaf, count, eax, ebx, ecx, edx;
+	unsigned long seg_base = 0;
+	unsigned char buf[2];
+	int not_copied;
+
+	if (!cpuid_fault)
+		return false;
+
+	if (test_thread_flag(TIF_NOCPUID))
+		return false;
+
+	if (!user_64bit_mode(regs))
+		seg_base = insn_get_seg_base(regs, INAT_SEG_REG_CS);
+
+	if (seg_base == -1L)
+		return false;
+
+	not_copied = copy_from_user(buf, (void __user *)(seg_base + regs->ip), sizeof(buf));
+	if (not_copied)
+		return false;
+
+	if (buf[0] != 0x0F || buf[1] != 0xA2) /* CPUID - OF A2 */
+		return false;
+
+	leaf = regs->ax;
+	count = regs->cx;
+
+	cpuid_count(leaf, count, &eax, &ebx, &ecx, &edx);
+
+	regs->ip += 2;
+	regs->ax = eax & cpuid_cap_mask(leaf, count, CPUID_EAX);
+	regs->bx = ebx & cpuid_cap_mask(leaf, count, CPUID_EBX);
+	regs->cx = ecx & cpuid_cap_mask(leaf, count, CPUID_ECX);
+	regs->dx = edx & cpuid_cap_mask(leaf, count, CPUID_EDX);
+
+	return true;
+}
+
 static void init_cpuid_fault(struct cpuinfo_x86 *c)
 {
 	u64 msr;
 
 	if (!rdmsrl_safe(MSR_PLATFORM_INFO, &msr)) {
-		if (msr & MSR_PLATFORM_INFO_CPUID_FAULT)
+		if (msr & MSR_PLATFORM_INFO_CPUID_FAULT) {
 			set_cpu_cap(c, X86_FEATURE_CPUID_FAULT);
+			if (cpuid_fault) {
+				this_cpu_or(msr_misc_features_shadow,
+					    MSR_MISC_FEATURES_ENABLES_CPUID_FAULT);
+			}
+		}
 	}
 }
 
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 94aa1c72ca98..faee10864db9 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -62,3 +62,18 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 			set_cpu_cap(c, cb->feature);
 	}
 }
+
+u32 scattered_cpuid_mask(u32 leaf, u32 count, enum cpuid_regs_idx reg)
+{
+	const struct cpuid_bit *cb;
+	u32 mask = ~0U;
+
+	for (cp = cpuid_bits; cb->feature; cb++) {
+		if (cb->level == leaf && cb->sub_leaf == count && cb->reg == reg) {
+			if (test_bit(cb->feature, cpu_caps_cleared))
+				mask &= ~BIT(cb->bit);
+		}
+	}
+
+	return mask;
+}
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 58ac7be52c7a..2b1dfd7ae65d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -196,6 +196,9 @@ static void set_cpuid_faulting(bool on)
 {
 	u64 msrval;
 
+	if (cpuid_fault)
+		return;
+
 	msrval = this_cpu_read(msr_misc_features_shadow);
 	msrval &= ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT;
 	msrval |= (on << MSR_MISC_FEATURES_ENABLES_CPUID_FAULT_BIT);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 2684a9d11e66..1762848cd2bd 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -517,6 +517,12 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 	do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
 }
 
+#ifdef CONFIG_CPU_SUP_INTEL
+extern bool fixup_cpuid_exception(struct pt_regs *regs);
+#else
+static inline bool fixup_cpuid_exception(struct pt_regs *regs) { return false; }
+#endif
+
 dotraplinkage void
 do_general_protection(struct pt_regs *regs, long error_code)
 {
@@ -531,6 +537,11 @@ do_general_protection(struct pt_regs *regs, long error_code)
 			return;
 	}
 
+	if (static_cpu_has(X86_FEATURE_CPUID_FAULT)) {
+		if (user_mode(regs) && fixup_cpuid_exception(regs))
+			return;
+	}
+
 	if (v8086_mode(regs)) {
 		local_irq_enable();
 		handle_vm86_fault((struct kernel_vm86_regs *) regs, error_code);

Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Dave Hansen]

On 2/12/19 8:48 AM, Peter Zijlstra wrote:
>>> IFF clearcpuid lives, it should also employ CPUID faulting and clear it
>>> for userspace too.
>> We have it already,
> D'0h right, I thought that was introduced here, but that's just
> extending it to multiple arguments.

... and making it take strings in addition to numbers so folks can use
the strings they see in /proc/cpuinfo.

RE: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

[Yu, Fenghua]

> From: Peter Zijlstra [mailto:peterz@infradead.org]
> On Tue, Feb 12, 2019 at 02:51:00PM +0100, Thomas Gleixner wrote:
> > On Tue, 12 Feb 2019, Peter Zijlstra wrote:
> >
> > > On Mon, Feb 11, 2019 at 11:16:43AM -0800, Fenghua Yu wrote:
> > > > 4. The feature can be disabled by kernel option
> > > > "clearcpuid=split_lock_detection" during early boot time.
> > >
> > > IFF clearcpuid lives, it should also employ CPUID faulting and clear
> > > it for userspace too.
> >
> > We have it already,
> 
> D'0h right, I thought that was introduced here, but that's just extending it
> to multiple arguments.

In this patch set, patch #5 extends clearcpuid to support multiple options and
Patch #6 extends clearcpuid to support cpu cap flag strings. So we already have
string support and can use "clearcpuid=split_lock_detection" to disable
the split lock feature at the boot time.

Thanks.

-Fenghua

Re: [PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[Ingo Molnar]

* Fenghua Yu <fenghua.yu@intel.com> wrote:

> On Mon, Feb 11, 2019 at 11:53:39AM +0100, Ingo Molnar wrote:
> > > +		do_trap(trapnr, signr, str, regs, error_code, BUS_ADRALN, NULL);
> > > +	}
> > > +}
> > 
> > Is there any experience with how frequently this signal is killing 
> > user-space processes on a modern distro? Any expectation of how frequent 
> > such SIGBUS task terminations are going to be in the field?
> 
> We did pretty intensive testing internally (zero day tests, many engineers
> and testers use the patches in their dailly work, etc). So far I'm not
> aware of any user space process hiting split lock issue. I guess GCC or
> other compilers takes care of the issue already. Inline assembly code may
> hit the issue if code is not right, but there are fewer inline assembly
> code in user space.
> 
> So far we only find two kernel split lock issues and fix them in the first
> two patches in this patch set. We also find one BIOS split lock issue
> internally which has been fixed in production BIOS.
> 
> Thanks.

Ok, still, for binary compatibility's sake, could you please add a sysctl 
knob (root-only, etc.) and a kernel boot parameter that disables this 
check?

Looks good otherwise.

Thanks,

	Ingo

RE: [PATCH v3 10/10] x86/split_lock: Handle #AC exception for split lock

[Yu, Fenghua]

> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo Molnar
> > On Mon, Feb 11, 2019 at 11:53:39AM +0100, Ingo Molnar wrote:
> > > > +		do_trap(trapnr, signr, str, regs, error_code, BUS_ADRALN,
> NULL);
> > > > +	}
> > > > +}
> > >
> > > Is there any experience with how frequently this signal is killing
> > > user-space processes on a modern distro? Any expectation of how
> > > frequent such SIGBUS task terminations are going to be in the field?
> >
> > We did pretty intensive testing internally (zero day tests, many
> > engineers and testers use the patches in their dailly work, etc). So
> > far I'm not aware of any user space process hiting split lock issue. I
> > guess GCC or other compilers takes care of the issue already. Inline
> > assembly code may hit the issue if code is not right, but there are
> > fewer inline assembly code in user space.
> >
> > So far we only find two kernel split lock issues and fix them in the
> > first two patches in this patch set. We also find one BIOS split lock
> > issue internally which has been fixed in production BIOS.
> >
> > Thanks.
> 
> Ok, still, for binary compatibility's sake, could you please add a sysctl knob
> (root-only, etc.) and a kernel boot parameter that disables this check?

In this patch set, we already extend kernel option "clearcpuid=" to support CPU
cap flags. So "clearcpuid=split_lock_detection" can disable this split lock
check during boot time.

In next version, I will add "/sys/kernel/split_lock_detection" to enable or disable
the split lock check during run time.

Hopefully these work.

Thanks.

-Fenghua