[PATCH v8 00/15] x86/split_lock: Enable split lock detection

[PATCH v8 00/15] x86/split_lock: Enable split lock detection

[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 performance
on one CPU but also overall system performance.

If the operand is cacheable and completely contained in one cache line,
the atomic operation is optimized by less expensive cache locking on
Intel P6 and recent processors. If a split lock operation is detected
and a developer fixes the issue so that the operand can be operated in one
cache line, cache locking instead of more expensive bus locking will be
used for the atomic operation. Removing the 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.

==Split lock detection==

Currently Linux 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.

==Enumerate split lock detection feature==

A control bit (bit 29) in MSR_TEST_CTL (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.

The split lock detection feature is enumerated through bit 5 in
MSR_IA32_CORE_CAPABILITY (0xcf). The MSR 0xcf itself is enumerated by
CPUID.(EAX=0x7,ECX=0):EDX[30].

The enumeration method is published in the latest Intel 64 and IA-32
Architecture Software Developer's Manual.

A few processors have the split lock detection feature. But they don't
have MSR_IA32_CORE_CAPABILITY to enumerate the feature. On those
processors, enumerate the split lock detection feature based on their
family/model/stepping numbers.

==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 the feature.

But this patch set uses 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 split lock
detection is disabled on the current 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.

- Enable split lock detection by default once the feature is enumerated.

- Disable split lock detection by kernel parameter "nosplit_lock_detect"
during boot time.

- Disable/enable split lock detection by sysfs interface
/sys/devices/system/cpu/split_lock_detect during run time.

==Expose to guest==

To expose this feature to guest, need to
1. Report the new CPUID bit to guest.
2. Emulate IA32_CORE_CAPABILITIES MSR.
3. Emulate TEST_CTL MSR. Since this patch series enable split lock
detection in host kernel by default, if do not emulate MSR_TEST_CTL
for guest, guest will run with the value set by host without knowing
that. So guest will run with split lock detection enabled due to the
host's value. Thus guest running with buggy firmware and old kernel
will fail because they lack the ability to handle #AC for split lock.
So need to emulate MSR_TEST_CTL and separate its value between host
and guest.

==Patches==
Patch 1-4: Fix a few existing split lock issues.
Patch 5-9: Enumerate features and define MSR_TEST_CTL.
Patch 10: Handle #AC for split lock.
Patch 11-12: Enable split lock detection in KVM.
Patch 13: Enable split lock detection by default after #AC handler and KVM 
are installed.
Patch 14: Disable split lock detection by kernel parameter
"nosplit_lock_detect" during boot time.
Patch 15: Define a sysfs interface to enable/disable split lock
detection during run time.

==Changelog==
v8:
Address issues pointed out by Thomas Gleixner:
- Remove all "clearcpuid=" related patches.
- Add kernel parameter "nosplit_lock_detect" patch.
- Merge definition and initialization of msr_test_ctl_cache into #AC
  handling patch which first uses the variable.
- Add justification for the sysfs knob and combine function and doc
  patches into one patch 0015.
- A few other adjustments.

v7:
- Add per cpu variable to cach MSR TEST_CTL. Suggested by Thomas Gleixner.
- Change a few other changes including locking, simplifying code, work
flow, KVM fixes, etc. Suggested by Thomas Gleixner.
- Fix KVM issues pointed out by Sean Christopherson.

v6:
- Fix #AC handler issues pointed out by Dave Hansen
- Add doc for the sysfs interface pointed out by Dave Hansen
- Fix a lock issue around wrmsr during split lock init, pointed out by Dave
  Hansen
- Update descriptions and comments suggested by Dave Hansen
- Fix __le32 issue in wlcore raised by Kalle Valo
- Add feature enumeration based on family/model/stepping for Icelake mobile

v5:
- Fix wlcore issue from Paolo Bonzini
- Fix b44 issue from Peter Zijlstra
- Change init sequence by Dave Hansen
- Fix KVM issues from Paolo Bonzini
- Re-order patch sequence

v4:
- Remove "setcpuid=" option
- Enable IA32_CORE_CAPABILITY enumeration for split lock
- Handle CPUID faulting by Peter Zijlstra
- Enable /sys interface to enable/disable split lock detection

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 (11):
  x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long
  x86/split_lock: Align x86_capability to unsigned long to avoid split
    locked access
  x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock
    detection bit
  x86/cpufeatures: Enumerate MSR_IA32_CORE_CAPABILITY
  x86/split_lock: Enumerate split lock detection by
    MSR_IA32_CORE_CAPABILITY
  x86/split_lock: Enumerate split lock detection on Icelake mobile
    processor
  x86/split_lock: Define MSR TEST_CTL register
  x86/split_lock: Handle #AC exception for split lock
  x86/split_lock: Enable split lock detection by default
  x86/split_lock: Disable split lock detection by kernel parameter
    "nosplit_lock_detect"
  x86/split_lock: Add a sysfs interface to enable/disable split lock
    detection during run time

Paolo Bonzini (1):
  wlcore: simplify/fix/optimize reg_ch_conf_pending operations

Peter Zijlstra (1):
  drivers/net/b44: Align pwol_mask to unsigned long for better
    performance

Xiaoyao Li (2):
  kvm/x86: Emulate MSR IA32_CORE_CAPABILITY
  kvm/vmx: Emulate MSR TEST_CTL

 .../ABI/testing/sysfs-devices-system-cpu      |  22 +++
 .../admin-guide/kernel-parameters.txt         |   2 +
 arch/x86/include/asm/cpu.h                    |   8 ++
 arch/x86/include/asm/cpufeatures.h            |   2 +
 arch/x86/include/asm/kvm_host.h               |   1 +
 arch/x86/include/asm/msr-index.h              |   7 +
 arch/x86/include/asm/processor.h              |   4 +-
 arch/x86/kernel/cpu/common.c                  |   7 +-
 arch/x86/kernel/cpu/cpuid-deps.c              |  79 ++++++-----
 arch/x86/kernel/cpu/intel.c                   | 133 ++++++++++++++++++
 arch/x86/kernel/traps.c                       |  31 +++-
 arch/x86/kvm/cpuid.c                          |   6 +
 arch/x86/kvm/vmx/vmx.c                        |  42 ++++++
 arch/x86/kvm/vmx/vmx.h                        |   2 +
 arch/x86/kvm/x86.c                            |  39 +++++
 drivers/net/ethernet/broadcom/b44.c           |   4 +-
 drivers/net/wireless/ti/wlcore/cmd.c          |  15 +-
 drivers/net/wireless/ti/wlcore/wlcore.h       |   4 +-
 18 files changed, 352 insertions(+), 56 deletions(-)

-- 
2.19.1

[PATCH v8 01/15] 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 cause the CPU to do a bus
lock 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 will be naturally aligned to unsigned long. But that needs additional
code changes. Adding __aligned(unsigned long) is a simpler fix.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
 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..3716e2bb028b 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];
+/* Aligned to unsigned long 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.19.1

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

[Fenghua Yu]

From: Peter Zijlstra <peterz@infradead.org>

A bit in pwol_mask is set in b44_magic_pattern() by atomic set_bit().
But since pwol_mask is local and never exposed to concurrency, there is
no need to set bit in pwol_mask atomically.

set_bit() sets the bit in a single unsigned long location. Because
pwol_mask may not be aligned to unsigned long, the location may cross two
cache lines. On x86, accessing two cache lines in locked instruction in
set_bit() is called split locked access and can cause overall performance
degradation.

So use non atomic __set_bit() to set pwol_mask bits. __set_bit() won't hit
split lock issue on x86.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 drivers/net/ethernet/broadcom/b44.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 97ab0dd25552..5738ab963dfb 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -1520,7 +1520,7 @@ static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
 
 	memset(ppattern + offset, 0xff, magicsync);
 	for (j = 0; j < magicsync; j++)
-		set_bit(len++, (unsigned long *) pmask);
+		__set_bit(len++, (unsigned long *)pmask);
 
 	for (j = 0; j < B44_MAX_PATTERNS; j++) {
 		if ((B44_PATTERN_SIZE - len) >= ETH_ALEN)
@@ -1532,7 +1532,7 @@ static int b44_magic_pattern(u8 *macaddr, u8 *ppattern, u8 *pmask, int offset)
 		for (k = 0; k< ethaddr_bytes; k++) {
 			ppattern[offset + magicsync +
 				(j * ETH_ALEN) + k] = macaddr[k];
-			set_bit(len++, (unsigned long *) pmask);
+			__set_bit(len++, (unsigned long *)pmask);
 		}
 	}
 	return len - 1;
-- 
2.19.1

[PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations

[Fenghua Yu]

From: Paolo Bonzini <pbonzini@redhat.com>

Bitmaps are defined on unsigned longs, so the usage of u32[2] in the
wlcore driver is incorrect.  As noted by Peter Zijlstra, casting arrays
to a bitmap is incorrect for big-endian architectures.

When looking at it I observed that:

- operations on reg_ch_conf_pending is always under the wl_lock mutex,
so set_bit is overkill

- the only case where reg_ch_conf_pending is accessed a u32 at a time is
unnecessary too.

This patch cleans up everything in this area, and changes tmp_ch_bitmap
to have the proper alignment.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 drivers/net/wireless/ti/wlcore/cmd.c    | 15 ++++++---------
 drivers/net/wireless/ti/wlcore/wlcore.h |  4 ++--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c
index 348be0aed97e..0415a064f6e2 100644
--- a/drivers/net/wireless/ti/wlcore/cmd.c
+++ b/drivers/net/wireless/ti/wlcore/cmd.c
@@ -1700,14 +1700,14 @@ void wlcore_set_pending_regdomain_ch(struct wl1271 *wl, u16 channel,
 	ch_bit_idx = wlcore_get_reg_conf_ch_idx(band, channel);
 
 	if (ch_bit_idx >= 0 && ch_bit_idx <= WL1271_MAX_CHANNELS)
-		set_bit(ch_bit_idx, (long *)wl->reg_ch_conf_pending);
+		__set_bit_le(ch_bit_idx, (long *)wl->reg_ch_conf_pending);
 }
 
 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];
+	__le32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));
 	struct wiphy *wiphy = wl->hw->wiphy;
 	struct ieee80211_supported_band *band;
 	bool timeout = false;
@@ -1717,7 +1717,7 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
 
 	wl1271_debug(DEBUG_CMD, "cmd reg domain config");
 
-	memset(tmp_ch_bitmap, 0, sizeof(tmp_ch_bitmap));
+	memcpy(tmp_ch_bitmap, wl->reg_ch_conf_pending, sizeof(tmp_ch_bitmap));
 
 	for (b = NL80211_BAND_2GHZ; b <= NL80211_BAND_5GHZ; b++) {
 		band = wiphy->bands[b];
@@ -1738,13 +1738,10 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
 			if (ch_bit_idx < 0)
 				continue;
 
-			set_bit(ch_bit_idx, (long *)tmp_ch_bitmap);
+			__set_bit_le(ch_bit_idx, (long *)tmp_ch_bitmap);
 		}
 	}
 
-	tmp_ch_bitmap[0] |= wl->reg_ch_conf_pending[0];
-	tmp_ch_bitmap[1] |= wl->reg_ch_conf_pending[1];
-
 	if (!memcmp(tmp_ch_bitmap, wl->reg_ch_conf_last, sizeof(tmp_ch_bitmap)))
 		goto out;
 
@@ -1754,8 +1751,8 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
 		goto out;
 	}
 
-	cmd->ch_bit_map1 = cpu_to_le32(tmp_ch_bitmap[0]);
-	cmd->ch_bit_map2 = cpu_to_le32(tmp_ch_bitmap[1]);
+	cmd->ch_bit_map1 = tmp_ch_bitmap[0];
+	cmd->ch_bit_map2 = tmp_ch_bitmap[1];
 	cmd->dfs_region = wl->dfs_region;
 
 	wl1271_debug(DEBUG_CMD,
diff --git a/drivers/net/wireless/ti/wlcore/wlcore.h b/drivers/net/wireless/ti/wlcore/wlcore.h
index dd14850b0603..870eea3e7a27 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore.h
@@ -320,9 +320,9 @@ struct wl1271 {
 	bool watchdog_recovery;
 
 	/* Reg domain last configuration */
-	u32 reg_ch_conf_last[2]  __aligned(8);
+	DECLARE_BITMAP(reg_ch_conf_last, 64);
 	/* Reg domain pending configuration */
-	u32 reg_ch_conf_pending[2];
+	DECLARE_BITMAP(reg_ch_conf_pending, 64);
 
 	/* Pointer that holds DMA-friendly block for the mailbox */
 	void *mbox;
-- 
2.19.1

[PATCH v8 04/15] 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 be aligned to unsigned long,
the single unsigned long location may cross two cache lines and
accessing the location by locked BTS/BTR introductions will cause
split lock.

To fix the split lock issue, align x86_capability to size of 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 size of unsigned long.
But this needs additional code changes. So choose the simpler solution
by setting the array's alignment to size of 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 2bb3a648fc12..7c62b9ad6e5a 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];
+	/* Aligned to size of unsigned long to avoid split lock in atomic 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.19.1

[PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Fenghua Yu]

A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
enumerates a model specific feature. Currently bit 5 enumerates split
lock detection. When bit 5 is 1, split lock detection is supported.
When the bit is 0, split lock detection is not supported.

Please check the latest Intel 64 and IA-32 Architectures Software
Developer's Manual for more detailed information on the MSR and the
split lock detection bit.

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

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index ca5bc0eacb95..f65ef6f783d2 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -59,6 +59,9 @@
 #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
 #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
 
+#define MSR_IA32_CORE_CAPABILITY	0x000000cf
+#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
+
 #define MSR_PKG_CST_CONFIG_CONTROL	0x000000e2
 #define NHM_C3_AUTO_DEMOTE		(1UL << 25)
 #define NHM_C1_AUTO_DEMOTE		(1UL << 26)
-- 
2.19.1

[PATCH v8 06/15] x86/cpufeatures: Enumerate MSR_IA32_CORE_CAPABILITY

[Fenghua Yu]

MSR_IA32_CORE_CAPABILITY (0xcf) contains bits that enumerate some model
specific features.

The MSR 0xcf itself is enumerated by CPUID.(EAX=0x7,ECX=0):EDX[30].
When this CPUID bit is 1, the MSR 0xcf exists.

Detailed information on the CPUID bit and the MSR can be found in the
latest Intel 64 and IA-32 Architectures Software Developer's Manual.

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 981ff9479648..eff25e2015a5 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -350,6 +350,7 @@
 #define X86_FEATURE_INTEL_STIBP		(18*32+27) /* "" Single Thread Indirect Branch Predictors */
 #define X86_FEATURE_FLUSH_L1D		(18*32+28) /* Flush L1D cache */
 #define X86_FEATURE_ARCH_CAPABILITIES	(18*32+29) /* IA32_ARCH_CAPABILITIES MSR (Intel) */
+#define X86_FEATURE_CORE_CAPABILITY	(18*32+30) /* "" IA32_CORE_CAPABILITY MSR */
 #define X86_FEATURE_SPEC_CTRL_SSBD	(18*32+31) /* "" Speculative Store Bypass Disable */
 
 /*
-- 
2.19.1

[PATCH v8 07/15] x86/split_lock: Enumerate split lock detection by MSR_IA32_CORE_CAPABILITY

[Fenghua Yu]

Bits in MSR_IA32_CORE_CAPABILITY enumerate a few features that are not
enumerated through CPUID. Currently bit 5 is defined to enumerate
feature of split lock detection. All other bits are reserved now.

When bit 5 is 1, the feature is supported and feature bit
X86_FEATURE_SPLIT_LOCK_DETECT is set. Otherwise, the feature is not
available.

The MSR_IA32_CORE_CAPABILITY itself is enumerated by
CPUID.(EAX=0x7,ECX=0):EDX[30].

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpu.h         |  5 ++
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/kernel/cpu/common.c       |  2 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 79 +++++++++++++++---------------
 arch/x86/kernel/cpu/intel.c        | 21 ++++++++
 5 files changed, 69 insertions(+), 39 deletions(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index adc6cc86b062..4e03f53fc079 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -40,4 +40,9 @@ 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);
+#ifdef CONFIG_CPU_SUP_INTEL
+void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
+#else
+static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
+#endif
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index eff25e2015a5..db0c1826d7ad 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_SPLIT_LOCK_DETECT	( 7*32+31) /* #AC for split lock */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 3716e2bb028b..bbdd69dd4f5f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1105,6 +1105,8 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
 	cpu_set_bug_bits(c);
 
+	cpu_set_core_cap_bits(c);
+
 	fpu__init_system(c);
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 2c0bd38a44ab..3d633f67fbd7 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -20,45 +20,46 @@ struct cpuid_dep {
  * but it's difficult to tell that to the init reference checker.
  */
 static const struct cpuid_dep cpuid_deps[] = {
-	{ X86_FEATURE_XSAVEOPT,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_XSAVEC,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_XSAVES,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_AVX,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_PKU,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_MPX,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_XGETBV1,		X86_FEATURE_XSAVE     },
-	{ X86_FEATURE_FXSR_OPT,		X86_FEATURE_FXSR      },
-	{ X86_FEATURE_XMM,		X86_FEATURE_FXSR      },
-	{ X86_FEATURE_XMM2,		X86_FEATURE_XMM       },
-	{ X86_FEATURE_XMM3,		X86_FEATURE_XMM2      },
-	{ X86_FEATURE_XMM4_1,		X86_FEATURE_XMM2      },
-	{ X86_FEATURE_XMM4_2,		X86_FEATURE_XMM2      },
-	{ X86_FEATURE_XMM3,		X86_FEATURE_XMM2      },
-	{ X86_FEATURE_PCLMULQDQ,	X86_FEATURE_XMM2      },
-	{ X86_FEATURE_SSSE3,		X86_FEATURE_XMM2,     },
-	{ X86_FEATURE_F16C,		X86_FEATURE_XMM2,     },
-	{ X86_FEATURE_AES,		X86_FEATURE_XMM2      },
-	{ X86_FEATURE_SHA_NI,		X86_FEATURE_XMM2      },
-	{ X86_FEATURE_FMA,		X86_FEATURE_AVX       },
-	{ X86_FEATURE_AVX2,		X86_FEATURE_AVX,      },
-	{ X86_FEATURE_AVX512F,		X86_FEATURE_AVX,      },
-	{ X86_FEATURE_AVX512IFMA,	X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512PF,		X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512ER,		X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512CD,		X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512DQ,		X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512BW,		X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512VL,		X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512VBMI,	X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512_VBMI2,	X86_FEATURE_AVX512VL  },
-	{ X86_FEATURE_GFNI,		X86_FEATURE_AVX512VL  },
-	{ X86_FEATURE_VAES,		X86_FEATURE_AVX512VL  },
-	{ X86_FEATURE_VPCLMULQDQ,	X86_FEATURE_AVX512VL  },
-	{ X86_FEATURE_AVX512_VNNI,	X86_FEATURE_AVX512VL  },
-	{ X86_FEATURE_AVX512_BITALG,	X86_FEATURE_AVX512VL  },
-	{ X86_FEATURE_AVX512_4VNNIW,	X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512_4FMAPS,	X86_FEATURE_AVX512F   },
-	{ X86_FEATURE_AVX512_VPOPCNTDQ, X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_XSAVEOPT,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVEC,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVES,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_AVX,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_PKU,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_MPX,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XGETBV1,			X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_FXSR_OPT,			X86_FEATURE_FXSR      },
+	{ X86_FEATURE_XMM,			X86_FEATURE_FXSR      },
+	{ X86_FEATURE_XMM2,			X86_FEATURE_XMM       },
+	{ X86_FEATURE_XMM3,			X86_FEATURE_XMM2      },
+	{ X86_FEATURE_XMM4_1,			X86_FEATURE_XMM2      },
+	{ X86_FEATURE_XMM4_2,			X86_FEATURE_XMM2      },
+	{ X86_FEATURE_XMM3,			X86_FEATURE_XMM2      },
+	{ X86_FEATURE_PCLMULQDQ,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_SSSE3,			X86_FEATURE_XMM2,     },
+	{ X86_FEATURE_F16C,			X86_FEATURE_XMM2,     },
+	{ X86_FEATURE_AES,			X86_FEATURE_XMM2      },
+	{ X86_FEATURE_SHA_NI,			X86_FEATURE_XMM2      },
+	{ X86_FEATURE_FMA,			X86_FEATURE_AVX       },
+	{ X86_FEATURE_AVX2,			X86_FEATURE_AVX,      },
+	{ X86_FEATURE_AVX512F,			X86_FEATURE_AVX,      },
+	{ X86_FEATURE_AVX512IFMA,		X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512PF,			X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512ER,			X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512CD,			X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512DQ,			X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512BW,			X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512VL,			X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512VBMI,		X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512_VBMI2,		X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_GFNI,			X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_VAES,			X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_VPCLMULQDQ,		X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_AVX512_VNNI,		X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_AVX512_BITALG,		X86_FEATURE_AVX512VL  },
+	{ X86_FEATURE_AVX512_4VNNIW,		X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512_4FMAPS,		X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512_VPOPCNTDQ,		X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_SPLIT_LOCK_DETECT,	X86_FEATURE_CORE_CAPABILITY},
 	{}
 };
 
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 3142fd7a9b32..86a3a646e0ce 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1029,3 +1029,24 @@ static const struct cpu_dev intel_cpu_dev = {
 
 cpu_dev_register(intel_cpu_dev);
 
+static void __init set_split_lock_detect(void)
+{
+	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
+}
+
+void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
+{
+	u64 ia32_core_cap = 0;
+
+	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITY))
+		return;
+
+	/*
+	 * If MSR_IA32_CORE_CAPABILITY exists, enumerate features that are
+	 * reported in the MSR.
+	 */
+	rdmsrl(MSR_IA32_CORE_CAPABILITY, ia32_core_cap);
+
+	if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT)
+		set_split_lock_detect();
+}
-- 
2.19.1

[PATCH v8 08/15] x86/split_lock: Enumerate split lock detection on Icelake mobile processor

[Fenghua Yu]

Icelake mobile processor can detect split lock operations although
the processor doesn't have MSR IA32_CORE_CAPABILITY and split lock
detection bit in the MSR. Set split lock detection feature bit
X86_FEATURE_SPLIT_LOCK_DETECT on the processor based on its
family/model/stepping.

In the future, a few other processors may also have the split lock
detection feature but don't have MSR IA32_CORE_CAPABILITY. The feature
will be enumerated on those processors once their family/model/stepping
information is released.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 86a3a646e0ce..d7e676c2aebf 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1038,8 +1038,18 @@ void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 {
 	u64 ia32_core_cap = 0;
 
-	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITY))
+	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITY)) {
+		/*
+		 * The following processors have split lock detection feature.
+		 * But since they don't have MSR IA32_CORE_CAPABILITY, the
+		 * feature cannot be enumerated by the MSR. So enumerate the
+		 * feature by family/model/stepping.
+		 */
+		if (c->x86 == 6 && c->x86_model == INTEL_FAM6_ICELAKE_MOBILE)
+			set_split_lock_detect();
+
 		return;
+	}
 
 	/*
 	 * If MSR_IA32_CORE_CAPABILITY exists, enumerate features that are
-- 
2.19.1

[PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register

[Fenghua Yu]

Setting bit 29 in MSR TEST_CTL (0x33) enables split lock detection and
clearing the bit disables split lock detection.

Define the MSR and the bit. The definitions will be used in enabling or
disabling split lock detection.

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

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index f65ef6f783d2..296eeb761ab6 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_SPLIT_LOCK_DETECT_SHIFT	29
+#define TEST_CTL_SPLIT_LOCK_DETECT		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 */
-- 
2.19.1

[PATCH v8 10/15] 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, a user
application developer needs to fix the split lock issue.

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

A kernel/driver developer should check the warning, which contains helpful
faulting address, context, and callstack info, and fix the split lock
issues. Then further split lock issues may be captured and fixed.

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

MSR TEST_CTL value is cached in per CPU msr_test_ctl_cache which will be
used in virtualization to avoid costly MSR read.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpu.h  |  3 +++
 arch/x86/kernel/cpu/intel.c | 24 ++++++++++++++++++++++++
 arch/x86/kernel/traps.c     | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 4e03f53fc079..5706461eb60f 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -42,7 +42,10 @@ unsigned int x86_model(unsigned int sig);
 unsigned int x86_stepping(unsigned int sig);
 #ifdef CONFIG_CPU_SUP_INTEL
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
+DECLARE_PER_CPU(u64, msr_test_ctl_cache);
+void handle_split_lock_kernel_mode(void);
 #else
 static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
+static inline void handle_split_lock_kernel_mode(void) {}
 #endif
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index d7e676c2aebf..2cc69217ca7c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -31,6 +31,9 @@
 #include <asm/apic.h>
 #endif
 
+DEFINE_PER_CPU(u64, msr_test_ctl_cache);
+EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
+
 /*
  * Just in case our CPU detection goes bad, or you have a weird system,
  * allow a way to override the automatic disabling of MPX.
@@ -654,6 +657,17 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
 	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
 }
 
+static void init_split_lock_detect(struct cpuinfo_x86 *c)
+{
+	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
+		u64 test_ctl_val;
+
+		/* Cache MSR TEST_CTL */
+		rdmsrl(MSR_TEST_CTL, test_ctl_val);
+		this_cpu_write(msr_test_ctl_cache, test_ctl_val);
+	}
+}
+
 static void init_intel(struct cpuinfo_x86 *c)
 {
 	early_init_intel(c);
@@ -766,6 +780,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 	init_intel_energy_perf(c);
 
 	init_intel_misc_features(c);
+
+	init_split_lock_detect(c);
 }
 
 #ifdef CONFIG_X86_32
@@ -1060,3 +1076,11 @@ void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 	if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT)
 		set_split_lock_detect();
 }
+
+void handle_split_lock_kernel_mode(void)
+{
+	/* Warn and disable split lock detection on this CPU */
+	msr_clear_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+	this_cpu_and(msr_test_ctl_cache, ~TEST_CTL_SPLIT_LOCK_DETECT);
+	WARN_ONCE(1, "split lock operation detected\n");
+}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d26f9e9c3d83..db6b18311dbc 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>
@@ -293,9 +294,37 @@ 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;
+
+	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)
+		return;
+
+	cond_local_irq_enable(regs);
+	if (!user_mode(regs) &&
+	    static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
+		/*
+		 * Only split lock can generate #AC from kernel at this point.
+		 * Warn and disable split lock detection on this CPU. The
+		 * faulting instruction will be executed without generating
+		 * another #AC fault.
+		 */
+		return handle_split_lock_kernel_mode();
+	}
+
+	/* Handle #AC generated in any other cases. */
+	do_trap(X86_TRAP_AC, SIGBUS, "alignment check", regs,
+		error_code, BUS_ADRALN, NULL);
+}
+
 #ifdef CONFIG_VMAP_STACK
 __visible void __noreturn handle_stack_overflow(const char *message,
 						struct pt_regs *regs,
-- 
2.19.1

[PATCH v8 11/15] kvm/x86: Emulate MSR IA32_CORE_CAPABILITY

[Fenghua Yu]

From: Xiaoyao Li <xiaoyao.li@linux.intel.com>

MSR IA32_CORE_CAPABILITY is a feature-enumerating MSR, bit 5 of which
reports the capability of enabling detection of split locks (will be
supported on future processors based on Tremont microarchitecture and
later).

CPUID.(EAX=7H,ECX=0):EDX[30] enumerates the presence of the
IA32_CORE_CAPABILITY MSR.

Please check the latest Intel 64 and IA-32 Architectures Software
Developer's Manual for more detailed information on the MSR and
the split lock bit.

Since MSR_IA32_CORE_CAPABILITY is a feature-enumerating MSR that plays the
similar role as CPUID, it can be emulated in software regardless of host's
capability. What we need to do is to set the right value of it to report
the capability of guest.

In this patch, just set the guest's core_capability as 0, because we
haven't added support of the features it indicates to guest. It's for
bisectability.

Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
Changes in v7:
  - make kvm_get_core_capability() static since it's only used in this
  file.

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            |  6 ++++++
 arch/x86/kvm/x86.c              | 22 ++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a9d03af34030..d4f9b13fcdd6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -570,6 +570,7 @@ struct kvm_vcpu_arch {
 	u64 ia32_xss;
 	u64 microcode_version;
 	u64 arch_capabilities;
+	u64 core_capability;
 
 	/*
 	 * Paging state of the vcpu
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index fd3951638ae4..4a2f7892ea31 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -505,6 +505,12 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 			 * if the host doesn't support it.
 			 */
 			entry->edx |= F(ARCH_CAPABILITIES);
+			/*
+			 * Since we emulate MSR IA32_CORE_CAPABILITY in
+			 * software, we can always enable it for guest
+			 * regardless of host's capability.
+			 */
+			entry->edx |= F(CORE_CAPABILITY);
 		} else {
 			entry->ebx = 0;
 			entry->ecx = 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a0d1fc80ac5a..e88be97d47b9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1161,6 +1161,7 @@ static u32 emulated_msrs[] = {
 	MSR_IA32_TSC_ADJUST,
 	MSR_IA32_TSCDEADLINE,
 	MSR_IA32_ARCH_CAPABILITIES,
+	MSR_IA32_CORE_CAPABILITY,
 	MSR_IA32_MISC_ENABLE,
 	MSR_IA32_MCG_STATUS,
 	MSR_IA32_MCG_CTL,
@@ -1200,6 +1201,7 @@ static u32 msr_based_features[] = {
 
 	MSR_F10H_DECFG,
 	MSR_IA32_UCODE_REV,
+	MSR_IA32_CORE_CAPABILITY,
 	MSR_IA32_ARCH_CAPABILITIES,
 };
 
@@ -1227,9 +1229,17 @@ u64 kvm_get_arch_capabilities(void)
 }
 EXPORT_SYMBOL_GPL(kvm_get_arch_capabilities);
 
+static u64 kvm_get_core_capability(void)
+{
+	return 0;
+}
+
 static int kvm_get_msr_feature(struct kvm_msr_entry *msr)
 {
 	switch (msr->index) {
+	case MSR_IA32_CORE_CAPABILITY:
+		msr->data = kvm_get_core_capability();
+		break;
 	case MSR_IA32_ARCH_CAPABILITIES:
 		msr->data = kvm_get_arch_capabilities();
 		break;
@@ -2453,6 +2463,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_EFER:
 		return set_efer(vcpu, data);
+	case MSR_IA32_CORE_CAPABILITY:
+		if (!msr_info->host_initiated)
+			return 1;
+		vcpu->arch.core_capability = data;
+		break;
 	case MSR_K7_HWCR:
 		data &= ~(u64)0x40;	/* ignore flush filter disable */
 		data &= ~(u64)0x100;	/* ignore ignne emulation enable */
@@ -2764,6 +2779,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_TSC:
 		msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset;
 		break;
+	case MSR_IA32_CORE_CAPABILITY:
+		if (!msr_info->host_initiated &&
+		    !guest_cpuid_has(vcpu, X86_FEATURE_CORE_CAPABILITY))
+			return 1;
+		msr_info->data = vcpu->arch.core_capability;
+		break;
 	case MSR_MTRRcap:
 	case 0x200 ... 0x2ff:
 		return kvm_mtrr_get_msr(vcpu, msr_info->index, &msr_info->data);
@@ -8760,6 +8781,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.arch_capabilities = kvm_get_arch_capabilities();
+	vcpu->arch.core_capability = kvm_get_core_capability();
 	vcpu->arch.msr_platform_info = MSR_PLATFORM_INFO_CPUID_FAULT;
 	kvm_vcpu_mtrr_init(vcpu);
 	vcpu_load(vcpu);
-- 
2.19.1

[PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Fenghua Yu]

From: Xiaoyao Li <xiaoyao.li@linux.intel.com>

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

Please check the latest Intel 64 and IA-32 Architectures Software
Developer's Manual for more detailed information on the MSR and
the split lock bit.

This patch emulates MSR_TEST_CTL with vmx->msr_test_ctl and does the
following:
1. As MSR TEST_CTL of guest is emulated, enable the related bit
in CORE_CAPABILITY to correctly report this feature to guest.

2. Differentiate MSR_TEST_CTL between host and guest.

To avoid costly RDMSR of TEST_CTL when switching between host and guest
during vmentry, read per CPU variable msr_test_ctl_cache which caches
the MSR value.

Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
Changes in v7:
  - Add vmx->msr_test_ctl_mask to indicate the valid bits of
  guest's MSR_TEST_CTL.
  - Add X86_FEATURE_SPLIT_LOCK_DETECT check to determine if it needs
  switch MSR_TEST_CTL.
  - Use msr_test_ctl_cache to replace costly RDMSR.
  - minimal adjustment in kvm_get_core_capability(), making it more
  clear.

 arch/x86/kvm/vmx/vmx.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.h |  2 ++
 arch/x86/kvm/x86.c     | 19 ++++++++++++++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b4e7d645275a..bbb9859350b5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1663,6 +1663,11 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u32 index;
 
 	switch (msr_info->index) {
+	case MSR_TEST_CTL:
+		if (!vmx->msr_test_ctl_mask)
+			return 1;
+		msr_info->data = vmx->msr_test_ctl;
+		break;
 #ifdef CONFIG_X86_64
 	case MSR_FS_BASE:
 		msr_info->data = vmcs_readl(GUEST_FS_BASE);
@@ -1797,6 +1802,12 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u32 index;
 
 	switch (msr_index) {
+	case MSR_TEST_CTL:
+		if (!vmx->msr_test_ctl_mask ||
+		    (data & vmx->msr_test_ctl_mask) != data)
+			return 1;
+		vmx->msr_test_ctl = data;
+		break;
 	case MSR_EFER:
 		ret = kvm_set_msr_common(vcpu, msr_info);
 		break;
@@ -4106,6 +4117,16 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
 	}
 }
 
+static u64 vmx_get_msr_test_ctl_mask(struct kvm_vcpu *vcpu)
+{
+	u64 mask = 0;
+
+	if (vcpu->arch.core_capability & CORE_CAP_SPLIT_LOCK_DETECT)
+		mask |= TEST_CTL_SPLIT_LOCK_DETECT;
+
+	return mask;
+}
+
 static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -4114,6 +4135,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 
 	vmx->rmode.vm86_active = 0;
 	vmx->spec_ctrl = 0;
+	vmx->msr_test_ctl = 0;
+	vmx->msr_test_ctl_mask = vmx_get_msr_test_ctl_mask(vcpu);
 
 	vcpu->arch.microcode_version = 0x100000000ULL;
 	vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
@@ -6313,6 +6336,23 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
 					msrs[i].host, false);
 }
 
+static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
+{
+	u64 host_msr_test_ctl;
+
+	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
+		return;
+
+	host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
+
+	if (host_msr_test_ctl == vmx->msr_test_ctl) {
+		clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
+	} else {
+		add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
+				      host_msr_test_ctl, false);
+	}
+}
+
 static void vmx_arm_hv_timer(struct vcpu_vmx *vmx, u32 val)
 {
 	vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, val);
@@ -6421,6 +6461,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
 	atomic_switch_perf_msrs(vmx);
 
+	atomic_switch_msr_test_ctl(vmx);
+
 	vmx_update_hv_timer(vcpu);
 
 	/*
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index f879529906b4..8690a1295548 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -190,6 +190,8 @@ struct vcpu_vmx {
 	u64		      msr_guest_kernel_gs_base;
 #endif
 
+	u64		      msr_test_ctl;
+	u64		      msr_test_ctl_mask;
 	u64		      spec_ctrl;
 
 	u32 vm_entry_controls_shadow;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e88be97d47b9..60aaf75d0fe5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1231,7 +1231,24 @@ EXPORT_SYMBOL_GPL(kvm_get_arch_capabilities);
 
 static u64 kvm_get_core_capability(void)
 {
-	return 0;
+	u64 data = 0;
+
+	if (boot_cpu_has(X86_FEATURE_CORE_CAPABILITY)) {
+		rdmsrl(MSR_IA32_CORE_CAPABILITY, data);
+
+		/* mask non-virtualizable functions */
+		data &= CORE_CAP_SPLIT_LOCK_DETECT;
+	} else if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
+		/*
+		 * There will be a list of FMS values that have split lock
+		 * detection but lack the CORE CAPABILITY MSR. In this case,
+		 * set CORE_CAP_SPLIT_LOCK_DETECT since we emulate
+		 * MSR CORE_CAPABILITY.
+		 */
+		data |= CORE_CAP_SPLIT_LOCK_DETECT;
+	}
+
+	return data;
 }
 
 static int kvm_get_msr_feature(struct kvm_msr_entry *msr)
-- 
2.19.1

[PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Fenghua Yu]

A split locked access locks bus and degrades overall memory access
performance. When split lock detection feature is enumerated, enable
the feature by default by writing 1 to bit 29 in MSR TEST_CTL to find
any split lock issue.

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

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 2cc69217ca7c..28cc6891ba48 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -657,6 +657,13 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
 	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
 }
 
+static void split_lock_update_msr(void)
+{
+	/* Enable split lock detection */
+	msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+	this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
+}
+
 static void init_split_lock_detect(struct cpuinfo_x86 *c)
 {
 	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
@@ -665,6 +672,8 @@ static void init_split_lock_detect(struct cpuinfo_x86 *c)
 		/* Cache MSR TEST_CTL */
 		rdmsrl(MSR_TEST_CTL, test_ctl_val);
 		this_cpu_write(msr_test_ctl_cache, test_ctl_val);
+
+		split_lock_update_msr();
 	}
 }
 
@@ -1045,9 +1054,13 @@ static const struct cpu_dev intel_cpu_dev = {
 
 cpu_dev_register(intel_cpu_dev);
 
+#undef pr_fmt
+#define pr_fmt(fmt) "x86/split lock detection: " fmt
+
 static void __init set_split_lock_detect(void)
 {
 	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
+	pr_info("enabled\n");
 }
 
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
-- 
2.19.1

[PATCH v8 14/15] x86/split_lock: Disable split lock detection by kernel parameter "nosplit_lock_detect"

[Fenghua Yu]

To work around or debug split lock issues, the kernel parameter
"nosplit_lock_detect" is introduced to disable the feature during boot
time.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 .../admin-guide/kernel-parameters.txt         |  2 ++
 arch/x86/kernel/cpu/intel.c                   | 25 ++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2b8ee90bb644..623a5f223ff1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3019,6 +3019,8 @@
 
 	nosoftlockup	[KNL] Disable the soft-lockup detector.
 
+	nosplit_lock_detect	[X86] Disable split lock detection
+
 	nosync		[HW,M68K] Disables sync negotiation for all devices.
 
 	nowatchdog	[KNL] Disable both lockup detectors, i.e.
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 28cc6891ba48..959ebf25beda 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -19,6 +19,7 @@
 #include <asm/microcode_intel.h>
 #include <asm/hwcap2.h>
 #include <asm/elf.h>
+#include <asm/cmdline.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>
@@ -34,6 +35,8 @@
 DEFINE_PER_CPU(u64, msr_test_ctl_cache);
 EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
 
+static bool split_lock_detect_enable;
+
 /*
  * Just in case our CPU detection goes bad, or you have a weird system,
  * allow a way to override the automatic disabling of MPX.
@@ -659,9 +662,15 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
 
 static void split_lock_update_msr(void)
 {
-	/* Enable split lock detection */
-	msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
-	this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
+	if (split_lock_detect_enable) {
+		/* Enable split lock detection */
+		msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+		this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
+	} else {
+		/* Disable split lock detection */
+		msr_clear_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+		this_cpu_and(msr_test_ctl_cache, ~TEST_CTL_SPLIT_LOCK_DETECT);
+	}
 }
 
 static void init_split_lock_detect(struct cpuinfo_x86 *c)
@@ -1060,7 +1069,15 @@ cpu_dev_register(intel_cpu_dev);
 static void __init set_split_lock_detect(void)
 {
 	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
-	pr_info("enabled\n");
+
+	if (cmdline_find_option_bool(boot_command_line,
+				     "nosplit_lock_detect")) {
+		split_lock_detect_enable = false;
+		pr_info("disabled\n");
+	} else {
+		split_lock_detect_enable = true;
+		pr_info("enabled\n");
+	}
 }
 
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
-- 
2.19.1

[PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time

[Fenghua Yu]

To workaround or debug a split lock issue, the administrator may need to
disable or enable split lock detection during run time without rebooting
the system.

The interface /sys/device/system/cpu/split_lock_detect is added to allow
the administrator to disable or enable split lock detection and show
current split lock detection setting.

Writing [yY1] or [oO][nN] to the file enables split lock detection and
writing [nN0] or [oO][fF] disables split lock detection. Split lock
detection is enabled or disabled on all CPUs.

Reading the file returns current global split lock detection setting:
0: disabled
1: enabled

Add an ABI document entry for /sys/devices/system/cpu/split_lock_detect.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
Not sure if the justification for the sysfs knob is valid. If not, this
patch could be removed from this patch set.

 .../ABI/testing/sysfs-devices-system-cpu      | 22 ++++++++
 arch/x86/kernel/cpu/intel.c                   | 52 ++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 9605dbd4b5b5..aad7b1698065 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -67,6 +67,28 @@ Description:	Discover NUMA node a CPU belongs to
 		/sys/devices/system/cpu/cpu42/node2 -> ../../node/node2
 
 
+What:		/sys/devices/system/cpu/split_lock_detect
+Date:		March 2019
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+Description:	(RW) Control split lock detection on Intel Tremont and
+		future CPUs
+
+		Reads return split lock detection status:
+			0: disabled
+			1: enabled
+
+		Writes enable or disable split lock detection:
+			The first character is one of 'Nn0' or [oO][fF] for off
+			disables the feature.
+			The first character is one of 'Yy1' or [oO][nN] for on
+			enables the feature.
+
+		Please note the interface only shows or controls global setting.
+		During run time, split lock detection on one CPU may be
+		disabled if split lock operation in kernel code happens on
+		the CPU. The interface doesn't show or control split lock
+		detection on individual CPU.
+
 What:		/sys/devices/system/cpu/cpu#/topology/core_id
 		/sys/devices/system/cpu/cpu#/topology/core_siblings
 		/sys/devices/system/cpu/cpu#/topology/core_siblings_list
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 959ebf25beda..f257d1e92706 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -35,6 +35,7 @@
 DEFINE_PER_CPU(u64, msr_test_ctl_cache);
 EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
 
+static DEFINE_MUTEX(split_lock_detect_mutex);
 static bool split_lock_detect_enable;
 
 /*
@@ -660,7 +661,7 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
 	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
 }
 
-static void split_lock_update_msr(void)
+static void split_lock_update_msr(void *__unused)
 {
 	if (split_lock_detect_enable) {
 		/* Enable split lock detection */
@@ -682,7 +683,7 @@ static void init_split_lock_detect(struct cpuinfo_x86 *c)
 		rdmsrl(MSR_TEST_CTL, test_ctl_val);
 		this_cpu_write(msr_test_ctl_cache, test_ctl_val);
 
-		split_lock_update_msr();
+		split_lock_update_msr(NULL);
 	}
 }
 
@@ -1114,3 +1115,50 @@ void handle_split_lock_kernel_mode(void)
 	this_cpu_and(msr_test_ctl_cache, ~TEST_CTL_SPLIT_LOCK_DETECT);
 	WARN_ONCE(1, "split lock operation detected\n");
 }
+
+static ssize_t
+split_lock_detect_show(struct device *dev, struct device_attribute *attr,
+		       char *buf)
+{
+	return sprintf(buf, "%u\n", split_lock_detect_enable);
+}
+
+static ssize_t
+split_lock_detect_store(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	bool val;
+	int ret;
+
+	ret = strtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&split_lock_detect_mutex);
+
+	split_lock_detect_enable = val;
+
+	/* Update the split lock detection setting in MSR on all online CPUs. */
+	on_each_cpu(split_lock_update_msr, NULL, 1);
+
+	if (split_lock_detect_enable)
+		pr_info("enabled\n");
+	else
+		pr_info("disabled\n");
+
+	mutex_unlock(&split_lock_detect_mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(split_lock_detect);
+
+static int __init split_lock_init(void)
+{
+	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
+		return -ENODEV;
+
+	return device_create_file(cpu_subsys.dev_root,
+				  &dev_attr_split_lock_detect);
+}
+subsys_initcall(split_lock_init);
-- 
2.19.1

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Ingo Molnar]

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

> A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> enumerates a model specific feature. Currently bit 5 enumerates split
> lock detection. When bit 5 is 1, split lock detection is supported.
> When the bit is 0, split lock detection is not supported.
> 
> Please check the latest Intel 64 and IA-32 Architectures Software
> Developer's Manual for more detailed information on the MSR and the
> split lock detection bit.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/include/asm/msr-index.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index ca5bc0eacb95..f65ef6f783d2 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -59,6 +59,9 @@
>  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
>  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
>  
> +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */

Please don't put comments into definitions.

Thanks,

	Ingo

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

[Ingo Molnar]

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

> 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, a user
> application developer needs to fix the split lock issue.
> 
> When #AC exception for split lock is triggered from a kernel instruction,
> disable split lock detection on local CPU and warn the split lock issue.
> After the exception, the faulting instruction will be executed and kernel
> execution continues. Split lock detection is only disabled on the local
> CPU, not globally. It will be re-enabled if the CPU is offline and then
> online or through sysfs interface.
> 
> A kernel/driver developer should check the warning, which contains helpful
> faulting address, context, and callstack info, and fix the split lock
> issues. Then further split lock issues may be captured and fixed.
> 
> After bit 29 in MSR_TEST_CTL is set to 1 in kernel, firmware inherits
> the setting when firmware is executed in S4, S5, run time services, SMI,
> etc. If there is a split lock operation in firmware, it will triggers
> #AC and may hang the system depending on how firmware handles the #AC.
> It's up to a firmware developer to fix split lock issues in firmware.
> 
> MSR TEST_CTL value is cached in per CPU msr_test_ctl_cache which will be
> used in virtualization to avoid costly MSR read.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/include/asm/cpu.h  |  3 +++
>  arch/x86/kernel/cpu/intel.c | 24 ++++++++++++++++++++++++
>  arch/x86/kernel/traps.c     | 31 ++++++++++++++++++++++++++++++-
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index 4e03f53fc079..5706461eb60f 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -42,7 +42,10 @@ unsigned int x86_model(unsigned int sig);
>  unsigned int x86_stepping(unsigned int sig);
>  #ifdef CONFIG_CPU_SUP_INTEL
>  void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
> +DECLARE_PER_CPU(u64, msr_test_ctl_cache);
> +void handle_split_lock_kernel_mode(void);
>  #else
>  static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
> +static inline void handle_split_lock_kernel_mode(void) {}
>  #endif
>  #endif /* _ASM_X86_CPU_H */
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index d7e676c2aebf..2cc69217ca7c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -31,6 +31,9 @@
>  #include <asm/apic.h>
>  #endif
>  
> +DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> +
>  /*
>   * Just in case our CPU detection goes bad, or you have a weird system,
>   * allow a way to override the automatic disabling of MPX.
> @@ -654,6 +657,17 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
>  	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
>  }
>  
> +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		u64 test_ctl_val;
> +
> +		/* Cache MSR TEST_CTL */
> +		rdmsrl(MSR_TEST_CTL, test_ctl_val);
> +		this_cpu_write(msr_test_ctl_cache, test_ctl_val);
> +	}
> +}
> +
>  static void init_intel(struct cpuinfo_x86 *c)
>  {
>  	early_init_intel(c);
> @@ -766,6 +780,8 @@ static void init_intel(struct cpuinfo_x86 *c)
>  	init_intel_energy_perf(c);
>  
>  	init_intel_misc_features(c);
> +
> +	init_split_lock_detect(c);
>  }
>  
>  #ifdef CONFIG_X86_32
> @@ -1060,3 +1076,11 @@ void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
>  	if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT)
>  		set_split_lock_detect();
>  }
> +
> +void handle_split_lock_kernel_mode(void)
> +{
> +	/* Warn and disable split lock detection on this CPU */
> +	msr_clear_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> +	this_cpu_and(msr_test_ctl_cache, ~TEST_CTL_SPLIT_LOCK_DETECT);
> +	WARN_ONCE(1, "split lock operation detected\n");

Please name this more descriptively, such as x86_split_lock_disable() or 
so.

Also, please reorganize the split lock detection namespace to be less 
idiosynchratic, use a common x86_split_lock_ prefix and organize the new 
namespace around that:

    x86_split_lock_init()          // was: init_split_lock_detect
    x86_split_lock_enable()        // was: set_split_lock_detect
    x86_split_lock_disable()       // was: handle_split_lock_kernel_mode
    etc.

> +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;
> +
> +	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)

Please do not break lines mid-line when it does absolutely nothing to 
improve readablity. Just ignore checkpatch.

> +	cond_local_irq_enable(regs);
> +	if (!user_mode(regs) &&
> +	    static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {

Ditto - in fact this doesn't even violate the col80 rule ...

> +		/*
> +		 * Only split lock can generate #AC from kernel at this point.
> +		 * Warn and disable split lock detection on this CPU. The
> +		 * faulting instruction will be executed without generating
> +		 * another #AC fault.
> +		 */

How about explaining all this more clearly:

> +		/*
> +		 * Only split locks can generate #AC from kernel mode.
> +              *
> +		 * The split-lock detection feature is a one-shot
> +              * debugging facility, so we disable it immediately and 
> +              * print a warning.
> +              *
> +              * This also solves the instruction restart problem: we 
> +              * return the faulting instruction right after this it 
> +              * will be executed without generating another #AC fault
> +              * and getting into an infinite loop, instead it will
> +              * continue without side effects to the interrupted
> +              * execution conext.
> +              *
> +              * Split-lock detection will remain disabled permanently
> +              * after this, until the next reboot.
> +		 */

?

Also, AFAICS this code will disable split-lock detection only on the 
current CPU - all the other 4,096 CPUs hitting this same lock at the 
exact same time will happily continue spamming the kernel log as they 
encounter the same split lock, correct?

While the warning itself uses WARN_ONCE(), that and the underlying 
BUGFLAG_ONCE mechanism is not an atomic facility.

Instead, please add an explicit, global split_lock_debug bit that the 
first CPU hitting it disables, and only that CPU is allowed to print a 
single warning. All other CPUs just disable split-lock debugging silently 
and continue.

This also solves the race if the split-lock #AC fault is re-triggered by 
NMI of perf context interrupting one split-lock warning execution while 
the original WARN_ON() is executing.

Thanks,

	Ingo

Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register

[Ingo Molnar]

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

> Setting bit 29 in MSR TEST_CTL (0x33) enables split lock detection and
> clearing the bit disables split lock detection.
> 
> Define the MSR and the bit. The definitions will be used in enabling or
> disabling split lock detection.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/include/asm/msr-index.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index f65ef6f783d2..296eeb761ab6 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_SPLIT_LOCK_DETECT_SHIFT	29
> +#define TEST_CTL_SPLIT_LOCK_DETECT		BIT(29)

Three problems:

 - Is MSR_TEST_CTL is not really a canonical MSR name... A quick look at 
   msr-index reveals the prevailing nomenclature:

     dagon:~/tip> git grep -h 'define MSR' arch/x86/include/asm/msr-index.h | cut -d_ -f1-2 | sort -n | uniq -c | sort -n | tail -10
       8 #define MSR_K8
       8 #define MSR_MTRRfix4K
      12 #define MSR_CORE
      13 #define MSR_IDT
      14 #define MSR_K7
      16 #define MSR_PKG
      19 #define MSR_F15H
      33 #define MSR_AMD64
      83 #define MSR_P4
     163 #define MSR_IA32

   I.e. this shouldn't this be something like MSR_IA32_TEST_CTL - or this 
   the name the Intel SDM uses? (I haven't checked.)

 - The canonical way to define MSR capabilities is to use the MSR's name 
   as a prefix. I.e.:

        MSR_TEST_CTL
        MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT
        MSR_TEST_CTL_SPLIT_LOCK_DETECT
        etc.

   Instead of the random mixture of MSR_ prefixed and non-prefixed 
   MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT and 
   TEST_CTL_SPLIT_LOCK_DETECT names.

 - Finally, this is not how we define bits - the _SHIFT postfix is actively
   confusing as we usually denote _SHIFT values with something that is 
   used in a bit-shift operation, which this isn't. Instead the proper 
   scheme is to postfix the bit number with _BIT and the mask with _MASK, 
   i.e. something like:

     #define MSR_TEST_CTL				0x00000033
     #define MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT		29
     #define MSR_TEST_CTL_SPLIT_LOCK_DETECT		BIT(MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT)

Note how this cleans up actual usage:

+       msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+       this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);

-       msr_set_bit(MSR_TEST_CTL, MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT);
-       this_cpu_or(msr_test_ctl_cache, MSR_TEST_CTL_SPLIT_LOCK_DETECT);

Frankly, this kind of disorganized code in a v8 submission is *really* 
disappointing, it's not like it's hard to look up these patterns and 
practices in existing code...

Sigh.

Thanks,

	Ingo

Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time

[Ingo Molnar]

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

> To workaround or debug a split lock issue, the administrator may need to
> disable or enable split lock detection during run time without rebooting
> the system.
> 
> The interface /sys/device/system/cpu/split_lock_detect is added to allow
> the administrator to disable or enable split lock detection and show
> current split lock detection setting.
> 
> Writing [yY1] or [oO][nN] to the file enables split lock detection and
> writing [nN0] or [oO][fF] disables split lock detection. Split lock
> detection is enabled or disabled on all CPUs.
> 
> Reading the file returns current global split lock detection setting:
> 0: disabled
> 1: enabled
> 
> Add an ABI document entry for /sys/devices/system/cpu/split_lock_detect.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> Not sure if the justification for the sysfs knob is valid. If not, this
> patch could be removed from this patch set.
> 
>  .../ABI/testing/sysfs-devices-system-cpu      | 22 ++++++++
>  arch/x86/kernel/cpu/intel.c                   | 52 ++++++++++++++++++-
>  2 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 9605dbd4b5b5..aad7b1698065 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -67,6 +67,28 @@ Description:	Discover NUMA node a CPU belongs to
>  		/sys/devices/system/cpu/cpu42/node2 -> ../../node/node2
>  
>  
> +What:		/sys/devices/system/cpu/split_lock_detect
> +Date:		March 2019
> +Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
> +Description:	(RW) Control split lock detection on Intel Tremont and
> +		future CPUs
> +
> +		Reads return split lock detection status:
> +			0: disabled
> +			1: enabled
> +
> +		Writes enable or disable split lock detection:
> +			The first character is one of 'Nn0' or [oO][fF] for off
> +			disables the feature.
> +			The first character is one of 'Yy1' or [oO][nN] for on
> +			enables the feature.
> +
> +		Please note the interface only shows or controls global setting.
> +		During run time, split lock detection on one CPU may be
> +		disabled if split lock operation in kernel code happens on
> +		the CPU. The interface doesn't show or control split lock
> +		detection on individual CPU.

I.e. implementation and possible actual state are out of sync. Why?

Also, if it's a global flag, why waste memory on putting a sysfs knob 
into every CPU's sysfs file?

Finally, why is a debugging facility in sysfs, why not a debugfs knob? 
Using a sysctl would solve the percpu vs. global confusion as well ...

> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -35,6 +35,7 @@
>  DEFINE_PER_CPU(u64, msr_test_ctl_cache);
>  EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
>  
> +static DEFINE_MUTEX(split_lock_detect_mutex);
>  static bool split_lock_detect_enable;

'enable' is a verb in plain form - which we use for function names.

For variable names that denotes current state we typically use past 
tense, i.e. 'enabled'.

(The only case where we'd us the split_lock_detect_enable name for a flag 
if it's a flag to trigger some sort of enabling action - which this 
isn't.)

Please review the whole series for various naming mishaps.

> +	mutex_lock(&split_lock_detect_mutex);
> +
> +	split_lock_detect_enable = val;
> +
> +	/* Update the split lock detection setting in MSR on all online CPUs. */
> +	on_each_cpu(split_lock_update_msr, NULL, 1);
> +
> +	if (split_lock_detect_enable)
> +		pr_info("enabled\n");
> +	else
> +		pr_info("disabled\n");
> +
> +	mutex_unlock(&split_lock_detect_mutex);

Instead of a mutex, please just use the global atomic debug flag which 
controls the warning printout. By using that flag both for the WARN()ing 
and for controlling MSR state all the races are solved and the code is 
simplified.


Thanks,

	Ingo

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

[Thomas Gleixner]

On Wed, 24 Apr 2019, Fenghua Yu wrote:

> +void handle_split_lock_kernel_mode(void)

....

> +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;
> +
> +	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)
> +		return;
> +
> +	cond_local_irq_enable(regs);
> +	if (!user_mode(regs) &&
> +	    static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		/*
> +		 * Only split lock can generate #AC from kernel at this point.
> +		 * Warn and disable split lock detection on this CPU. The
> +		 * faulting instruction will be executed without generating
> +		 * another #AC fault.
> +		 */
> +		return handle_split_lock_kernel_mode();

return fun()? For some reason gcc will not complain about that, but for the
reader it's confusing at best.

Thanks,

	tglx

Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Thomas Gleixner]

On Wed, 24 Apr 2019, Fenghua Yu wrote:
>  
> +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
> +{
> +	u64 host_msr_test_ctl;
> +
> +	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> +		return;

Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask.

> +	host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
> +
> +	if (host_msr_test_ctl == vmx->msr_test_ctl) {

This still assumes that the only bit which can be set in the MSR is that
lock detect bit.

> +		clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
> +	} else {
> +		add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
> +				      host_msr_test_ctl, false);

So what happens here is that if any other bit is set on the host, VMENTER
will happily clear it.

     guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl;

That preserves any bits which are not exposed to the guest.

But the way more interesting question is why are you exposing the MSR and
the bit to the guest at all if the host has split lock detection enabled?

That does not make any sense as you basically allow the guest to switch it
off and then launch a slowdown attack. If the host has it enabled, then a
guest has to be treated like any other process and the #AC trap has to be
caught by the hypervisor which then kills the guest.

Only if the host has split lock detection disabled, then you can expose it
and allow the guest to turn it on and handle it on its own.

Thanks,

	tglx

Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Thomas Gleixner]

On Wed, 24 Apr 2019, Fenghua Yu wrote:
>  
> +static void split_lock_update_msr(void)
> +{
> +	/* Enable split lock detection */
> +	msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> +	this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);

I'm pretty sure, that I told you to utilize the cache proper. Again:

> > Nothing in this file initializes msr_test_ctl_cache explicitely. Register
> > caching always requires to read the register and store it in the cache
> > before doing anything with it. Nothing guarantees that all bits in that MSR
> > are 0 by default forever.
> >
> > And once you do that _before_ calling split_lock_update_msr() then you can
> > spare the RMW in that function.

So you managed to fix the initializaiton part, but then you still do a
pointless RMW.

Thanks,

	tglx

RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[David Laight]

From:  Fenghua Yu
> Sent: 24 April 2019 20:33
> A split locked access locks bus and degrades overall memory access
> performance. When split lock detection feature is enumerated, enable
> the feature by default by writing 1 to bit 29 in MSR TEST_CTL to find
> any split lock issue.

You can't enable this by default until ALL the known potentially
misaligned locked memory operations have been fixed.

AFAICT you've only fixed the ones that have actually faulted.
You've not even fixed the other ones in the same source files
as ones you've 'fixed'.

You need to do a code audit of all the in-tree kernel code
that uses locked operations - especially the 'bit' ones
since a lot of code casts the bitmap address - so it probably
isn't long aligned.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Thomas Gleixner]

On Thu, 25 Apr 2019, David Laight wrote:

> From:  Fenghua Yu
> > Sent: 24 April 2019 20:33
> > A split locked access locks bus and degrades overall memory access
> > performance. When split lock detection feature is enumerated, enable
> > the feature by default by writing 1 to bit 29 in MSR TEST_CTL to find
> > any split lock issue.
> 
> You can't enable this by default until ALL the known potentially
> misaligned locked memory operations have been fixed.

Errm? The result will be a WARN_ON() printed and no further damage. It's
not making anything worse than it is now. In fact we just should add a

    WARN_ON_ONCE(!aligned_to_long(p)) to all the xxx_bit() operations.

so we catch them even when they do not trigger that #AC thingy.

Thanks,

	tglx

RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[David Laight]

From: Thomas Gleixne]
> Sent: 25 April 2019 11:59
> On Thu, 25 Apr 2019, David Laight wrote:
> 
> > From:  Fenghua Yu
> > > Sent: 24 April 2019 20:33
> > > A split locked access locks bus and degrades overall memory access
> > > performance. When split lock detection feature is enumerated, enable
> > > the feature by default by writing 1 to bit 29 in MSR TEST_CTL to find
> > > any split lock issue.
> >
> > You can't enable this by default until ALL the known potentially
> > misaligned locked memory operations have been fixed.
> 
> Errm? The result will be a WARN_ON() printed and no further damage.

ISTR something about sending SIGSEGV to userspace.

> It's not making anything worse than it is now. In fact we just should add a
> 
>     WARN_ON_ONCE(!aligned_to_long(p)) to all the xxx_bit() operations.
> 
> so we catch them even when they do not trigger that #AC thingy.

That will explode the kernel code size.
In any case some of the items I found in a quick scan were bss/data
so the alignment will vary from build to build.

I also found some casts on the xxx_bit() functions in generic code.
I didn't look to see how badly wrong they go on BE systems.

While the x86 xxx_bit() functions could easily be changed to do
32bit accesses, the 'misaligned' operations will affect all
architectures - and may have different effects on others.

I'm not at all sure that 'compare and exchange' operations
are atomic on all cpus if the data is misaligned and crosses
a page boundary and either (or both) pages need faulting in
(or hit a TLB miss).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Peter Zijlstra]

On Thu, Apr 25, 2019 at 11:13:23AM +0000, David Laight wrote:
> I'm not at all sure that 'compare and exchange' operations
> are atomic on all cpus if the data is misaligned and crosses
> a page boundary and either (or both) pages need faulting in
> (or hit a TLB miss).

Most sane architectures already trap if you attempt that.

RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Thomas Gleixner]

On Thu, 25 Apr 2019, David Laight wrote:
> From: Thomas Gleixne]
> > Sent: 25 April 2019 11:59
> > On Thu, 25 Apr 2019, David Laight wrote:
> > 
> > > From:  Fenghua Yu
> > > > Sent: 24 April 2019 20:33
> > > > A split locked access locks bus and degrades overall memory access
> > > > performance. When split lock detection feature is enumerated, enable
> > > > the feature by default by writing 1 to bit 29 in MSR TEST_CTL to find
> > > > any split lock issue.
> > >
> > > You can't enable this by default until ALL the known potentially
> > > misaligned locked memory operations have been fixed.
> > 
> > Errm? The result will be a WARN_ON() printed and no further damage.
> 
> ISTR something about sending SIGSEGV to userspace.

Nope. If the #AC originates from kernel then a warning is printed and the
detection is disabled.

Userspace is a different story. We cannot audit all user space upfront,
right?

> > It's not making anything worse than it is now. In fact we just should add a
> > 
> >     WARN_ON_ONCE(!aligned_to_long(p)) to all the xxx_bit() operations.
> > 
> > so we catch them even when they do not trigger that #AC thingy.
> 
> That will explode the kernel code size.

So what? We have enough debug options which make the kernel big. One more
does not really matter.

Thanks,

	tglx

Re: [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations

[Kalle Valo]

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

> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Bitmaps are defined on unsigned longs, so the usage of u32[2] in the
> wlcore driver is incorrect.  As noted by Peter Zijlstra, casting arrays
> to a bitmap is incorrect for big-endian architectures.
> 
> When looking at it I observed that:
> 
> - operations on reg_ch_conf_pending is always under the wl_lock mutex,
> so set_bit is overkill
> 
> - the only case where reg_ch_conf_pending is accessed a u32 at a time is
> unnecessary too.
> 
> This patch cleans up everything in this area, and changes tmp_ch_bitmap
> to have the proper alignment.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Patch applied to wireless-drivers-next.git, thanks.

147b502bda33 wlcore: simplify/fix/optimize reg_ch_conf_pending operations

-- 
https://patchwork.kernel.org/patch/10915635/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Fenghua Yu]

On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > enumerates a model specific feature. Currently bit 5 enumerates split
> > lock detection. When bit 5 is 1, split lock detection is supported.
> > When the bit is 0, split lock detection is not supported.
> > 
> > Please check the latest Intel 64 and IA-32 Architectures Software
> > Developer's Manual for more detailed information on the MSR and the
> > split lock detection bit.
> > 
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> >  arch/x86/include/asm/msr-index.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index ca5bc0eacb95..f65ef6f783d2 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -59,6 +59,9 @@
> >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> >  
> > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> 
> Please don't put comments into definitions.

I'll remove the comment and change definitions of the MSR and the split lock
detection bit as following:

+#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
+#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
+#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)

Are these right changes?

Thanks.

-Fenghua

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Ingo Molnar]

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

> On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> > 
> > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > 
> > > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > > enumerates a model specific feature. Currently bit 5 enumerates split
> > > lock detection. When bit 5 is 1, split lock detection is supported.
> > > When the bit is 0, split lock detection is not supported.
> > > 
> > > Please check the latest Intel 64 and IA-32 Architectures Software
> > > Developer's Manual for more detailed information on the MSR and the
> > > split lock detection bit.
> > > 
> > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > ---
> > >  arch/x86/include/asm/msr-index.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > index ca5bc0eacb95..f65ef6f783d2 100644
> > > --- a/arch/x86/include/asm/msr-index.h
> > > +++ b/arch/x86/include/asm/msr-index.h
> > > @@ -59,6 +59,9 @@
> > >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> > >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> > >  
> > > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> > 
> > Please don't put comments into definitions.
> 
> I'll remove the comment and change definitions of the MSR and the split lock
> detection bit as following:
> 
> +#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
> +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
> +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)
> 
> Are these right changes?

I suspect it could be shortened to CORE_CAP as you (partly) did it 
originally.

Thanks,

	Ingo

Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register

[Fenghua Yu]

On Thu, Apr 25, 2019 at 08:21:26AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > Setting bit 29 in MSR TEST_CTL (0x33) enables split lock detection and
> > clearing the bit disables split lock detection.
> > 
> > Define the MSR and the bit. The definitions will be used in enabling or
> > disabling split lock detection.
> > 
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> >  arch/x86/include/asm/msr-index.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index f65ef6f783d2..296eeb761ab6 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_SPLIT_LOCK_DETECT_SHIFT	29
> > +#define TEST_CTL_SPLIT_LOCK_DETECT		BIT(29)
> 
> Three problems:
> 
>  - Is MSR_TEST_CTL is not really a canonical MSR name... A quick look at 
>    msr-index reveals the prevailing nomenclature:
> 
>      dagon:~/tip> git grep -h 'define MSR' arch/x86/include/asm/msr-index.h | cut -d_ -f1-2 | sort -n | uniq -c | sort -n | tail -10
>        8 #define MSR_K8
>        8 #define MSR_MTRRfix4K
>       12 #define MSR_CORE
>       13 #define MSR_IDT
>       14 #define MSR_K7
>       16 #define MSR_PKG
>       19 #define MSR_F15H
>       33 #define MSR_AMD64
>       83 #define MSR_P4
>      163 #define MSR_IA32
> 
>    I.e. this shouldn't this be something like MSR_IA32_TEST_CTL - or this 
>    the name the Intel SDM uses? (I haven't checked.)

TEST_CTL is the MSR's exact name shown in Table 2-14 in the latest SDM.
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

So can I still use MSR_TEST_CTL here?

> 
>  - The canonical way to define MSR capabilities is to use the MSR's name 
>    as a prefix. I.e.:
> 
>         MSR_TEST_CTL
>         MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT
>         MSR_TEST_CTL_SPLIT_LOCK_DETECT
>         etc.
> 
>    Instead of the random mixture of MSR_ prefixed and non-prefixed 
>    MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT and 
>    TEST_CTL_SPLIT_LOCK_DETECT names.
> 
>  - Finally, this is not how we define bits - the _SHIFT postfix is actively
>    confusing as we usually denote _SHIFT values with something that is 
>    used in a bit-shift operation, which this isn't. Instead the proper 
>    scheme is to postfix the bit number with _BIT and the mask with _MASK, 
>    i.e. something like:
> 
>      #define MSR_TEST_CTL				0x00000033
>      #define MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT		29
>      #define MSR_TEST_CTL_SPLIT_LOCK_DETECT		BIT(MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT)
> 
> Note how this cleans up actual usage:
> 
> +       msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> +       this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
> 
> -       msr_set_bit(MSR_TEST_CTL, MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT);
> -       this_cpu_or(msr_test_ctl_cache, MSR_TEST_CTL_SPLIT_LOCK_DETECT);
> 
> Frankly, this kind of disorganized code in a v8 submission is *really* 
> disappointing, it's not like it's hard to look up these patterns and 
> practices in existing code...

OK. Will change the bit and mask definitions.

Thanks.

-Fenghua

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Fenghua Yu]

On Thu, Apr 25, 2019 at 09:47:14PM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> > > 
> > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > 
> > > > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > > > enumerates a model specific feature. Currently bit 5 enumerates split
> > > > lock detection. When bit 5 is 1, split lock detection is supported.
> > > > When the bit is 0, split lock detection is not supported.
> > > > 
> > > > Please check the latest Intel 64 and IA-32 Architectures Software
> > > > Developer's Manual for more detailed information on the MSR and the
> > > > split lock detection bit.
> > > > 
> > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > > ---
> > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > > index ca5bc0eacb95..f65ef6f783d2 100644
> > > > --- a/arch/x86/include/asm/msr-index.h
> > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > @@ -59,6 +59,9 @@
> > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> > > >  
> > > > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > > > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> > > 
> > > Please don't put comments into definitions.
> > 
> > I'll remove the comment and change definitions of the MSR and the split lock
> > detection bit as following:
> > 
> > +#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
> > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
> > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)
> > 
> > Are these right changes?
> 
> I suspect it could be shortened to CORE_CAP as you (partly) did it 
> originally.

IA32_CORE_CAPABILITY is the MSR's exact name in the latest SDM (in Table 2-14):
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

So can I define the MSR and the bits as follows?

+#define MSR_IA32_CORE_CAP                       0x000000cf
+#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT 5
+#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT)

Thanks.

-Fenghua

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Ingo Molnar]

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

> On Thu, Apr 25, 2019 at 09:47:14PM +0200, Ingo Molnar wrote:
> > 
> > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > 
> > > On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > > 
> > > > > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > > > > enumerates a model specific feature. Currently bit 5 enumerates split
> > > > > lock detection. When bit 5 is 1, split lock detection is supported.
> > > > > When the bit is 0, split lock detection is not supported.
> > > > > 
> > > > > Please check the latest Intel 64 and IA-32 Architectures Software
> > > > > Developer's Manual for more detailed information on the MSR and the
> > > > > split lock detection bit.
> > > > > 
> > > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > > > ---
> > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > > > index ca5bc0eacb95..f65ef6f783d2 100644
> > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > @@ -59,6 +59,9 @@
> > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> > > > >  
> > > > > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > > > > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> > > > 
> > > > Please don't put comments into definitions.
> > > 
> > > I'll remove the comment and change definitions of the MSR and the split lock
> > > detection bit as following:
> > > 
> > > +#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
> > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
> > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)
> > > 
> > > Are these right changes?
> > 
> > I suspect it could be shortened to CORE_CAP as you (partly) did it 
> > originally.
> 
> IA32_CORE_CAPABILITY is the MSR's exact name in the latest SDM (in Table 2-14):
> 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
> 
> So can I define the MSR and the bits as follows?
> 
> +#define MSR_IA32_CORE_CAP                       0x000000cf
> +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT 5
> +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT)

Yeah, I suppose that looks OK.

Thanks,

	Ingo

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Fenghua Yu]

On Thu, Apr 25, 2019 at 10:08:30PM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > On Thu, Apr 25, 2019 at 09:47:14PM +0200, Ingo Molnar wrote:
> > > 
> > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > 
> > > > On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> > > > > 
> > > > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > > > 
> > > > > > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > > > > > enumerates a model specific feature. Currently bit 5 enumerates split
> > > > > > lock detection. When bit 5 is 1, split lock detection is supported.
> > > > > > When the bit is 0, split lock detection is not supported.
> > > > > > 
> > > > > > Please check the latest Intel 64 and IA-32 Architectures Software
> > > > > > Developer's Manual for more detailed information on the MSR and the
> > > > > > split lock detection bit.
> > > > > > 
> > > > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > > > > ---
> > > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > > > > index ca5bc0eacb95..f65ef6f783d2 100644
> > > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > > @@ -59,6 +59,9 @@
> > > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> > > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> > > > > >  
> > > > > > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > > > > > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> > > > > 
> > > > > Please don't put comments into definitions.
> > > > 
> > > > I'll remove the comment and change definitions of the MSR and the split lock
> > > > detection bit as following:
> > > > 
> > > > +#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
> > > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
> > > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)
> > > > 
> > > > Are these right changes?
> > > 
> > > I suspect it could be shortened to CORE_CAP as you (partly) did it 
> > > originally.
> > 
> > IA32_CORE_CAPABILITY is the MSR's exact name in the latest SDM (in Table 2-14):
> > 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
> > 
> > So can I define the MSR and the bits as follows?
> > 
> > +#define MSR_IA32_CORE_CAP                       0x000000cf
> > +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT 5
> > +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT)
> 
> Yeah, I suppose that looks OK.

Should I also change the feature definition 'X86_FEATURE_CORE_CAPABILITY' to
'X86_FEATURE_CORE_CAP' in cpufeatures.h in patch #0006 to match the
MSR definition here? Or should I still keep the current feature definition?

Thanks.

-Fenghua

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Ingo Molnar]

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

> On Thu, Apr 25, 2019 at 10:08:30PM +0200, Ingo Molnar wrote:
> > 
> > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > 
> > > On Thu, Apr 25, 2019 at 09:47:14PM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > > 
> > > > > On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> > > > > > 
> > > > > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > > > > 
> > > > > > > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > > > > > > enumerates a model specific feature. Currently bit 5 enumerates split
> > > > > > > lock detection. When bit 5 is 1, split lock detection is supported.
> > > > > > > When the bit is 0, split lock detection is not supported.
> > > > > > > 
> > > > > > > Please check the latest Intel 64 and IA-32 Architectures Software
> > > > > > > Developer's Manual for more detailed information on the MSR and the
> > > > > > > split lock detection bit.
> > > > > > > 
> > > > > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > > > > > ---
> > > > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > > > > > index ca5bc0eacb95..f65ef6f783d2 100644
> > > > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > > > @@ -59,6 +59,9 @@
> > > > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> > > > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> > > > > > >  
> > > > > > > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > > > > > > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> > > > > > 
> > > > > > Please don't put comments into definitions.
> > > > > 
> > > > > I'll remove the comment and change definitions of the MSR and the split lock
> > > > > detection bit as following:
> > > > > 
> > > > > +#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
> > > > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
> > > > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)
> > > > > 
> > > > > Are these right changes?
> > > > 
> > > > I suspect it could be shortened to CORE_CAP as you (partly) did it 
> > > > originally.
> > > 
> > > IA32_CORE_CAPABILITY is the MSR's exact name in the latest SDM (in Table 2-14):
> > > 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
> > > 
> > > So can I define the MSR and the bits as follows?
> > > 
> > > +#define MSR_IA32_CORE_CAP                       0x000000cf
> > > +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT 5
> > > +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT)
> > 
> > Yeah, I suppose that looks OK.
> 
> Should I also change the feature definition 'X86_FEATURE_CORE_CAPABILITY' to
> 'X86_FEATURE_CORE_CAP' in cpufeatures.h in patch #0006 to match the
> MSR definition here? Or should I still keep the current feature definition?
> 
> Thanks.

Hm, no, for CPU features it's good to follow the vendor convention.

So I guess the long-form CPU_CAPABILITY for all of these is the best 
after all.

Thanks,

	Ingo

Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Xiaoyao Li]

On Thu, 2019-04-25 at 09:42 +0200, Thomas Gleixner wrote:
> On Wed, 24 Apr 2019, Fenghua Yu wrote:
> >  
> > +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
> > +{
> > +	u64 host_msr_test_ctl;
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> > +		return;
> 
> Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask.
> 
> > +	host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
> > +
> > +	if (host_msr_test_ctl == vmx->msr_test_ctl) {
> 
> This still assumes that the only bit which can be set in the MSR is that
> lock detect bit.
> 
> > +		clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
> > +	} else {
> > +		add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
> > +				      host_msr_test_ctl, false);
> 
> So what happens here is that if any other bit is set on the host, VMENTER
> will happily clear it.

There are two bits of MSR TEST_CTL defined in Intel SDM now, which is bit 29 and
bit 31. Bit 31 is not used in kernel, and here we only need to switch bit 29
between host and guest. 
So should I also change the name to atomic_switch_split_lock_detect() to
indicate that we only switch bit 29?

>      guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl;
> 
> That preserves any bits which are not exposed to the guest.
> 
> But the way more interesting question is why are you exposing the MSR and
> the bit to the guest at all if the host has split lock detection enabled?
> 
> That does not make any sense as you basically allow the guest to switch it
> off and then launch a slowdown attack. If the host has it enabled, then a
> guest has to be treated like any other process and the #AC trap has to be
> caught by the hypervisor which then kills the guest.
> 
> Only if the host has split lock detection disabled, then you can expose it
> and allow the guest to turn it on and handle it on its own.

Indeed, if we use split lock detection for protection purpose, when host has it
enabled we should directly pass it to guest and forbid guest from disabling it.
And only when host disables split lock detection, we can expose it and allow the
guest to turn it on.

If it is used for protection purpose, then it should follow what you said and
this feature needs to be disabled by default. Because there are split lock
issues in old/current kernels and BIOS. That will cause the existing guest
booting failure and killed due to those split lock.

If it is only used for debug purpose, I think it might be OK to enable this
feature by default and make it indepedent between host and guest?

So I think how to handle this feature between host and guest depends on how we
use it? Once you give me a decision, I will follow it in next version.

> Thanks,
> 
> 	tglx
> 
> 

Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Thomas Gleixner]

On Sat, 27 Apr 2019, Xiaoyao Li wrote:
> On Thu, 2019-04-25 at 09:42 +0200, Thomas Gleixner wrote:
> > On Wed, 24 Apr 2019, Fenghua Yu wrote:
> > >  
> > > +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
> > > +{
> > > +	u64 host_msr_test_ctl;
> > > +
> > > +	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> > > +		return;
> > 
> > Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask.
> > 
> > > +	host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
> > > +
> > > +	if (host_msr_test_ctl == vmx->msr_test_ctl) {
> > 
> > This still assumes that the only bit which can be set in the MSR is that
> > lock detect bit.
> > 
> > > +		clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
> > > +	} else {
> > > +		add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
> > > +				      host_msr_test_ctl, false);
> > 
> > So what happens here is that if any other bit is set on the host, VMENTER
> > will happily clear it.
> 
> There are two bits of MSR TEST_CTL defined in Intel SDM now, which is bit
> 29 and bit 31. Bit 31 is not used in kernel, and here we only need to
> switch bit 29 between host and guest.  So should I also change the name
> to atomic_switch_split_lock_detect() to indicate that we only switch bit
> 29?

No. Just because we ony use the split lock bit now, there is no
jusification to name everything splitlock. This is going to have renamed
when yet another bit is added in the future. The MSR is exposed to the
guest and the restriction of bits happens to be splitlock today.

> >      guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl;
> > 
> > That preserves any bits which are not exposed to the guest.
> > 
> > But the way more interesting question is why are you exposing the MSR and
> > the bit to the guest at all if the host has split lock detection enabled?
> > 
> > That does not make any sense as you basically allow the guest to switch it
> > off and then launch a slowdown attack. If the host has it enabled, then a
> > guest has to be treated like any other process and the #AC trap has to be
> > caught by the hypervisor which then kills the guest.
> > 
> > Only if the host has split lock detection disabled, then you can expose it
> > and allow the guest to turn it on and handle it on its own.
> 
> Indeed, if we use split lock detection for protection purpose, when host
> has it enabled we should directly pass it to guest and forbid guest from
> disabling it.  And only when host disables split lock detection, we can
> expose it and allow the guest to turn it on.
?
> If it is used for protection purpose, then it should follow what you said and
> this feature needs to be disabled by default. Because there are split lock
> issues in old/current kernels and BIOS. That will cause the existing guest
> booting failure and killed due to those split lock.

Rightfully so.

> If it is only used for debug purpose, I think it might be OK to enable this
> feature by default and make it indepedent between host and guest?

No. It does not make sense.

> So I think how to handle this feature between host and guest depends on how we
> use it? Once you give me a decision, I will follow it in next version.

As I said: The host kernel makes the decision.

If the host kernel has it enabled then the guest is not allowed to change
it. If the guest triggers an #AC it will be killed.

If the host kernel has it disabled then the guest can enable it for it's
own purposes.

Thanks,

	tglx

Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Xiaoyao Li]

On 4/28/2019 3:09 PM, Thomas Gleixner wrote:
> On Sat, 27 Apr 2019, Xiaoyao Li wrote:
>> On Thu, 2019-04-25 at 09:42 +0200, Thomas Gleixner wrote:
>>> On Wed, 24 Apr 2019, Fenghua Yu wrote:
>>>>   
>>>> +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
>>>> +{
>>>> +	u64 host_msr_test_ctl;
>>>> +
>>>> +	if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
>>>> +		return;
>>>
>>> Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask.
>>>
>>>> +	host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
>>>> +
>>>> +	if (host_msr_test_ctl == vmx->msr_test_ctl) {
>>>
>>> This still assumes that the only bit which can be set in the MSR is that
>>> lock detect bit.
>>>
>>>> +		clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
>>>> +	} else {
>>>> +		add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
>>>> +				      host_msr_test_ctl, false);
>>>
>>> So what happens here is that if any other bit is set on the host, VMENTER
>>> will happily clear it.
>>
>> There are two bits of MSR TEST_CTL defined in Intel SDM now, which is bit
>> 29 and bit 31. Bit 31 is not used in kernel, and here we only need to
>> switch bit 29 between host and guest.  So should I also change the name
>> to atomic_switch_split_lock_detect() to indicate that we only switch bit
>> 29?
> 
> No. Just because we ony use the split lock bit now, there is no
> jusification to name everything splitlock. This is going to have renamed
> when yet another bit is added in the future. The MSR is exposed to the
> guest and the restriction of bits happens to be splitlock today.

Got it.

>>>       guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl;
>>>
>>> That preserves any bits which are not exposed to the guest.
>>>
>>> But the way more interesting question is why are you exposing the MSR and
>>> the bit to the guest at all if the host has split lock detection enabled?
>>>
>>> That does not make any sense as you basically allow the guest to switch it
>>> off and then launch a slowdown attack. If the host has it enabled, then a
>>> guest has to be treated like any other process and the #AC trap has to be
>>> caught by the hypervisor which then kills the guest.
>>>
>>> Only if the host has split lock detection disabled, then you can expose it
>>> and allow the guest to turn it on and handle it on its own.
>>
>> Indeed, if we use split lock detection for protection purpose, when host
>> has it enabled we should directly pass it to guest and forbid guest from
>> disabling it.  And only when host disables split lock detection, we can
>> expose it and allow the guest to turn it on.
> ?
>> If it is used for protection purpose, then it should follow what you said and
>> this feature needs to be disabled by default. Because there are split lock
>> issues in old/current kernels and BIOS. That will cause the existing guest
>> booting failure and killed due to those split lock.
> 
> Rightfully so.

So, the patch 13 "Enable split lock detection by default" needs to be 
removed?

>> If it is only used for debug purpose, I think it might be OK to enable this
>> feature by default and make it indepedent between host and guest?
> 
> No. It does not make sense.
> 
>> So I think how to handle this feature between host and guest depends on how we
>> use it? Once you give me a decision, I will follow it in next version.
> 
> As I said: The host kernel makes the decision.
> 
> If the host kernel has it enabled then the guest is not allowed to change
> it. If the guest triggers an #AC it will be killed.
> 
> If the host kernel has it disabled then the guest can enable it for it's
> own purposes.
> 
> Thanks,
> 
> 	tglx
> 

Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Xiaoyao Li]

Hi, Thomas,

Base on your comments, I plan to make the design as following:

1) When host enables this feature, there is no switch between host and 
guest that guest running with it enabled by force. Since #AC in 
exception bitmap is set in current kvm, every #AC in guest will be 
trapped. And in handle_exception() handler in kvm, if #AC is caused by 
alignment check, kvm injects #AC back to guest; if #AC is caused by 
split lock, kvm sends a SIGBUS to userspace.

2) When host disables this feature, there needs atomic switch between 
host and guest if different. And in handle_exception() handler in kvm, 
we can just inject #AC back to guest, and let guest to handle it.

Besides, I think there might be an optimization for case #1.
When host has it enabled and guest also has it enabled, I think it's OK 
to inject #AC back to guest, not directly kill the guest.
Because guest kernel has it enabled means it knows what this feature is 
and it also want to get aware of and fault every split lock.
At this point, if guest has it enabled, we can leave it to guest. Only 
when guest's configuration is having it disabled, can it be regards as 
potentially harmful that we kill the guest once there is a #AC due to 
split lock.

How do you think about the design and this optimization?

Hi, Paolo,

What's your opinion about this design of split lock in KVM?

Thanks.

On 4/28/2019 3:09 PM, Thomas Gleixner wrote:
> On Sat, 27 Apr 2019, Xiaoyao Li wrote:
>> On Thu, 2019-04-25 at 09:42 +0200, Thomas Gleixner wrote:
>>> But the way more interesting question is why are you exposing the MSR and
>>> the bit to the guest at all if the host has split lock detection enabled?
>>>
>>> That does not make any sense as you basically allow the guest to switch it
>>> off and then launch a slowdown attack. If the host has it enabled, then a
>>> guest has to be treated like any other process and the #AC trap has to be
>>> caught by the hypervisor which then kills the guest.
>>>
>>> Only if the host has split lock detection disabled, then you can expose it
>>> and allow the guest to turn it on and handle it on its own.
>>
>> Indeed, if we use split lock detection for protection purpose, when host
>> has it enabled we should directly pass it to guest and forbid guest from
>> disabling it.  And only when host disables split lock detection, we can
>> expose it and allow the guest to turn it on.
> ?
>> If it is used for protection purpose, then it should follow what you said and
>> this feature needs to be disabled by default. Because there are split lock
>> issues in old/current kernels and BIOS. That will cause the existing guest
>> booting failure and killed due to those split lock.
> 
> Rightfully so.
> 
>> If it is only used for debug purpose, I think it might be OK to enable this
>> feature by default and make it indepedent between host and guest?
> 
> No. It does not make sense.
> 
>> So I think how to handle this feature between host and guest depends on how we
>> use it? Once you give me a decision, I will follow it in next version.
> 
> As I said: The host kernel makes the decision.
> 
> If the host kernel has it enabled then the guest is not allowed to change
> it. If the guest triggers an #AC it will be killed.
> 
> If the host kernel has it disabled then the guest can enable it for it's
> own purposes.
> 
> Thanks,
> 
> 	tglx
> 

Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL

[Thomas Gleixner]

On Sun, 28 Apr 2019, Xiaoyao Li wrote:
> On 4/28/2019 3:09 PM, Thomas Gleixner wrote:
> > On Sat, 27 Apr 2019, Xiaoyao Li wrote:
> > > Indeed, if we use split lock detection for protection purpose, when host
> > > has it enabled we should directly pass it to guest and forbid guest from
> > > disabling it.  And only when host disables split lock detection, we can
> > > expose it and allow the guest to turn it on.
> > ?
> > > If it is used for protection purpose, then it should follow what you said
> > > and
> > > this feature needs to be disabled by default. Because there are split lock
> > > issues in old/current kernels and BIOS. That will cause the existing guest
> > > booting failure and killed due to those split lock.
> > 
> > Rightfully so.
> 
> So, the patch 13 "Enable split lock detection by default" needs to be removed?

Why? No. We enable it by default and everything which violates the rules
gets what it deserves. If there is an issue, boot with ac_splitlock_off and
be done with it.

Thanks,

	tglx

Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit

[Fenghua Yu]

On Fri, Apr 26, 2019 at 08:00:10AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > On Thu, Apr 25, 2019 at 10:08:30PM +0200, Ingo Molnar wrote:
> > > 
> > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > 
> > > > On Thu, Apr 25, 2019 at 09:47:14PM +0200, Ingo Molnar wrote:
> > > > > 
> > > > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > > > 
> > > > > > On Thu, Apr 25, 2019 at 07:45:11AM +0200, Ingo Molnar wrote:
> > > > > > > 
> > > > > > > * Fenghua Yu <fenghua.yu@intel.com> wrote:
> > > > > > > 
> > > > > > > > A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> > > > > > > > enumerates a model specific feature. Currently bit 5 enumerates split
> > > > > > > > lock detection. When bit 5 is 1, split lock detection is supported.
> > > > > > > > When the bit is 0, split lock detection is not supported.
> > > > > > > > 
> > > > > > > > Please check the latest Intel 64 and IA-32 Architectures Software
> > > > > > > > Developer's Manual for more detailed information on the MSR and the
> > > > > > > > split lock detection bit.
> > > > > > > > 
> > > > > > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > > > > > > > ---
> > > > > > > >  arch/x86/include/asm/msr-index.h | 3 +++
> > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > > > > > > index ca5bc0eacb95..f65ef6f783d2 100644
> > > > > > > > --- a/arch/x86/include/asm/msr-index.h
> > > > > > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > > > > > @@ -59,6 +59,9 @@
> > > > > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT	31
> > > > > > > >  #define MSR_PLATFORM_INFO_CPUID_FAULT		BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
> > > > > > > >  
> > > > > > > > +#define MSR_IA32_CORE_CAPABILITY	0x000000cf
> > > > > > > > +#define CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)     /* Detect split lock */
> > > > > > > 
> > > > > > > Please don't put comments into definitions.
> > > > > > 
> > > > > > I'll remove the comment and change definitions of the MSR and the split lock
> > > > > > detection bit as following:
> > > > > > 
> > > > > > +#define MSR_IA32_CORE_CAPABILITY                       0x000000cf
> > > > > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT 5
> > > > > > +#define MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAPABILITY_SPLIT_LOCK_DETECT_BIT)
> > > > > > 
> > > > > > Are these right changes?
> > > > > 
> > > > > I suspect it could be shortened to CORE_CAP as you (partly) did it 
> > > > > originally.
> > > > 
> > > > IA32_CORE_CAPABILITY is the MSR's exact name in the latest SDM (in Table 2-14):
> > > > 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
> > > > 
> > > > So can I define the MSR and the bits as follows?
> > > > 
> > > > +#define MSR_IA32_CORE_CAP                       0x000000cf
> > > > +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT 5
> > > > +#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT     BIT(MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT)
> > > 
> > > Yeah, I suppose that looks OK.
> > 
> > Should I also change the feature definition 'X86_FEATURE_CORE_CAPABILITY' to
> > 'X86_FEATURE_CORE_CAP' in cpufeatures.h in patch #0006 to match the
> > MSR definition here? Or should I still keep the current feature definition?
> > 
> > Thanks.
> 
> Hm, no, for CPU features it's good to follow the vendor convention.
> 
> So I guess the long-form CPU_CAPABILITY for all of these is the best 
> after all.

Since MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT_BIT is not used anywhere else
except in this patch, is it OK not to define this macro?

So this patch will only has two shorter lines:

+#define MSR_IA32_CORE_CAP                      0x000000cf
+#define MSR_IA32_CORE_CAP_SPLIT_LOCK_DETECT	BIT(5)

Is this OK for this patch to only define these two macros?

Thanks.

-Fenghua

Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time

[Fenghua Yu]

On Thu, Apr 25, 2019 at 08:31:15AM +0200, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > +		disabled if split lock operation in kernel code happens on
> > +		the CPU. The interface doesn't show or control split lock
> > +		detection on individual CPU.
> 
> I.e. implementation and possible actual state are out of sync. Why?
> 
> Also, if it's a global flag, why waste memory on putting a sysfs knob 
> into every CPU's sysfs file?
> 
> Finally, why is a debugging facility in sysfs, why not a debugfs knob? 
> Using a sysctl would solve the percpu vs. global confusion as well ...

Can I put the interface in /sys/kernel/debug/x86/split_lock_detect?

> 
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -35,6 +35,7 @@
> >  DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> >  EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> >  
> > +static DEFINE_MUTEX(split_lock_detect_mutex);
> >  static bool split_lock_detect_enable;
> 
> 'enable' is a verb in plain form - which we use for function names.
> 
> For variable names that denotes current state we typically use past 
> tense, i.e. 'enabled'.
> 
> (The only case where we'd us the split_lock_detect_enable name for a flag 
> if it's a flag to trigger some sort of enabling action - which this 
> isn't.)
> 
> Please review the whole series for various naming mishaps.
OK.

> 
> > +	mutex_lock(&split_lock_detect_mutex);
> > +
> > +	split_lock_detect_enable = val;
> > +
> > +	/* Update the split lock detection setting in MSR on all online CPUs. */
> > +	on_each_cpu(split_lock_update_msr, NULL, 1);
> > +
> > +	if (split_lock_detect_enable)
> > +		pr_info("enabled\n");
> > +	else
> > +		pr_info("disabled\n");
> > +
> > +	mutex_unlock(&split_lock_detect_mutex);
> 
> Instead of a mutex, please just use the global atomic debug flag which 
> controls the warning printout. By using that flag both for the WARN()ing 
> and for controlling MSR state all the races are solved and the code is 
> simplified.

So is it OK to define split_lock_debug and use it in #AC handler and in
here?

static atomic_t split_lock_debug;

in #AC handler:

+       if (atomic_cmpxchg(&split_lock_debug, 0, 1) == 0) {
+               /* Only warn split lock once */
+               WARN_ONCE(1, "split lock operation detected\n");
+               atomic_set(&split_lock_debug, 0);
+       }

And in split_lock_detect_store(), replace the mutex with split_lock_debug
like this:
 
-       mutex_lock(&split_lock_detect_mutex);
+       while (atomic_cmpxchg(&split_lock_debug, 1, 0))
+              cpu_relax();
.... 
-       mutex_unlock(&split_lock_detect_mutex);
+       atomic_set(&split_lock_debug, 0);
 
Is this right code for sync in both #AC handler and in
split_lock_detect_store()?

Thanks.

-Fenghua

Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Fenghua Yu]

On Thu, Apr 25, 2019 at 09:50:20AM +0200, Thomas Gleixner wrote:
> On Wed, 24 Apr 2019, Fenghua Yu wrote:
> >  
> > +static void split_lock_update_msr(void)
> > +{
> > +	/* Enable split lock detection */
> > +	msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> > +	this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
> 
> I'm pretty sure, that I told you to utilize the cache proper. Again:
> 
> > > Nothing in this file initializes msr_test_ctl_cache explicitely. Register
> > > caching always requires to read the register and store it in the cache
> > > before doing anything with it. Nothing guarantees that all bits in that MSR
> > > are 0 by default forever.
> > >
> > > And once you do that _before_ calling split_lock_update_msr() then you can
> > > spare the RMW in that function.
> 
> So you managed to fix the initializaiton part, but then you still do a
> pointless RMW.

Ok. I see. msr_set_bit() is a RMW operation.

So is the following the right code to update msr and cache variable?

+static void split_lock_update_msr(void)
+{
+   /* Enable split lock detection */
+   this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
+   wrmsrl(MSR_TEST_CTL, msr_test_ctl_cache);

Thanks.

-Fenghua

Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default

[Fenghua Yu]

On Thu, Apr 25, 2019 at 12:58:32PM +0200, Thomas Gleixner wrote:
> On Thu, 25 Apr 2019, David Laight wrote:
> 
> > From:  Fenghua Yu
> > > Sent: 24 April 2019 20:33
> > > A split locked access locks bus and degrades overall memory access
> > > performance. When split lock detection feature is enumerated, enable
> > > the feature by default by writing 1 to bit 29 in MSR TEST_CTL to find
> > > any split lock issue.
> > 
> > You can't enable this by default until ALL the known potentially
> > misaligned locked memory operations have been fixed.
> 
> Errm? The result will be a WARN_ON() printed and no further damage. It's
> not making anything worse than it is now. In fact we just should add a
> 
>     WARN_ON_ONCE(!aligned_to_long(p)) to all the xxx_bit() operations.
> 
> so we catch them even when they do not trigger that #AC thingy.

I add WARN_ON_ONCE() in atomic xxx_bit(). But the code cannot be compiled.

Here is a simplified patch (only adding warning in set_bit()):

diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 8e790ec219a5..bc889ac12e26 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -14,6 +14,8 @@
 #endif

 #include <linux/compiler.h>
+#include <linux/kernel.h>
+#include <asm-generic/bug.h>
 #include <asm/alternative.h>
 #include <asm/rmwcc.h>
 #include <asm/barrier.h>
@@ -67,6 +69,8 @@
 static __always_inline void
 set_bit(long nr, volatile unsigned long *addr)
 {
+       WARN_ON_ONCE(!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)));
+
        if (IS_IMMEDIATE(nr)) {
                asm volatile(LOCK_PREFIX "orb %1,%0"
                        : CONST_MASK_ADDR(nr, addr)

gcc reports errors:
  CC      kernel/bounds.s
  CALL    scripts/atomic/check-atomics.sh
In file included from ./include/linux/bitops.h:19,
                 from ./include/linux/kernel.h:12,
                 from ./include/asm-generic/bug.h:18,
                 from ./arch/x86/include/asm/bug.h:83,
                 from ./include/linux/bug.h:5,
                 from ./include/linux/page-flags.h:10,
                 from kernel/bounds.c:10:
./arch/x86/include/asm/bitops.h: In function ‘set_bit’:
./arch/x86/include/asm/bitops.h:72:2: error: implicit declaration of function ‘WARN_ON_ONCE’; did you mean ‘WRITE_ONCE’? [-Werror=implicit-function-declaration]
  WARN_ON_ONCE(!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)));
  ^~~~~~~~~~~~
./arch/x86/include/asm/bitops.h:72:16: error: implicit declaration of function ‘IS_ALIGNED’; did you mean ‘IS_ENABLED’? [-Werror=implicit-function-declaration]
  WARN_ON_ONCE(!IS_ALIGNED((unsigned long)addr, sizeof(unsigned long)));
                ^~~~~~~~~~
I think it's because arch/x86/include/asm/bitops.h is included in
include/linux/kernel.h before IS_ALIGNED() is defined and in
include/asm-generic/bug.h before WARN_ON_ONCE() is defined.

How to write a right warn patch and solve the compilation issue?

Thanks.

-Fenghua