* [PATCH v8 01/15] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 02/15] drivers/net/b44: Align pwol_mask to unsigned long for better performance Fenghua Yu
` (13 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* [PATCH v8 02/15] drivers/net/b44: Align pwol_mask to unsigned long for better performance
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 01/15] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations Fenghua Yu
` (12 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 01/15] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 02/15] drivers/net/b44: Align pwol_mask to unsigned long for better performance Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-25 17:12 ` Kalle Valo
2019-04-24 19:32 ` [PATCH v8 04/15] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access Fenghua Yu
` (11 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations
2019-04-24 19:32 ` [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations Fenghua Yu
@ 2019-04-25 17:12 ` Kalle Valo
0 siblings, 0 replies; 45+ messages in thread
From: Kalle Valo @ 2019-04-25 17:12 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, Michael Chan, linux-kernel, x86, kvm,
netdev, linux-wireless, Fenghua Yu
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 04/15] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (2 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 03/15] wlcore: simplify/fix/optimize reg_ch_conf_pending operations Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit Fenghua Yu
` (10 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (3 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 04/15] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-25 5:45 ` Ingo Molnar
2019-04-24 19:32 ` [PATCH v8 06/15] x86/cpufeatures: Enumerate MSR_IA32_CORE_CAPABILITY Fenghua Yu
` (9 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-24 19:32 ` [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit Fenghua Yu
@ 2019-04-25 5:45 ` Ingo Molnar
2019-04-25 19:01 ` Fenghua Yu
0 siblings, 1 reply; 45+ messages in thread
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
* 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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-25 5:45 ` Ingo Molnar
@ 2019-04-25 19:01 ` Fenghua Yu
2019-04-25 19:47 ` Ingo Molnar
0 siblings, 1 reply; 45+ messages in thread
From: Fenghua Yu @ 2019-04-25 19:01 UTC (permalink / raw)
To: Ingo Molnar
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-25 19:01 ` Fenghua Yu
@ 2019-04-25 19:47 ` Ingo Molnar
2019-04-25 19:51 ` Fenghua Yu
0 siblings, 1 reply; 45+ messages in thread
From: Ingo Molnar @ 2019-04-25 19:47 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
* 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-25 19:47 ` Ingo Molnar
@ 2019-04-25 19:51 ` Fenghua Yu
2019-04-25 20:08 ` Ingo Molnar
0 siblings, 1 reply; 45+ messages in thread
From: Fenghua Yu @ 2019-04-25 19:51 UTC (permalink / raw)
To: Ingo Molnar
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-25 19:51 ` Fenghua Yu
@ 2019-04-25 20:08 ` Ingo Molnar
2019-04-25 20:22 ` Fenghua Yu
0 siblings, 1 reply; 45+ messages in thread
From: Ingo Molnar @ 2019-04-25 20:08 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
* 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-25 20:08 ` Ingo Molnar
@ 2019-04-25 20:22 ` Fenghua Yu
2019-04-26 6:00 ` Ingo Molnar
0 siblings, 1 reply; 45+ messages in thread
From: Fenghua Yu @ 2019-04-25 20:22 UTC (permalink / raw)
To: Ingo Molnar
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-25 20:22 ` Fenghua Yu
@ 2019-04-26 6:00 ` Ingo Molnar
2019-05-06 0:12 ` Fenghua Yu
0 siblings, 1 reply; 45+ messages in thread
From: Ingo Molnar @ 2019-04-26 6:00 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
* 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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
2019-04-26 6:00 ` Ingo Molnar
@ 2019-05-06 0:12 ` Fenghua Yu
0 siblings, 0 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-05-06 0:12 UTC (permalink / raw)
To: Ingo Molnar
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 06/15] x86/cpufeatures: Enumerate MSR_IA32_CORE_CAPABILITY
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (4 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 07/15] x86/split_lock: Enumerate split lock detection by MSR_IA32_CORE_CAPABILITY Fenghua Yu
` (8 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* [PATCH v8 07/15] x86/split_lock: Enumerate split lock detection by MSR_IA32_CORE_CAPABILITY
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (5 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 06/15] x86/cpufeatures: Enumerate MSR_IA32_CORE_CAPABILITY Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 08/15] x86/split_lock: Enumerate split lock detection on Icelake mobile processor Fenghua Yu
` (7 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v8 08/15] x86/split_lock: Enumerate split lock detection on Icelake mobile processor
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (6 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 07/15] x86/split_lock: Enumerate split lock detection by MSR_IA32_CORE_CAPABILITY Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register Fenghua Yu
` (6 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (7 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 08/15] x86/split_lock: Enumerate split lock detection on Icelake mobile processor Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-25 6:21 ` Ingo Molnar
2019-04-24 19:32 ` [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock Fenghua Yu
` (5 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register
2019-04-24 19:32 ` [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register Fenghua Yu
@ 2019-04-25 6:21 ` Ingo Molnar
2019-04-25 19:48 ` Fenghua Yu
0 siblings, 1 reply; 45+ messages in thread
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
* 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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register
2019-04-25 6:21 ` Ingo Molnar
@ 2019-04-25 19:48 ` Fenghua Yu
0 siblings, 0 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-04-25 19:48 UTC (permalink / raw)
To: Ingo Molnar
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (8 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-25 6:07 ` Ingo Molnar
2019-04-25 7:29 ` Thomas Gleixner
2019-04-24 19:32 ` [PATCH v8 11/15] kvm/x86: Emulate MSR IA32_CORE_CAPABILITY Fenghua Yu
` (4 subsequent siblings)
14 siblings, 2 replies; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
2019-04-24 19:32 ` [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock Fenghua Yu
@ 2019-04-25 6:07 ` Ingo Molnar
2019-04-25 7:29 ` Thomas Gleixner
1 sibling, 0 replies; 45+ messages in thread
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
* 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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
2019-04-24 19:32 ` [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock Fenghua Yu
2019-04-25 6:07 ` Ingo Molnar
@ 2019-04-25 7:29 ` Thomas Gleixner
1 sibling, 0 replies; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* [PATCH v8 11/15] kvm/x86: Emulate MSR IA32_CORE_CAPABILITY
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (9 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-24 19:32 ` [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL Fenghua Yu
` (3 subsequent siblings)
14 siblings, 0 replies; 45+ messages in thread
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, Xiaoyao Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (10 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 11/15] kvm/x86: Emulate MSR IA32_CORE_CAPABILITY Fenghua Yu
@ 2019-04-24 19:32 ` Fenghua Yu
2019-04-25 7:42 ` Thomas Gleixner
2019-04-24 19:33 ` [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default Fenghua Yu
` (2 subsequent siblings)
14 siblings, 1 reply; 45+ messages in thread
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, Xiaoyao Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-24 19:32 ` [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL Fenghua Yu
@ 2019-04-25 7:42 ` Thomas Gleixner
2019-04-27 12:20 ` Xiaoyao Li
0 siblings, 1 reply; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-25 7:42 ` Thomas Gleixner
@ 2019-04-27 12:20 ` Xiaoyao Li
2019-04-28 7:09 ` Thomas Gleixner
0 siblings, 1 reply; 45+ messages in thread
From: Xiaoyao Li @ 2019-04-27 12:20 UTC (permalink / raw)
To: Thomas Gleixner, Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Christopherson Sean J, Kalle Valo, Michael Chan, linux-kernel,
x86, kvm, netdev, linux-wireless
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
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-27 12:20 ` Xiaoyao Li
@ 2019-04-28 7:09 ` Thomas Gleixner
2019-04-28 7:34 ` Xiaoyao Li
2019-04-29 5:21 ` Xiaoyao Li
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Gleixner @ 2019-04-28 7:09 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Fenghua Yu, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-28 7:09 ` Thomas Gleixner
@ 2019-04-28 7:34 ` Xiaoyao Li
2019-04-29 7:31 ` Thomas Gleixner
2019-04-29 5:21 ` Xiaoyao Li
1 sibling, 1 reply; 45+ messages in thread
From: Xiaoyao Li @ 2019-04-28 7:34 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Fenghua Yu, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
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
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-28 7:34 ` Xiaoyao Li
@ 2019-04-29 7:31 ` Thomas Gleixner
0 siblings, 0 replies; 45+ messages in thread
From: Thomas Gleixner @ 2019-04-29 7:31 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Fenghua Yu, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
2019-04-28 7:09 ` Thomas Gleixner
2019-04-28 7:34 ` Xiaoyao Li
@ 2019-04-29 5:21 ` Xiaoyao Li
1 sibling, 0 replies; 45+ messages in thread
From: Xiaoyao Li @ 2019-04-29 5:21 UTC (permalink / raw)
To: Thomas Gleixner, Xiaoyao Li, Paolo Bonzini
Cc: Fenghua Yu, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Christopherson Sean J, Kalle Valo, Michael Chan, linux-kernel,
x86, kvm, netdev, linux-wireless
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
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (11 preceding siblings ...)
2019-04-24 19:32 ` [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL Fenghua Yu
@ 2019-04-24 19:33 ` Fenghua Yu
2019-04-25 7:50 ` Thomas Gleixner
2019-04-25 10:52 ` David Laight
2019-04-24 19:33 ` [PATCH v8 14/15] x86/split_lock: Disable split lock detection by kernel parameter "nosplit_lock_detect" Fenghua Yu
2019-04-24 19:33 ` [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time Fenghua Yu
14 siblings, 2 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-04-24 19:33 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
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-24 19:33 ` [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default Fenghua Yu
@ 2019-04-25 7:50 ` Thomas Gleixner
2019-05-06 21:39 ` Fenghua Yu
2019-04-25 10:52 ` David Laight
1 sibling, 1 reply; 45+ messages in thread
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
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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-25 7:50 ` Thomas Gleixner
@ 2019-05-06 21:39 ` Fenghua Yu
0 siblings, 0 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-05-06 21:39 UTC (permalink / raw)
To: Thomas Gleixner
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-24 19:33 ` [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default Fenghua Yu
2019-04-25 7:50 ` Thomas Gleixner
@ 2019-04-25 10:52 ` David Laight
2019-04-25 10:58 ` Thomas Gleixner
1 sibling, 1 reply; 45+ messages in thread
From: David Laight @ 2019-04-25 10:52 UTC (permalink / raw)
To: 'Fenghua Yu',
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
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)
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-25 10:52 ` David Laight
@ 2019-04-25 10:58 ` Thomas Gleixner
2019-04-25 11:13 ` David Laight
2019-05-07 20:48 ` Fenghua Yu
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Gleixner @ 2019-04-25 10:58 UTC (permalink / raw)
To: David Laight
Cc: 'Fenghua Yu',
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-25 10:58 ` Thomas Gleixner
@ 2019-04-25 11:13 ` David Laight
2019-04-25 11:41 ` Peter Zijlstra
2019-04-25 13:04 ` Thomas Gleixner
2019-05-07 20:48 ` Fenghua Yu
1 sibling, 2 replies; 45+ messages in thread
From: David Laight @ 2019-04-25 11:13 UTC (permalink / raw)
To: 'Thomas Gleixner'
Cc: 'Fenghua Yu',
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
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)
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-25 11:13 ` David Laight
@ 2019-04-25 11:41 ` Peter Zijlstra
2019-04-25 13:04 ` Thomas Gleixner
1 sibling, 0 replies; 45+ messages in thread
From: Peter Zijlstra @ 2019-04-25 11:41 UTC (permalink / raw)
To: David Laight
Cc: 'Thomas Gleixner', 'Fenghua Yu',
Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Ravi V Shankar, Xiaoyao Li,
Christopherson Sean J, Kalle Valo, Michael Chan, linux-kernel,
x86, kvm, netdev, linux-wireless
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-25 11:13 ` David Laight
2019-04-25 11:41 ` Peter Zijlstra
@ 2019-04-25 13:04 ` Thomas Gleixner
1 sibling, 0 replies; 45+ messages in thread
From: Thomas Gleixner @ 2019-04-25 13:04 UTC (permalink / raw)
To: David Laight
Cc: 'Fenghua Yu',
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
2019-04-25 10:58 ` Thomas Gleixner
2019-04-25 11:13 ` David Laight
@ 2019-05-07 20:48 ` Fenghua Yu
1 sibling, 0 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-05-07 20:48 UTC (permalink / raw)
To: Thomas Gleixner
Cc: David Laight, 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
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v8 14/15] x86/split_lock: Disable split lock detection by kernel parameter "nosplit_lock_detect"
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (12 preceding siblings ...)
2019-04-24 19:33 ` [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default Fenghua Yu
@ 2019-04-24 19:33 ` Fenghua Yu
2019-04-24 19:33 ` [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time Fenghua Yu
14 siblings, 0 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-04-24 19:33 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
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
2019-04-24 19:32 [PATCH v8 00/15] x86/split_lock: Enable split lock detection Fenghua Yu
` (13 preceding siblings ...)
2019-04-24 19:33 ` [PATCH v8 14/15] x86/split_lock: Disable split lock detection by kernel parameter "nosplit_lock_detect" Fenghua Yu
@ 2019-04-24 19:33 ` Fenghua Yu
2019-04-25 6:31 ` Ingo Molnar
14 siblings, 1 reply; 45+ messages in thread
From: Fenghua Yu @ 2019-04-24 19:33 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
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
2019-04-24 19:33 ` [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time Fenghua Yu
@ 2019-04-25 6:31 ` Ingo Molnar
2019-05-06 0:21 ` Fenghua Yu
0 siblings, 1 reply; 45+ messages in thread
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
* 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 [flat|nested] 45+ messages in thread
* Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
2019-04-25 6:31 ` Ingo Molnar
@ 2019-05-06 0:21 ` Fenghua Yu
0 siblings, 0 replies; 45+ messages in thread
From: Fenghua Yu @ 2019-05-06 0:21 UTC (permalink / raw)
To: Ingo Molnar
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
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
^ permalink raw reply [flat|nested] 45+ messages in thread