[PATCH] x86/split_lock: Remove dead kmsg formatting when split_lock_detect=fatal

Rong Zhang posted 1 patch 1 month, 3 weeks ago
arch/x86/kernel/cpu/bus_lock.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
[PATCH] x86/split_lock: Remove dead kmsg formatting when split_lock_detect=fatal
Posted by Rong Zhang 1 month, 3 weeks ago
sld_state_show() has a dead kmsg formatting:

	if (A) {
		...
	} else if (B) {
		pr_info(... A ? str1 : str2 ...);
	}

where A is always false in the second block, implied by the "if (A)
else" pattern. Hence, str2 is always used.

This seems to be some mysterious legacy inherited from the earlier patch
revisions of commit ebb1064e7c2e ("x86/traps: Handle #DB for bus lock").
Earlier revisions [1] did enable both sld and bld at the same time to
detect non-WB bus_locks when split_lock_detect=fatal, but that's no
longer true in the merged revision.

Remove it and translate the pr_info() into its equivalent form.

[1]: https://lore.kernel.org/r/20201121023624.3604415-3-fenghua.yu@intel.com/

Signed-off-by: Rong Zhang <i@rong.moe>
---
 arch/x86/kernel/cpu/bus_lock.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index dbc99a47be45..77eadf3b7040 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -413,9 +413,7 @@ static void sld_state_show(void)
 		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
 			pr_info("#AC: crashing the kernel on kernel split_locks and sending SIGBUS on user-space split_locks\n");
 		} else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
-			pr_info("#DB: sending SIGBUS on user-space bus_locks%s\n",
-				boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) ?
-				" from non-WB" : "");
+			pr_info("#DB: sending SIGBUS on user-space bus_locks\n");
 		}
 		break;
 	case sld_ratelimit:

base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
-- 
2.51.0
[PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()
Posted by Borislav Petkov 1 month ago
On Tue, Dec 16, 2025 at 02:11:51AM +0800, Rong Zhang wrote:
> sld_state_show() has a dead kmsg formatting:
> 
> 	if (A) {
> 		...
> 	} else if (B) {
> 		pr_info(... A ? str1 : str2 ...);
> 	}
> 
> where A is always false in the second block, implied by the "if (A)
> else" pattern. Hence, str2 is always used.
> 
> This seems to be some mysterious legacy inherited from the earlier patch
> revisions of commit ebb1064e7c2e ("x86/traps: Handle #DB for bus lock").
> Earlier revisions [1] did enable both sld and bld at the same time to
> detect non-WB bus_locks when split_lock_detect=fatal, but that's no
> longer true in the merged revision.
> 
> Remove it and translate the pr_info() into its equivalent form.
> 
> [1]: https://lore.kernel.org/r/20201121023624.3604415-3-fenghua.yu@intel.com/
> 
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>  arch/x86/kernel/cpu/bus_lock.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

While staring at this, that sld_state_show() function looks like it needs
a good scrubbin'. Here's a first attempt ontop:

---

From e52fe2e2009e488c720f3e98a77963145ac9153c Mon Sep 17 00:00:00 2001
From: "Borislav Petkov (AMD)" <bp@alien8.de>
Date: Sun, 4 Jan 2026 14:40:23 +0100
Subject: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()

Handle the easy cases first and leave the meat of the code at the end,
after having removed all possible gunk which makes it even more
unreadable than it is.

Have the CPU-going-offline check for both fatal and warning settings
because there's no point to have it only in the sld_warn case.

There should be no functional changes resulting from this cleanup.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/kernel/cpu/bus_lock.c | 39 ++++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index fb166662bc0d..811f87906c1e 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -391,34 +391,31 @@ static void __init split_lock_setup(struct cpuinfo_x86 *c)
 
 static void sld_state_show(void)
 {
+	const char *action = "warning";
+
 	if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) &&
 	    !boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
 		return;
 
-	switch (sld_state) {
-	case sld_off:
+	if (sld_state == sld_off) {
 		pr_info("disabled\n");
-		break;
-	case sld_warn:
-		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
-			pr_info("#AC: crashing the kernel on kernel split_locks and warning on user-space split_locks\n");
-			if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
-					      "x86/splitlock", NULL, splitlock_cpu_offline) < 0)
-				pr_warn("No splitlock CPU offline handler\n");
-		} else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
-			pr_info("#DB: warning on user-space bus_locks\n");
-		}
-		break;
-	case sld_fatal:
-		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
-			pr_info("#AC: crashing the kernel on kernel split_locks and sending SIGBUS on user-space split_locks\n");
-		else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
-			pr_info("#DB: sending SIGBUS on user-space bus_locks\n");
-		break;
-	case sld_ratelimit:
+		return;
+	} else if (sld_state == sld_ratelimit) {
 		if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
 			pr_info("#DB: setting system wide bus lock rate limit to %u/sec\n", bld_ratelimit.burst);
-		break;
+		return;
+	}
+
+	if (sld_state == sld_fatal)
+		action = "sending SIGBUS";
+
+	if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
+		pr_info("#AC: crashing the kernel on kernel split_locks and %s on user-space split_locks\n", action);
+		if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				      "x86/splitlock", NULL, splitlock_cpu_offline) < 0)
+			pr_warn("No splitlock CPU offline handler\n");
+	} else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
+		pr_info("#DB: %s on user-space bus_locks\n", action);
 	}
 }
 
-- 
2.51.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()
Posted by Xiaoyao Li 1 month ago
+ Tony

On 1/5/2026 3:17 AM, Borislav Petkov wrote:
>  From e52fe2e2009e488c720f3e98a77963145ac9153c Mon Sep 17 00:00:00 2001
> From: "Borislav Petkov (AMD)"<bp@alien8.de>
> Date: Sun, 4 Jan 2026 14:40:23 +0100
> Subject: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()
> 
> Handle the easy cases first and leave the meat of the code at the end,
> after having removed all possible gunk which makes it even more
> unreadable than it is.
> 
> Have the CPU-going-offline check for both fatal and warning settings
> because there's no point to have it only in the sld_warn case.

If I understand correctly, the CPU offline callback was added to avoid 
the case where a CPU is taken offline when split lock detection is 
disabled temporarily and before the delayed work to re-enable it being 
called. Since the MSR_TEST_CTRL is per-core scope, the sibling CPU on 
the same core may then be left running with split lock detection 
disabled without the delayed work to re-enable it.

For fatal mode, there is no such handling of temporarily disabling the 
feature and we don't need the CPU offline callback.

> There should be no functional changes resulting from this cleanup.
> 
> Signed-off-by: Borislav Petkov (AMD)<bp@alien8.de>
> ---
>   arch/x86/kernel/cpu/bus_lock.c | 39 ++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
> index fb166662bc0d..811f87906c1e 100644
> --- a/arch/x86/kernel/cpu/bus_lock.c
> +++ b/arch/x86/kernel/cpu/bus_lock.c
> @@ -391,34 +391,31 @@ static void __init split_lock_setup(struct cpuinfo_x86 *c)
>   
>   static void sld_state_show(void)
>   {
> +	const char *action = "warning";
> +
>   	if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) &&
>   	    !boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
>   		return;
>   
> -	switch (sld_state) {
> -	case sld_off:
> +	if (sld_state == sld_off) {
>   		pr_info("disabled\n");
> -		break;
> -	case sld_warn:
> -		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> -			pr_info("#AC: crashing the kernel on kernel split_locks and warning on user-space split_locks\n");
> -			if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> -					      "x86/splitlock", NULL, splitlock_cpu_offline) < 0)
> -				pr_warn("No splitlock CPU offline handler\n");
> -		} else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
> -			pr_info("#DB: warning on user-space bus_locks\n");
> -		}
> -		break;
> -	case sld_fatal:
> -		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> -			pr_info("#AC: crashing the kernel on kernel split_locks and sending SIGBUS on user-space split_locks\n");
> -		else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
> -			pr_info("#DB: sending SIGBUS on user-space bus_locks\n");
> -		break;
> -	case sld_ratelimit:
> +		return;
> +	} else if (sld_state == sld_ratelimit) {
>   		if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
>   			pr_info("#DB: setting system wide bus lock rate limit to %u/sec\n", bld_ratelimit.burst);
> -		break;
> +		return;
> +	}
> +
> +	if (sld_state == sld_fatal)
> +		action = "sending SIGBUS";
> +
> +	if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		pr_info("#AC: crashing the kernel on kernel split_locks and %s on user-space split_locks\n", action);
> +		if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				      "x86/splitlock", NULL, splitlock_cpu_offline) < 0)
> +			pr_warn("No splitlock CPU offline handler\n");
> +	} else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
> +		pr_info("#DB: %s on user-space bus_locks\n", action);
>   	}
>   }
>   
> -- 2.51.0
Re: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()
Posted by Borislav Petkov 1 month ago
On Wed, Jan 07, 2026 at 01:38:41PM +0800, Xiaoyao Li wrote:
> For fatal mode, there is no such handling of temporarily disabling the
> feature and we don't need the CPU offline callback.

Yah, I got that. But why?

Why is fatal mode different?

Why does it matter what the split lock mode is when we offline CPUs?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
RE: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()
Posted by Luck, Tony 1 month ago
> > For fatal mode, there is no such handling of temporarily disabling the
> > feature and we don't need the CPU offline callback.
>
> Yah, I got that. But why?
>
> Why is fatal mode different?
>
> Why does it matter what the split lock mode is when we offline CPUs?

It matters because the mode is shared by both hyper-threads, and you may
only offline one of them.

The offline handler isn't needed in the fatal case because split lock is
always enabled. So the "Unconditionally re-enable detection here."
won't change anything.

That said, it wouldn't break anything to run that. So if it makes the setup
code easier to read, it's OK to do this. But should have a comment to
describe what it going on.

-Tony
Re: [PATCH] x86/split_lock: Zap the unwieldy switch-case in sld_state_show()
Posted by Borislav Petkov 1 month ago
On Wed, Jan 07, 2026 at 04:41:22PM +0000, Luck, Tony wrote:
> It matters because the mode is shared by both hyper-threads, and you may
> only offline one of them.
> 
> The offline handler isn't needed in the fatal case because split lock is
> always enabled. So the "Unconditionally re-enable detection here."
> won't change anything.

/me goes and reads

b041b525dab9 ("x86/split_lock: Make life miserable for split lockers")

Oh wow, that's a fancy dance there :)
 
> That said, it wouldn't break anything to run that. So if it makes the setup
> code easier to read, it's OK to do this. But should have a comment to
> describe what it going on.

Right, exactly.

My goal is to have it be more readable by dialing down on the repetitive
stuff.

IOW, this (pasting the whole function):

static void sld_state_show(void)
{
        const char *action = "warning";
 
        if (!boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT) &&
            !boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
                return;
 
        if (sld_state == sld_off) {
                pr_info("disabled\n");
                return;
        } else if (sld_state == sld_ratelimit) {
                if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
                        pr_info("#DB: setting system wide bus lock rate limit to %u/sec\n", bld_ratelimit.burst);
                return;
        }
 
        if (sld_state == sld_fatal)
                action = "sending SIGBUS";
 
        if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
                pr_info("#AC: crashing the kernel on kernel split_locks and %s on user-space split_locks\n", action);
 
                /*
                 * This is handling the case where a CPU goes offline at the
                 * moment where split lock detection is disabled in the warn
                 * setting, see split_lock_warn(). It doesn't have any effect
                 * in the fatal case.
                 */
                if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                                      "x86/splitlock", NULL, splitlock_cpu_offline) < 0)
                        pr_warn("No splitlock CPU offline handler\n");
        } else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
                pr_info("#DB: %s on user-space bus_locks\n", action);
        }
}

Btw, looking some more, that MSR writing function could save a write if not
necessary:

diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index 811f87906c1e..bdf518d28310 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -164,7 +164,10 @@ static void sld_update_msr(bool on)
        if (on)
                test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
 
-       wrmsrq(MSR_TEST_CTRL, test_ctrl_val);
+       if (test_ctrl_val != msr_test_ctrl_cache) {
+               wrmsrq(MSR_TEST_CTRL, test_ctrl_val);
+               msr_test_ctrl_cache = test_ctrl_val;
+       }
 }
 
 void split_lock_init(void)

---

but maybe that's not important as slow path...

Also, from the looks of it, that cached value could be dropped in favor of
using msr_{set,clear}_bit(). But maybe something for another day...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
[tip: x86/cleanups] x86/split_lock: Remove dead string when split_lock_detect=fatal
Posted by tip-bot2 for Rong Zhang 1 month ago
The following commit has been merged into the x86/cleanups branch of tip:

Commit-ID:     6823f10dcc84f35ca652eff0448f7da3d3b26548
Gitweb:        https://git.kernel.org/tip/6823f10dcc84f35ca652eff0448f7da3d3b26548
Author:        Rong Zhang <i@rong.moe>
AuthorDate:    Tue, 16 Dec 2025 02:11:51 +08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Sun, 04 Jan 2026 14:26:02 +01:00

x86/split_lock: Remove dead string when split_lock_detect=fatal

sld_state_show() has a dead str1 below:

  if (A) {
  	...
  } else if (B) {
  	pr_info(... A ? str1 : str2 ...);
  }

where A is always false in the second block, implied by the "if (A) else"
pattern. Hence, str2 is always used.

This seems to be some mysterious legacy inherited from the earlier patch
revisions of

  ebb1064e7c2e ("x86/traps: Handle #DB for bus lock").

Earlier revisions¹ did enable both sld and bld at the same time to detect
non-WB bus_locks when split_lock_detect=fatal, but that's no longer true in
the merged revision.

Remove it and translate the pr_info() into its equivalent form.

¹ https://lore.kernel.org/r/20201121023624.3604415-3-fenghua.yu@intel.com

  [ bp: Massage commit message; simplify braces ]

Signed-off-by: Rong Zhang <i@rong.moe>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://patch.msgid.link/20251215182907.152881-1-i@rong.moe
---
 arch/x86/kernel/cpu/bus_lock.c |  9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index dbc99a4..fb16666 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -410,13 +410,10 @@ static void sld_state_show(void)
 		}
 		break;
 	case sld_fatal:
-		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
+		if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
 			pr_info("#AC: crashing the kernel on kernel split_locks and sending SIGBUS on user-space split_locks\n");
-		} else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) {
-			pr_info("#DB: sending SIGBUS on user-space bus_locks%s\n",
-				boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) ?
-				" from non-WB" : "");
-		}
+		else if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))
+			pr_info("#DB: sending SIGBUS on user-space bus_locks\n");
 		break;
 	case sld_ratelimit:
 		if (boot_cpu_has(X86_FEATURE_BUS_LOCK_DETECT))