Patchwork [v5,12/18] x86/split_lock: Enable #AC for split lock by default

login
register
mail settings
Submitter Fenghua Yu
Date March 12, 2019, 11 p.m.
Message ID <1552431636-31511-13-git-send-email-fenghua.yu@intel.com>
Download mbox | patch
Permalink /patch/747769/
State New
Headers show

Comments

Fenghua Yu - March 12, 2019, 11 p.m.
Split locked access locks bus and degradates overall memory access
performance. When split lock detection feature is enumerated, we want to
enable the feature by default to find any split lock issue and then fix
the issue.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
Dave Hansen - March 12, 2019, 11:43 p.m.
On 3/12/19 4:00 PM, Fenghua Yu wrote:
> Split locked access locks bus and degradates overall memory access

Either "split locked accesses lock" or "A split lock access locks".

s/degradates/degrades/

> performance. When split lock detection feature is enumerated, we want to

		   ^ the

Also, you need to go back and remove all the "we"'s.  Our friendly x86
maintainers prefer phrasing this like:

	 When split lock detection feature is enumerated, enable the 	
	feature...

> enable the feature by default to find any split lock issue and then fix
> the issue.

Generally, the changelogs here are passable but pretty rough.  They have
a ton of issues like this and I'm sure they can be improved.  I'm sure
that with some love an attention they can be brought up to the highest
standards.

> +#define DISABLE_SPLIT_LOCK_DETECT 0
> +#define ENABLE_SPLIT_LOCK_DETECT  1

These are a bit overkill, but oh well.

> +static int split_lock_detect_val;
> +
>  /*
>   * Just in case our CPU detection goes bad, or you have a weird system,
>   * allow a way to override the automatic disabling of MPX.
> @@ -161,10 +166,35 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
>  	return false;
>  }
>  
> +static u32 new_sp_test_ctl_val(u32 test_ctl_val)
> +{
> +	/* Change the split lock setting. */
> +	if (split_lock_detect_val == DISABLE_SPLIT_LOCK_DETECT)
> +		test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> +	else
> +		test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> +
> +	return test_ctl_val;
> +}
> +
> +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		u32 l, h;
> +
> +		rdmsr(MSR_TEST_CTL, l, h);
> +		l = new_sp_test_ctl_val(l);
> +		wrmsr(MSR_TEST_CTL, l, h);
> +		pr_info_once("#AC for split lock is enabled\n");
> +	}
> +}

Please make sure you've removed all the "#AC for split lock"
nomenclature in these patches.  It's acronym nonsense and there's no
reason to be so oblique.  Just say "split lock detection enabled".  This
also needs a proper prefix, like "x86/intel:".  Look around for examples.

Patch

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 889390d51c17..561f7d50246a 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -31,6 +31,11 @@ 
 #include <asm/apic.h>
 #endif
 
+#define DISABLE_SPLIT_LOCK_DETECT 0
+#define ENABLE_SPLIT_LOCK_DETECT  1
+
+static int split_lock_detect_val;
+
 /*
  * Just in case our CPU detection goes bad, or you have a weird system,
  * allow a way to override the automatic disabling of MPX.
@@ -161,10 +166,35 @@  static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 	return false;
 }
 
+static u32 new_sp_test_ctl_val(u32 test_ctl_val)
+{
+	/* Change the split lock setting. */
+	if (split_lock_detect_val == DISABLE_SPLIT_LOCK_DETECT)
+		test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
+	else
+		test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
+
+	return test_ctl_val;
+}
+
+static void init_split_lock_detect(struct cpuinfo_x86 *c)
+{
+	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
+		u32 l, h;
+
+		rdmsr(MSR_TEST_CTL, l, h);
+		l = new_sp_test_ctl_val(l);
+		wrmsr(MSR_TEST_CTL, l, h);
+		pr_info_once("#AC for split lock is enabled\n");
+	}
+}
+
 static void early_init_intel(struct cpuinfo_x86 *c)
 {
 	u64 misc_enable;
 
+	init_split_lock_detect(c);
+
 	/* Unmask CPUID levels if masked: */
 	if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
 		if (msr_clear_bit(MSR_IA32_MISC_ENABLE,
@@ -1048,6 +1078,8 @@  void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 	 */
 	rdmsrl(MSR_IA32_CORE_CAPABILITY, ia32_core_cap);
 
-	if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT)
+	if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT) {
 		setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
+		split_lock_detect_val = 1;
+	}
 }