* [PATCH v8 06/15] x86/cpufeatures: Enumerate MSR_IA32_CORE_CAPABILITY
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 04/15] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 00/15] x86/split_lock: Enable split lock detection
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, 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
^ permalink raw reply
* [PATCH v8 01/15] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* [PATCH v8 02/15] drivers/net/b44: Align pwol_mask to unsigned long for better performance
From: Fenghua Yu @ 2019-04-24 19:32 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li , Christopherson Sean J, Kalle Valo,
Michael Chan
Cc: linux-kernel, x86, kvm, netdev, linux-wireless, Fenghua Yu
In-Reply-To: <1556134382-58814-1-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply related
* Re: [PATCH] mac80211: Set CAN_REPLACE_PTK0 for SW crypto only drivers
From: Alexander Wetzel @ 2019-04-24 20:58 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <45ef6418002ddb01bc99a06a5c52e0dcd30afd4b.camel@sipsolutions.net>
Am 24.04.19 um 20:55 schrieb Johannes Berg:
> On Wed, 2019-04-24 at 19:32 +0200, Alexander Wetzel wrote:
>> Mac80211 SW crypto handles replacing PTK keys correctly.
>>
>> Don't trigger needless warnings or workarounds when the driver can only
>> use the known good SW crypto provided by mac80211.
>>
>> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
>> ---
>> net/mac80211/main.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index e56650a9838e..2b608044ae23 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -1060,6 +1060,13 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>> wiphy_ext_feature_set(local->hw.wiphy,
>> NL80211_EXT_FEATURE_EXT_KEY_ID);
>>
>> + /* Mac80211 and therefore all cards only using SW crypto are able to
>> + * handle PTK rekeys correctly
>> + */
>> + if (!local->ops->set_key)
>> + wiphy_ext_feature_set(local->hw.wiphy,
>> + NL80211_EXT_FEATURE_CAN_REPLACE_PTK0);
>
> Now I wonder - shouldn't the same A-MPDU issue apply here? After all, if
> you replace the PTK 0 surely you shouldn't use different ones for the
> same frame in an A-MPDU?
Not from what I found in IEEE 802.11. It's only forbidden to mix keyIDs,
not MPDUs using different keys. And without Extended Key ID the keyID
can only be zero. So from a standard point of view we are ok, no keyID
mixing possible.
From a practical point of view cards like mvm cards will for sure
corrupt MPDUs aggregated in a A-MPDU when different key were used for
them. But we still don't care:-)
We'll corrupt the MPDU's encoded with either the old or the new key
anyhow and we don't care which ones. After all the card will only have
one key active for key ID 0 at any time won't have the second key
installed at all.
Alexander
^ permalink raw reply
* Re: [PATCH] mac80211: Set CAN_REPLACE_PTK0 for SW crypto only drivers
From: Johannes Berg @ 2019-04-24 21:01 UTC (permalink / raw)
To: Alexander Wetzel; +Cc: linux-wireless
In-Reply-To: <0e1d8f51-ca1b-a55e-73a1-d4b95fe3f0b5@wetzel-home.de>
On Wed, 2019-04-24 at 22:58 +0200, Alexander Wetzel wrote:
>
> > Now I wonder - shouldn't the same A-MPDU issue apply here? After all, if
> > you replace the PTK 0 surely you shouldn't use different ones for the
> > same frame in an A-MPDU?
>
> Not from what I found in IEEE 802.11. It's only forbidden to mix keyIDs,
> not MPDUs using different keys. And without Extended Key ID the keyID
> can only be zero. So from a standard point of view we are ok, no keyID
> mixing possible.
Yeah, well, the standard probably didn't consider this. From an
implementation POV, having two subframes with different keys will not
really be possible, *especially* if they have the same key ID.
I think this basically was not considered in spec writing.
> From a practical point of view cards like mvm cards will for sure
> corrupt MPDUs aggregated in a A-MPDU when different key were used for
> them. But we still don't care:-)
>
> We'll corrupt the MPDU's encoded with either the old or the new key
> anyhow and we don't care which ones. After all the card will only have
> one key active for key ID 0 at any time won't have the second key
> installed at all.
Yeah, ok, fair point.
johannes
^ permalink raw reply
* Re: [PATCH v2 wireless-drivers-next] rtlwifi: rtl8723ae: Fix missing break in switch statement
From: Gustavo A. R. Silva @ 2019-04-24 22:17 UTC (permalink / raw)
To: Ping-Ke Shih, Kalle Valo, David S. Miller, Larry Finger
Cc: linux-wireless, netdev, linux-kernel, Kees Cook
In-Reply-To: <20190416151722.GA31598@embeddedor>
Hi all,
Friendly ping:
Who can take this?
Thanks
--
Gustavo
On 4/16/19 10:17 AM, Gustavo A. R. Silva wrote:
> Add missing break statement in order to prevent the code from falling
> through to case 0x1025, and erroneously setting rtlhal->oem_id to
> RT_CID_819X_ACER when rtlefuse->eeprom_svid is equal to 0x10EC and
> none of the cases in switch (rtlefuse->eeprom_smid) match.
>
> This bug was found thanks to the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Fixes: 238ad2ddf34b ("rtlwifi: rtl8723ae: Clean up the hardware info routine")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> Changes in v2:
> - Update tag from wireless-drivers to wireless-drivers-next
>
> drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
> index 6bab162e1bb8..655460f61bbc 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
> @@ -1675,6 +1675,7 @@ static void _rtl8723e_read_adapter_info(struct ieee80211_hw *hw,
> rtlhal->oem_id = RT_CID_819X_LENOVO;
> break;
> }
> + break;
> case 0x1025:
> rtlhal->oem_id = RT_CID_819X_ACER;
> break;
>
^ permalink raw reply
* [PATCH -next] NFC: st95hf: remove set but not used variables 'dev, nfcddev'
From: YueHaibing @ 2019-04-25 2:07 UTC (permalink / raw)
To: Samuel Ortiz, Daniel Mack, David S. Miller, Daniel Gomez
Cc: YueHaibing, linux-wireless, netdev, kernel-janitors
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/nfc/st95hf/core.c: In function 'st95hf_irq_thread_handler':
drivers/nfc/st95hf/core.c:786:26: warning:
variable 'nfcddev' set but not used [-Wunused-but-set-variable]
drivers/nfc/st95hf/core.c:784:17: warning:
variable 'dev' set but not used [-Wunused-but-set-variable]
They are never used since introduction in
commit cab47333f0f7 ("NFC: Add STMicroelectronics ST95HF driver")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/nfc/st95hf/core.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/nfc/st95hf/core.c b/drivers/nfc/st95hf/core.c
index 815194bfda8e..f3bec0a84595 100644
--- a/drivers/nfc/st95hf/core.c
+++ b/drivers/nfc/st95hf/core.c
@@ -781,9 +781,7 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
int result = 0;
int res_len;
static bool wtx;
- struct device *dev;
struct device *spidevice;
- struct nfc_digital_dev *nfcddev;
struct sk_buff *skb_resp;
struct st95hf_context *stcontext =
(struct st95hf_context *)st95hfcontext;
@@ -828,8 +826,6 @@ static irqreturn_t st95hf_irq_thread_handler(int irq, void *st95hfcontext)
goto end;
}
- dev = &stcontext->nfcdev->dev;
- nfcddev = stcontext->ddev;
if (skb_resp->data[2] == WTX_REQ_FROM_TAG) {
/* Request for new FWT from tag */
result = st95hf_handle_wtx(stcontext, true, skb_resp->data[3]);
^ permalink raw reply related
* Re: [PATCH] brcmfmac: remove the duplicate line of sending mail box interrupt
From: Wright Feng @ 2019-04-25 3:37 UTC (permalink / raw)
To: Arend Van Spriel, franky.lin@broadcom.com,
hante.meuleman@broadcom.com, kvalo@codeaurora.org, Chi-Hsien Lin
Cc: linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
In-Reply-To: <164b59e8-633b-84cc-179f-9bdd7ec696a7@broadcom.com>
On 2019/4/24 下午 04:46, Arend Van Spriel wrote:
> On 4/24/2019 7:24 AM, Arend Van Spriel wrote:
>> On April 22, 2019 4:47:31 AM Wright Feng <Wright.Feng@cypress.com> wrote:
>>
>>> The line is duplicate so remove it from pcie.c. We don't need to send
>>> host to dongle mail box interrupt twice after writing data.
>>
>> Hi Wright,
>>
>> Thanks for the patch. I am a bit early as it is not uncommon that
>> hardware requires an extra kick. Might be the case here for some
>> chips. Will follow-up on it.
>
> Hi Wright,
>
> I checked in our dhd driver code and it also does a double write. Turns
> out to be a hardware workaround so we need to keep this. It is worth a
> comment though.
>
Hi Arend,
Thanks for the information.
NXP and I worked on suspend/resume stress test issue with 1FD(4359b1)
two months ago, and we found one of root cause is duplicate mailbox
interrupt.
I've checked the history of the hardware issue internally, and it seems that
the issue is only on PCIEGEN2 revision 1 to 13.
So is that okay we can skip the hardware workaround when PCIEGEN2
revision is greater than 13 and add the comment on it in my patch v2?
Regards,
Wright
> Thanks,
> Arend
^ permalink raw reply
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
From: Ingo Molnar @ 2019-04-25 5:45 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-6-git-send-email-fenghua.yu@intel.com>
* 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
^ permalink raw reply
* Re: [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
From: Ingo Molnar @ 2019-04-25 6:07 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-11-git-send-email-fenghua.yu@intel.com>
* 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
^ permalink raw reply
* Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register
From: Ingo Molnar @ 2019-04-25 6:21 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-10-git-send-email-fenghua.yu@intel.com>
* 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
^ permalink raw reply
* Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Ingo Molnar @ 2019-04-25 6:31 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-16-git-send-email-fenghua.yu@intel.com>
* 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
^ permalink raw reply
* [PATCH v2] brcmfmac: send mailbox interrupt twice for specific hardware device
From: Wright Feng @ 2019-04-25 7:05 UTC (permalink / raw)
To: arend.vanspriel@broadcom.com, franky.lin@broadcom.com,
hante.meuleman@broadcom.com, kvalo@codeaurora.org, Chi-Hsien Lin
Cc: Wright Feng, linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
For PCIE wireless device with core revision less than 14, device may miss
PCIE to System Backplane Interrupt via PCIEtoSBMailbox. So add sending
mail box interrupt twice as a hardware workaround.
Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index fd3968f..66ee92b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -698,7 +698,11 @@ brcmf_pcie_send_mb_data(struct brcmf_pciedev_info *devinfo, u32 htod_mb_data)
brcmf_pcie_write_tcm32(devinfo, addr, htod_mb_data);
pci_write_config_dword(devinfo->pdev, BRCMF_PCIE_REG_SBMBX, 1);
- pci_write_config_dword(devinfo->pdev, BRCMF_PCIE_REG_SBMBX, 1);
+
+ /* Send mailbox interrupt twice as a hardware workaround */
+ core = brcmf_chip_get_core(devinfo->ci, BCMA_CORE_PCIE2);
+ if (core->rev <= 13)
+ pci_write_config_dword(devinfo->pdev, BRCMF_PCIE_REG_SBMBX, 1);
return 0;
}
--
2.1.0
^ permalink raw reply related
* [PATCH v5.1] iwlwifi: mvm: fix merge damage in iwl_mvm_vif_dbgfs_register()
From: Luca Coelho @ 2019-04-25 7:14 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Luca Coelho
From: Luca Coelho <luciano.coelho@intel.com>
When I rebased Greg's patch, I accidentally left the old if block that
was already there. Remove it.
Fixes: 154d4899e411 ("iwlwifi: mvm: properly check debugfs dentry before using it")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
I am going to include this patch when I send my next pull request for
v5.1. This is not so important, but it's simple and clear and will
prevent further merge conflicts later.
drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
index 738eddb2e7ac..6925527d8457 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
@@ -780,12 +780,6 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
return;
}
- if (!mvmvif->dbgfs_dir) {
- IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
- dbgfs_dir);
- return;
- }
-
if (iwlmvm_mod_params.power_scheme != IWL_POWER_SCHEME_CAM &&
((vif->type == NL80211_IFTYPE_STATION && !vif->p2p) ||
(vif->type == NL80211_IFTYPE_STATION && vif->p2p)))
--
2.20.1
^ permalink raw reply related
* Re: brcmfmac: NULL pointer dereference during brcmf_detach() after firmware crash
From: Arend Van Spriel @ 2019-04-25 7:17 UTC (permalink / raw)
To: Rafał Miłecki
Cc: linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
brcm80211-dev-list, Aaron Blair
In-Reply-To: <CACna6rwF7rai_PMY3VtYXD=uBAT+jHsr8zrWP28vKjUx2S7Uyg@mail.gmail.com>
On 4/18/2019 1:55 PM, Rafał Miłecki wrote:
> On Fri, 15 Feb 2019 at 07:15, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On Thu, 14 Feb 2019 at 23:37, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 2/14/2019 11:30 PM, Rafał Miłecki wrote:
>>>> I've just found a well reproducible brcmfmac crash (NULL pointer
>>>> dereference).
>>>>
>>>> Steps:
>>>> 1. Wait for or trigger a FullMAC firmware crash
>>>> 2. Wait for some skb to get queued on a flowring
>>>> 3. Call rmmod brcmfmac
>>>>
>>>> Problem:
>>>> There is a NULL pointer dereference in one of the brcmf_detach() calls.
>>>>
>>>> Explanation:
>>>> brcmf_detach() first frees all "ifp"s and then deletes flowrings. If any
>>>> flowring has a skb it results in calling brcmf_txfinalize() which tries
>>>> to access "ifp" (struct brcmf_if) which is a NULL.
>>>
>>> Hi Rafał,
>>>
>>> Thanks for diving in. That was my suspicion. Does it mean you are
>>> working on a patch or shall I take care of it.
>>
>> It would be nice to have someone more experienced with detaching &
>> rings look at it. Is adding a simple
>> if (ifp)
>> enough? Or should that code get redesigned? Should we e.g. reorder detach order?
>
> Hi Arend, would you find a moment to look at that crash, please?
Hi Rafał,
Sorry for getting back on this so late. The driver tries to gracefully
teardown stuff by sending firmware commands to the device. I think that
makes no sense as that level of communication is not possible once our
driver .remove() callback is called. So I think upon calling
brcmf_detach() we should cleanup everything bottom-up. I will rework the
code and see how that goes.
Thanks,
Arend
^ permalink raw reply
* Re: [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
From: Thomas Gleixner @ 2019-04-25 7:29 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-11-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply
* Re: [PATCH v2] brcmfmac: send mailbox interrupt twice for specific hardware device
From: Arend Van Spriel @ 2019-04-25 7:37 UTC (permalink / raw)
To: Wright Feng, franky.lin@broadcom.com, hante.meuleman@broadcom.com,
kvalo@codeaurora.org, Chi-Hsien Lin
Cc: linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
In-Reply-To: <1556175939-174559-1-git-send-email-wright.feng@cypress.com>
On 4/25/2019 9:05 AM, Wright Feng wrote:
> For PCIE wireless device with core revision less than 14, device may miss
> PCIE to System Backplane Interrupt via PCIEtoSBMailbox. So add sending
> mail box interrupt twice as a hardware workaround.
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
^ permalink raw reply
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
From: Thomas Gleixner @ 2019-04-25 7:42 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless, Xiaoyao Li
In-Reply-To: <1556134382-58814-13-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply
* Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
From: Thomas Gleixner @ 2019-04-25 7:50 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-14-git-send-email-fenghua.yu@intel.com>
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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox