[PATCH v7 0/2] Fix and optimization of split_lock_detection

[PATCH v7 0/2] Fix and optimization of split_lock_detection

[Xiaoyao Li]

This series is split from "[PATCH v6 0/8] x86/split_lock: Fix and
virtualization of split lock detection"[1]. It contains one fix for the
initialization flow of split_lock_detection and one optimiazation for
runtime TEST_CTRL MSR access.

Other patches of [1] needs more time to improve.

Thanks.

[1]: https://lore.kernel.org/kvm/20200324151859.31068-1-xiaoyao.li@intel.com/

Changes in v7:
 - only pick patch 1 and patch 2, and hold all the left.
 - Update SLD bit on each processor based on sld_state.

Changes in v6:
 - Drop the sld_not_exist flag and use X86_FEATURE_SPLIT_LOCK_DETECT to
   check whether need to init split lock detection. [tglx]
 - Use tglx's method to verify the existence of split lock detectoin.
 - small optimization of sld_update_msr() that the default value of
   msr_test_ctrl_cache has split_lock_detect bit cleared.
 - Drop the patch3 in v5 that introducing kvm_only option. [tglx]
 - Rebase patch4-8 to kvm/queue.
 - use the new kvm-cpu-cap to expose X86_FEATURE_CORE_CAPABILITIES in
   Patch 6.

Changes in v5:
 - Use X86_FEATURE_SPLIT_LOCK_DETECT flag in kvm to ensure split lock
   detection is really supported.
 - Add and export sld related helper functions in their related usecase 
   kvm patches.

Changes in v4:
 - Add patch 1 to rework the initialization flow of split lock
   detection.
 - Drop percpu MSR_TEST_CTRL cache, just use a static variable to cache
   the reserved/unused bit of MSR_TEST_CTRL. [Sean]
 - Add new option for split_lock_detect kernel param.
 - Changlog refinement. [Sean]
 - Add a new patch to enable MSR_TEST_CTRL for intel guest. [Sean]


Xiaoyao Li (2):
  x86/split_lock: Rework the initialization flow of split lock detection
  x86/split_lock: Avoid runtime reads of the TEST_CTRL MSR

 arch/x86/kernel/cpu/intel.c | 85 +++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 37 deletions(-)

-- 
2.20.1

[PATCH v7 1/2] x86/split_lock: Rework the initialization flow of split lock detection

[Xiaoyao Li]

Current initialization flow of split lock detection has following
issues:

1. It assumes the initial value of MSR_TEST_CTRL.SPLIT_LOCK_DETECT to be
   zero. However, it's possible that BIOS/firmware has set it.

2. X86_FEATURE_SPLIT_LOCK_DETECT flag is unconditionally set even if
   there is a virtualization flaw that FMS indicates the existence while
   it's actually not supported.

Rework the initialization flow to solve above issues. In detail,
explicitly clear and set split_lock_detect bit to verify MSR_TEST_CTRL
can be accessed, and rdmsr after wrmsr to ensure bit is cleared/set
successfully.

X86_FEATURE_SPLIT_LOCK_DETECT flag is set only when the feature does
exist and the feature is not disabled with kernel param
"split_lock_detect=off"

On each processor, explicitly updating the SPLIT_LOCK_DETECT bit based
on sld_sate in split_lock_init() since BIOS/firmware may touch it.

Originally-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 78 +++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index db3e745e5d47..deb5c42c2089 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -44,7 +44,7 @@ enum split_lock_detect_state {
  * split_lock_setup() will switch this to sld_warn on systems that support
  * split lock detect, unless there is a command line override.
  */
-static enum split_lock_detect_state sld_state = sld_off;
+static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
 
 /*
  * Processors which have self-snooping capability can handle conflicting
@@ -984,78 +984,90 @@ static inline bool match_option(const char *arg, int arglen, const char *opt)
 	return len == arglen && !strncmp(arg, opt, len);
 }
 
+static bool split_lock_verify_msr(bool on)
+{
+	u64 ctrl, tmp;
+
+	if (rdmsrl_safe(MSR_TEST_CTRL, &ctrl))
+		return false;
+
+	if (on)
+		ctrl |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
+	else
+		ctrl &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
+
+	if (wrmsrl_safe(MSR_TEST_CTRL, ctrl))
+		return false;
+
+	rdmsrl(MSR_TEST_CTRL, tmp);
+	return ctrl == tmp;
+}
+
 static void __init split_lock_setup(void)
 {
+	enum split_lock_detect_state state = sld_warn;
 	char arg[20];
 	int i, ret;
 
-	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
-	sld_state = sld_warn;
+	if (!split_lock_verify_msr(false)) {
+		pr_info("MSR access failed: Disabled\n");
+		return;
+	}
 
 	ret = cmdline_find_option(boot_command_line, "split_lock_detect",
 				  arg, sizeof(arg));
 	if (ret >= 0) {
 		for (i = 0; i < ARRAY_SIZE(sld_options); i++) {
 			if (match_option(arg, ret, sld_options[i].option)) {
-				sld_state = sld_options[i].state;
+				state = sld_options[i].state;
 				break;
 			}
 		}
 	}
 
-	switch (sld_state) {
+	switch (state) {
 	case sld_off:
 		pr_info("disabled\n");
-		break;
-
+		return;
 	case sld_warn:
 		pr_info("warning about user-space split_locks\n");
 		break;
-
 	case sld_fatal:
 		pr_info("sending SIGBUS on user-space split_locks\n");
 		break;
 	}
+
+	if (!split_lock_verify_msr(true)) {
+		pr_info("MSR access failed: Disabled\n");
+		return;
+	}
+
+	sld_state = state;
+	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
 }
 
 /*
- * Locking is not required at the moment because only bit 29 of this
- * MSR is implemented and locking would not prevent that the operation
- * of one thread is immediately undone by the sibling thread.
- * Use the "safe" versions of rdmsr/wrmsr here because although code
- * checks CPUID and MSR bits to make sure the TEST_CTRL MSR should
- * exist, there may be glitches in virtualization that leave a guest
- * with an incorrect view of real h/w capabilities.
+ * MSR_TEST_CTRL is per core, but we treat it like a per CPU MSR. Locking
+ * is not implemented as one thread could undo the setting of the other
+ * thread immediately after dropping the lock anyway.
  */
-static bool __sld_msr_set(bool on)
+static void sld_update_msr(bool on)
 {
 	u64 test_ctrl_val;
 
-	if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
-		return false;
+	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
 
 	if (on)
 		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
 	else
 		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
 
-	return !wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val);
+	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
 }
 
 static void split_lock_init(void)
 {
-	if (sld_state == sld_off)
-		return;
-
-	if (__sld_msr_set(true))
-		return;
-
-	/*
-	 * If this is anything other than the boot-cpu, you've done
-	 * funny things and you get to keep whatever pieces.
-	 */
-	pr_warn("MSR fail -- disabled\n");
-	sld_state = sld_off;
+	split_lock_verify_msr(sld_state != sld_off);
 }
 
 bool handle_user_split_lock(struct pt_regs *regs, long error_code)
@@ -1071,7 +1083,7 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
 	 * progress and set TIF_SLD so the detection is re-enabled via
 	 * switch_to_sld() when the task is scheduled out.
 	 */
-	__sld_msr_set(false);
+	sld_update_msr(false);
 	set_tsk_thread_flag(current, TIF_SLD);
 	return true;
 }
@@ -1085,7 +1097,7 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
  */
 void switch_to_sld(unsigned long tifn)
 {
-	__sld_msr_set(!(tifn & _TIF_SLD));
+	sld_update_msr(!(tifn & _TIF_SLD));
 }
 
 #define SPLIT_LOCK_CPU(model) {X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY}
-- 
2.20.1

[PATCH v7 2/2] x86/split_lock: Avoid runtime reads of the TEST_CTRL MSR

[Xiaoyao Li]

In a context switch from a task that is detecting split locks
to one that is not (or vice versa) we need to update the TEST_CTRL
MSR. Currently this is done with the common sequence:
	read the MSR
	flip the bit
	write the MSR
in order to avoid changing the value of any reserved bits in the MSR.

Cache unused and reserved bits of TEST_CTRL MSR with SPLIT_LOCK_DETECT
bit cleared during initialization, so we can avoid an expensive RDMSR
instruction during context switch.

Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Originally-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index deb5c42c2089..1f414578899c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -45,6 +45,7 @@ enum split_lock_detect_state {
  * split lock detect, unless there is a command line override.
  */
 static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
+static u64 msr_test_ctrl_cache __ro_after_init;
 
 /*
  * Processors which have self-snooping capability can handle conflicting
@@ -1037,6 +1038,8 @@ static void __init split_lock_setup(void)
 		break;
 	}
 
+	rdmsrl(MSR_TEST_CTRL, msr_test_ctrl_cache);
+
 	if (!split_lock_verify_msr(true)) {
 		pr_info("MSR access failed: Disabled\n");
 		return;
@@ -1053,14 +1056,10 @@ static void __init split_lock_setup(void)
  */
 static void sld_update_msr(bool on)
 {
-	u64 test_ctrl_val;
-
-	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
+	u64 test_ctrl_val = msr_test_ctrl_cache;
 
 	if (on)
 		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
-	else
-		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
 
 	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
 }
-- 
2.20.1

Re: [PATCH v7 1/2] x86/split_lock: Rework the initialization flow of split lock detection

[Sean Christopherson]

On Wed, Mar 25, 2020 at 11:09:23AM +0800, Xiaoyao Li wrote:
>  static void __init split_lock_setup(void)
>  {
> +	enum split_lock_detect_state state = sld_warn;
>  	char arg[20];
>  	int i, ret;
>  
> -	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
> -	sld_state = sld_warn;
> +	if (!split_lock_verify_msr(false)) {
> +		pr_info("MSR access failed: Disabled\n");

A few nits on the error handling.

The error message for this is a bit wonky, lots of colons and it's not
super clear what "Disabled" refers to.

  [    0.000000] x86/split lock detection: MSR access failed: Disabled

Maybe this, so that it reads "split lock detection disabled because the MSR
access failed".

		pr_info("Disabled, MSR access failed\n");

And rather than duplicate the error message, maybe use a goto, e.g.

	if (!split_lock_verify_msr(false))
		goto msr_failed;

	...

	if (!split_lock_verify_msr(true))
		goto msr_failed;


> +		return;
> +	}
>  
>  	ret = cmdline_find_option(boot_command_line, "split_lock_detect",
>  				  arg, sizeof(arg));
>  	if (ret >= 0) {
>  		for (i = 0; i < ARRAY_SIZE(sld_options); i++) {
>  			if (match_option(arg, ret, sld_options[i].option)) {
> -				sld_state = sld_options[i].state;
> +				state = sld_options[i].state;
>  				break;
>  			}
>  		}
>  	}
>  
> -	switch (sld_state) {
> +	switch (state) {
>  	case sld_off:
>  		pr_info("disabled\n");
> -		break;
> -
> +		return;
>  	case sld_warn:
>  		pr_info("warning about user-space split_locks\n");
>  		break;
> -
>  	case sld_fatal:
>  		pr_info("sending SIGBUS on user-space split_locks\n");
>  		break;
>  	}
> +
> +	if (!split_lock_verify_msr(true)) {
> +		pr_info("MSR access failed: Disabled\n");
> +		return;
> +	}
> +
> +	sld_state = state;
> +	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
>  }
>  
>  /*
> - * Locking is not required at the moment because only bit 29 of this
> - * MSR is implemented and locking would not prevent that the operation
> - * of one thread is immediately undone by the sibling thread.
> - * Use the "safe" versions of rdmsr/wrmsr here because although code
> - * checks CPUID and MSR bits to make sure the TEST_CTRL MSR should
> - * exist, there may be glitches in virtualization that leave a guest
> - * with an incorrect view of real h/w capabilities.
> + * MSR_TEST_CTRL is per core, but we treat it like a per CPU MSR. Locking
> + * is not implemented as one thread could undo the setting of the other
> + * thread immediately after dropping the lock anyway.
>   */
> -static bool __sld_msr_set(bool on)
> +static void sld_update_msr(bool on)
>  {
>  	u64 test_ctrl_val;
>  
> -	if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
> -		return false;
> +	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
>  
>  	if (on)
>  		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>  	else
>  		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>  
> -	return !wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val);
> +	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
>  }
>  
>  static void split_lock_init(void)
>  {
> -	if (sld_state == sld_off)
> -		return;
> -
> -	if (__sld_msr_set(true))
> -		return;
> -
> -	/*
> -	 * If this is anything other than the boot-cpu, you've done
> -	 * funny things and you get to keep whatever pieces.
> -	 */
> -	pr_warn("MSR fail -- disabled\n");
> -	sld_state = sld_off;
> +	split_lock_verify_msr(sld_state != sld_off);

I think it'd be worth a WARN_ON() if this fails with sld_state != off.  If
the WRMSR fails, then presumably SLD is off when it's expected to be on.
The implied WARN on the unsafe WRMSR in sld_update_msr() won't fire unless
a task generates an #AC on a non-buggy core and then gets migrated to the
buggy core.  Even if the WARNs are redundant, if something is wrong it'd be
a lot easier for a user to triage/debug if there is a WARN in boot as
opposed to a runtime WARN that requires a misbehaving application and
scheduler behavior.

>  }
>  
>  bool handle_user_split_lock(struct pt_regs *regs, long error_code)
> @@ -1071,7 +1083,7 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>  	 * progress and set TIF_SLD so the detection is re-enabled via
>  	 * switch_to_sld() when the task is scheduled out.
>  	 */
> -	__sld_msr_set(false);
> +	sld_update_msr(false);
>  	set_tsk_thread_flag(current, TIF_SLD);
>  	return true;
>  }
> @@ -1085,7 +1097,7 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>   */
>  void switch_to_sld(unsigned long tifn)
>  {
> -	__sld_msr_set(!(tifn & _TIF_SLD));
> +	sld_update_msr(!(tifn & _TIF_SLD));
>  }
>  
>  #define SPLIT_LOCK_CPU(model) {X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY}
> -- 
> 2.20.1
> 

Re: [PATCH v7 2/2] x86/split_lock: Avoid runtime reads of the TEST_CTRL MSR

[Sean Christopherson]

On Wed, Mar 25, 2020 at 11:09:24AM +0800, Xiaoyao Li wrote:
> In a context switch from a task that is detecting split locks
> to one that is not (or vice versa) we need to update the TEST_CTRL
> MSR. Currently this is done with the common sequence:
> 	read the MSR
> 	flip the bit
> 	write the MSR
> in order to avoid changing the value of any reserved bits in the MSR.
> 
> Cache unused and reserved bits of TEST_CTRL MSR with SPLIT_LOCK_DETECT
> bit cleared during initialization, so we can avoid an expensive RDMSR
> instruction during context switch.
> 
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Originally-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index deb5c42c2089..1f414578899c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -45,6 +45,7 @@ enum split_lock_detect_state {
>   * split lock detect, unless there is a command line override.
>   */
>  static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
> +static u64 msr_test_ctrl_cache __ro_after_init;

What about using "msr_test_ctrl_base_value", or something along those lines?
"cache" doesn't make it clear that SPLIT_LOCK_DETECT is guaranteed to be
zero in this variable.

>  
>  /*
>   * Processors which have self-snooping capability can handle conflicting
> @@ -1037,6 +1038,8 @@ static void __init split_lock_setup(void)
>  		break;
>  	}
>  
> +	rdmsrl(MSR_TEST_CTRL, msr_test_ctrl_cache);

If we're going to bother skipping the RDMSR if state=sld_off on the command
line then it also makes sense to skip it if enabling fails, i.e. move this
below split_lock_verify_msr(true).

> +
>  	if (!split_lock_verify_msr(true)) {
>  		pr_info("MSR access failed: Disabled\n");
>  		return;
> @@ -1053,14 +1056,10 @@ static void __init split_lock_setup(void)
>   */
>  static void sld_update_msr(bool on)
>  {
> -	u64 test_ctrl_val;
> -
> -	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
> +	u64 test_ctrl_val = msr_test_ctrl_cache;
>  
>  	if (on)
>  		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> -	else
> -		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>  
>  	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
>  }
> -- 
> 2.20.1
> 

Re: [PATCH v7 2/2] x86/split_lock: Avoid runtime reads of the TEST_CTRL MSR

[Xiaoyao Li]

On 3/29/2020 12:34 AM, Sean Christopherson wrote:
> On Wed, Mar 25, 2020 at 11:09:24AM +0800, Xiaoyao Li wrote:
>> In a context switch from a task that is detecting split locks
>> to one that is not (or vice versa) we need to update the TEST_CTRL
>> MSR. Currently this is done with the common sequence:
>> 	read the MSR
>> 	flip the bit
>> 	write the MSR
>> in order to avoid changing the value of any reserved bits in the MSR.
>>
>> Cache unused and reserved bits of TEST_CTRL MSR with SPLIT_LOCK_DETECT
>> bit cleared during initialization, so we can avoid an expensive RDMSR
>> instruction during context switch.
>>
>> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Originally-by: Tony Luck <tony.luck@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kernel/cpu/intel.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>> index deb5c42c2089..1f414578899c 100644
>> --- a/arch/x86/kernel/cpu/intel.c
>> +++ b/arch/x86/kernel/cpu/intel.c
>> @@ -45,6 +45,7 @@ enum split_lock_detect_state {
>>    * split lock detect, unless there is a command line override.
>>    */
>>   static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
>> +static u64 msr_test_ctrl_cache __ro_after_init;
> 
> What about using "msr_test_ctrl_base_value", or something along those lines?
> "cache" doesn't make it clear that SPLIT_LOCK_DETECT is guaranteed to be
> zero in this variable.
> 
>>   
>>   /*
>>    * Processors which have self-snooping capability can handle conflicting
>> @@ -1037,6 +1038,8 @@ static void __init split_lock_setup(void)
>>   		break;
>>   	}
>>   
>> +	rdmsrl(MSR_TEST_CTRL, msr_test_ctrl_cache);
> 
> If we're going to bother skipping the RDMSR if state=sld_off on the command
> line then it also makes sense to skip it if enabling fails, i.e. move this
> below split_lock_verify_msr(true).

OK.

Then, the sld bit is 1 for msr_test_ctrl_base_value. Do you think 
"msr_test_ctrl_base_value" still make sense?

or we keep the "else" branch in sld_update_msr() to not rely on the sld 
bit in the base_value?

>> +
>>   	if (!split_lock_verify_msr(true)) {
>>   		pr_info("MSR access failed: Disabled\n");
>>   		return;
>> @@ -1053,14 +1056,10 @@ static void __init split_lock_setup(void)
>>    */
>>   static void sld_update_msr(bool on)
>>   {
>> -	u64 test_ctrl_val;
>> -
>> -	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
>> +	u64 test_ctrl_val = msr_test_ctrl_cache;
>>   
>>   	if (on)
>>   		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>> -	else
>> -		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>>   
>>   	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
>>   }
>> -- 
>> 2.20.1
>>

Re: [PATCH v7 1/2] x86/split_lock: Rework the initialization flow of split lock detection

[Xiaoyao Li]

On 3/29/2020 12:32 AM, Sean Christopherson wrote:
> On Wed, Mar 25, 2020 at 11:09:23AM +0800, Xiaoyao Li wrote:
>>   static void __init split_lock_setup(void)
>>   {
>> +	enum split_lock_detect_state state = sld_warn;
>>   	char arg[20];
>>   	int i, ret;
>>   
>> -	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
>> -	sld_state = sld_warn;
>> +	if (!split_lock_verify_msr(false)) {
>> +		pr_info("MSR access failed: Disabled\n");
> 
> A few nits on the error handling.
> 
> The error message for this is a bit wonky, lots of colons and it's not
> super clear what "Disabled" refers to.
> 
>    [    0.000000] x86/split lock detection: MSR access failed: Disabled
> 
> Maybe this, so that it reads "split lock detection disabled because the MSR
> access failed".
> 
> 		pr_info("Disabled, MSR access failed\n");
> 
> And rather than duplicate the error message, maybe use a goto, e.g.
> 
> 	if (!split_lock_verify_msr(false))
> 		goto msr_failed;
> 
> 	...
> 
> 	if (!split_lock_verify_msr(true))
> 		goto msr_failed;
> 

Will do it in next version.

thanks

>> +		return;
>> +	}
>>   
>>   	ret = cmdline_find_option(boot_command_line, "split_lock_detect",
>>   				  arg, sizeof(arg));
>>   	if (ret >= 0) {
>>   		for (i = 0; i < ARRAY_SIZE(sld_options); i++) {
>>   			if (match_option(arg, ret, sld_options[i].option)) {
>> -				sld_state = sld_options[i].state;
>> +				state = sld_options[i].state;
>>   				break;
>>   			}
>>   		}
>>   	}
>>   
>> -	switch (sld_state) {
>> +	switch (state) {
>>   	case sld_off:
>>   		pr_info("disabled\n");
>> -		break;
>> -
>> +		return;
>>   	case sld_warn:
>>   		pr_info("warning about user-space split_locks\n");
>>   		break;
>> -
>>   	case sld_fatal:
>>   		pr_info("sending SIGBUS on user-space split_locks\n");
>>   		break;
>>   	}
>> +
>> +	if (!split_lock_verify_msr(true)) {
>> +		pr_info("MSR access failed: Disabled\n");
>> +		return;
>> +	}
>> +
>> +	sld_state = state;
>> +	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
>>   }
>>   
>>   /*
>> - * Locking is not required at the moment because only bit 29 of this
>> - * MSR is implemented and locking would not prevent that the operation
>> - * of one thread is immediately undone by the sibling thread.
>> - * Use the "safe" versions of rdmsr/wrmsr here because although code
>> - * checks CPUID and MSR bits to make sure the TEST_CTRL MSR should
>> - * exist, there may be glitches in virtualization that leave a guest
>> - * with an incorrect view of real h/w capabilities.
>> + * MSR_TEST_CTRL is per core, but we treat it like a per CPU MSR. Locking
>> + * is not implemented as one thread could undo the setting of the other
>> + * thread immediately after dropping the lock anyway.
>>    */
>> -static bool __sld_msr_set(bool on)
>> +static void sld_update_msr(bool on)
>>   {
>>   	u64 test_ctrl_val;
>>   
>> -	if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
>> -		return false;
>> +	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
>>   
>>   	if (on)
>>   		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>>   	else
>>   		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
>>   
>> -	return !wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val);
>> +	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
>>   }
>>   
>>   static void split_lock_init(void)
>>   {
>> -	if (sld_state == sld_off)
>> -		return;
>> -
>> -	if (__sld_msr_set(true))
>> -		return;
>> -
>> -	/*
>> -	 * If this is anything other than the boot-cpu, you've done
>> -	 * funny things and you get to keep whatever pieces.
>> -	 */
>> -	pr_warn("MSR fail -- disabled\n");
>> -	sld_state = sld_off;
>> +	split_lock_verify_msr(sld_state != sld_off);
> 
> I think it'd be worth a WARN_ON() if this fails with sld_state != off.  If
> the WRMSR fails, then presumably SLD is off when it's expected to be on.
> The implied WARN on the unsafe WRMSR in sld_update_msr() won't fire unless
> a task generates an #AC on a non-buggy core and then gets migrated to the
> buggy core.  Even if the WARNs are redundant, if something is wrong it'd be
> a lot easier for a user to triage/debug if there is a WARN in boot as
> opposed to a runtime WARN that requires a misbehaving application and
> scheduler behavior.
> 

IIUC, you're recommending something like below?

         WARN_ON(!split_lock_verify_msr(sld_state != sld_off) &&
		sld_state != sld_off);

>>   }
>>   
>>   bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>> @@ -1071,7 +1083,7 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>>   	 * progress and set TIF_SLD so the detection is re-enabled via
>>   	 * switch_to_sld() when the task is scheduled out.
>>   	 */
>> -	__sld_msr_set(false);
>> +	sld_update_msr(false);
>>   	set_tsk_thread_flag(current, TIF_SLD);
>>   	return true;
>>   }
>> @@ -1085,7 +1097,7 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>>    */
>>   void switch_to_sld(unsigned long tifn)
>>   {
>> -	__sld_msr_set(!(tifn & _TIF_SLD));
>> +	sld_update_msr(!(tifn & _TIF_SLD));
>>   }
>>   
>>   #define SPLIT_LOCK_CPU(model) {X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY}
>> -- 
>> 2.20.1
>>

Re: [PATCH v7 1/2] x86/split_lock: Rework the initialization flow of split lock detection

[Sean Christopherson]

On Mon, Mar 30, 2020 at 09:26:25PM +0800, Xiaoyao Li wrote:
> On 3/29/2020 12:32 AM, Sean Christopherson wrote:
> >On Wed, Mar 25, 2020 at 11:09:23AM +0800, Xiaoyao Li wrote:
> >>  static void split_lock_init(void)
> >>  {
> >>-	if (sld_state == sld_off)
> >>-		return;
> >>-
> >>-	if (__sld_msr_set(true))
> >>-		return;
> >>-
> >>-	/*
> >>-	 * If this is anything other than the boot-cpu, you've done
> >>-	 * funny things and you get to keep whatever pieces.
> >>-	 */
> >>-	pr_warn("MSR fail -- disabled\n");
> >>-	sld_state = sld_off;
> >>+	split_lock_verify_msr(sld_state != sld_off);
> >
> >I think it'd be worth a WARN_ON() if this fails with sld_state != off.  If
> >the WRMSR fails, then presumably SLD is off when it's expected to be on.
> >The implied WARN on the unsafe WRMSR in sld_update_msr() won't fire unless
> >a task generates an #AC on a non-buggy core and then gets migrated to the
> >buggy core.  Even if the WARNs are redundant, if something is wrong it'd be
> >a lot easier for a user to triage/debug if there is a WARN in boot as
> >opposed to a runtime WARN that requires a misbehaving application and
> >scheduler behavior.
> >
> 
> IIUC, you're recommending something like below?
> 
>         WARN_ON(!split_lock_verify_msr(sld_state != sld_off) &&
> 		sld_state != sld_off);

Ya.

Re: [PATCH v7 2/2] x86/split_lock: Avoid runtime reads of the TEST_CTRL MSR

[Sean Christopherson]

On Sun, Mar 29, 2020 at 05:13:23PM +0800, Xiaoyao Li wrote:
> On 3/29/2020 12:34 AM, Sean Christopherson wrote:
> >On Wed, Mar 25, 2020 at 11:09:24AM +0800, Xiaoyao Li wrote:
> >>In a context switch from a task that is detecting split locks
> >>to one that is not (or vice versa) we need to update the TEST_CTRL
> >>MSR. Currently this is done with the common sequence:
> >>	read the MSR
> >>	flip the bit
> >>	write the MSR
> >>in order to avoid changing the value of any reserved bits in the MSR.
> >>
> >>Cache unused and reserved bits of TEST_CTRL MSR with SPLIT_LOCK_DETECT
> >>bit cleared during initialization, so we can avoid an expensive RDMSR
> >>instruction during context switch.
> >>
> >>Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >>Originally-by: Tony Luck <tony.luck@intel.com>
> >>Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> >>---
> >>  arch/x86/kernel/cpu/intel.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> >>index deb5c42c2089..1f414578899c 100644
> >>--- a/arch/x86/kernel/cpu/intel.c
> >>+++ b/arch/x86/kernel/cpu/intel.c
> >>@@ -45,6 +45,7 @@ enum split_lock_detect_state {
> >>   * split lock detect, unless there is a command line override.
> >>   */
> >>  static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
> >>+static u64 msr_test_ctrl_cache __ro_after_init;
> >
> >What about using "msr_test_ctrl_base_value", or something along those lines?
> >"cache" doesn't make it clear that SPLIT_LOCK_DETECT is guaranteed to be
> >zero in this variable.
> >
> >>  /*
> >>   * Processors which have self-snooping capability can handle conflicting
> >>@@ -1037,6 +1038,8 @@ static void __init split_lock_setup(void)
> >>  		break;
> >>  	}
> >>+	rdmsrl(MSR_TEST_CTRL, msr_test_ctrl_cache);
> >
> >If we're going to bother skipping the RDMSR if state=sld_off on the command
> >line then it also makes sense to skip it if enabling fails, i.e. move this
> >below split_lock_verify_msr(true).
> 
> OK.
> 
> Then, the sld bit is 1 for msr_test_ctrl_base_value. Do you think
> "msr_test_ctrl_base_value" still make sense?

Ah, I missed that (obviously).  An alternative (to keeping the rdmsr() where
it is) would be to explicitly clear SLD in the base value after the rdmsr().
That'd double as documentation of what is stored in msr_test_ctrl_base_value.

But, the location of rdmsr() is a nit, it can certainly stay where it is if
someone else has a strong preference.

> or we keep the "else" branch in sld_update_msr() to not rely on the sld bit
> in the base_value?

IMO it's better to have SLD=0 in the base value, regardless of how we make
that happen.

> >>+
> >>  	if (!split_lock_verify_msr(true)) {
> >>  		pr_info("MSR access failed: Disabled\n");
> >>  		return;
> >>@@ -1053,14 +1056,10 @@ static void __init split_lock_setup(void)
> >>   */
> >>  static void sld_update_msr(bool on)
> >>  {
> >>-	u64 test_ctrl_val;
> >>-
> >>-	rdmsrl(MSR_TEST_CTRL, test_ctrl_val);
> >>+	u64 test_ctrl_val = msr_test_ctrl_cache;
> >>  	if (on)
> >>  		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> >>-	else
> >>-		test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> >>  	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
> >>  }
> >>-- 
> >>2.20.1
> >>
> 

[PATCH 0/1] x86/split_lock: check split lock feature on initialization

[Benjamin Lamowski]

Hi,

During regression testing of our hypervisor[1] with the current git tip,
we got writes to the TEST_CTRL MSR on hardware that does not support
split lock detection. While the original split_lock implementation does
not exhibit this behavior, the reworked initialization from
dbaba47085b0c unconditionally calls split_lock_verify_msr() from
split_lock_init().

After the elaborate checks in cpu_set_core_cap_bits() this seems like an
oversight. The following simple patch fixes our regression by checking
for X86_FEATURE_SPLIT_LOCK_DETECT before accessing the TEST_CTRL MSR.

Please CC me on replies - I am not subscribed to the LKML.

Kind regards,

Ben

[1] https://github.com/kernkonzept/uvmm

-- 
Benjamin Lamowski - +49.351.41883235
Operating Systems Engineer - https://www.kernkonzept.com

Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
Geschäftsführer: Dr.-Ing. Michael Hohmuth

[PATCH 1/1] x86/split_lock: check split lock feature on initialization

[Benjamin Lamowski]

While the setup code probes for the availability of the TEST_CTRL MSR,
the current initialization code unconditionally probes it even on
systems where this architectural MSR is not available.

This commit changes the code to check for the availability of the split
lock detect feature before initializing it.

Fixes: dbaba47085b0c ("x86/split_lock: Rework the initialization flow of split lock detection")
Signed-off-by: Benjamin Lamowski <benjamin.lamowski@kernkonzept.com>
---
 arch/x86/kernel/cpu/intel.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 9a26e972cdea..70d338ff4807 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -586,7 +586,7 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
 	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
 }
 
-static void split_lock_init(void);
+static void split_lock_init(struct cpuinfo_x86 *c);
 
 static void init_intel(struct cpuinfo_x86 *c)
 {
@@ -703,7 +703,7 @@ static void init_intel(struct cpuinfo_x86 *c)
 	if (tsx_ctrl_state == TSX_CTRL_DISABLE)
 		tsx_disable();
 
-	split_lock_init();
+	split_lock_init(c);
 }
 
 #ifdef CONFIG_X86_32
@@ -1061,9 +1061,10 @@ static void sld_update_msr(bool on)
 	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
 }
 
-static void split_lock_init(void)
+static void split_lock_init(struct cpuinfo_x86 *c)
 {
-	split_lock_verify_msr(sld_state != sld_off);
+	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT))
+		split_lock_verify_msr(sld_state != sld_off);
 }
 
 bool handle_user_split_lock(struct pt_regs *regs, long error_code)
-- 
2.25.1

Re: [PATCH 1/1] x86/split_lock: check split lock feature on initialization

[Sean Christopherson]

On Fri, Apr 03, 2020 at 07:44:03PM +0200, Benjamin Lamowski wrote:
> While the setup code probes for the availability of the TEST_CTRL MSR,
> the current initialization code unconditionally probes it even on
> systems where this architectural MSR is not available.
> 
> This commit changes the code to check for the availability of the split
> lock detect feature before initializing it.
> 
> Fixes: dbaba47085b0c ("x86/split_lock: Rework the initialization flow of split lock detection")
> Signed-off-by: Benjamin Lamowski <benjamin.lamowski@kernkonzept.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 9a26e972cdea..70d338ff4807 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -586,7 +586,7 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
>  	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
>  }
>  
> -static void split_lock_init(void);
> +static void split_lock_init(struct cpuinfo_x86 *c);
>  
>  static void init_intel(struct cpuinfo_x86 *c)
>  {
> @@ -703,7 +703,7 @@ static void init_intel(struct cpuinfo_x86 *c)
>  	if (tsx_ctrl_state == TSX_CTRL_DISABLE)
>  		tsx_disable();
>  
> -	split_lock_init();
> +	split_lock_init(c);
>  }
>  
>  #ifdef CONFIG_X86_32
> @@ -1061,9 +1061,10 @@ static void sld_update_msr(bool on)
>  	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
>  }
>  
> -static void split_lock_init(void)
> +static void split_lock_init(struct cpuinfo_x86 *c)
>  {
> -	split_lock_verify_msr(sld_state != sld_off);
> +	if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT))
> +		split_lock_verify_msr(sld_state != sld_off);

Calling split_lock_verify_msr() with X86_FEATURE_SPLIT_LOCK_DETECT=0 is
intentional, the idea is to ensure SLD is disabled on all CPUs, e.g. in the
unlikely scenario that BIOS enabled SLD.

The first rdmsrl_safe() should short circuit split_lock_verify_msr() if
the RDMSR faults, i.e. it might fault, but it shouldn't WARN.  Are you
seeing issues or was this found via code inspection?

>  }
>  
>  bool handle_user_split_lock(struct pt_regs *regs, long error_code)
> -- 
> 2.25.1
> 

Re: [PATCH 1/1] x86/split_lock: check split lock feature on initialization

[Benjamin Lamowski]

[...]
> Calling split_lock_verify_msr() with X86_FEATURE_SPLIT_LOCK_DETECT=0 is
> intentional, the idea is to ensure SLD is disabled on all CPUs, e.g. in the
> unlikely scenario that BIOS enabled SLD.

I was aware of the intention, but I still don't understand under which
scenario this could be set by the BIOS although the earlier feature
detection has failed, i.e. shouldn't the feature have been detected in
all cases where SLD can actually be disabled? If so, checking for
availability first instead of catching a #GP(0) if it is not implemented
seems to be the cleaner solution.


> The first rdmsrl_safe() should short circuit split_lock_verify_msr() if
> the RDMSR faults, i.e. it might fault, but it shouldn't WARN.  Are you
> seeing issues or was this found via code inspection?

We stumbled across this issue because the x86 version of our VMM is not
yet ready for production and when accessing unimplemented MSRs would
simply return 0 and issue a warning. This is of course a deviation from
rdmsr as defined in the SDM and the pieces are ours to keep, it will be
changed to generate a #GP(0) once the last missing MSRs are emulated
properly.

-- 
Benjamin Lamowski - +49.351.41883235
Operating Systems Engineer - https://www.kernkonzept.com

Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
Geschäftsführer: Dr.-Ing. Michael Hohmuth

Re: [PATCH 1/1] x86/split_lock: check split lock feature on initialization

[Xiaoyao Li]

On 4/6/2020 4:23 PM, Benjamin Lamowski wrote:
> [...]
>> Calling split_lock_verify_msr() with X86_FEATURE_SPLIT_LOCK_DETECT=0 is
>> intentional, the idea is to ensure SLD is disabled on all CPUs, e.g. in the
>> unlikely scenario that BIOS enabled SLD.
> 
> I was aware of the intention, but I still don't understand under which
> scenario this could be set by the BIOS although the earlier feature
> detection has failed, 

It's for the case that SLD is explicitly disabled by kernel params 
"split_lock_detect=off". You know, BIOS may turn SLD on for itself. So 
if user uses "split_lock_detect=off", we have to clear the SLD bit in 
case BIOS forgets to clear it.

> i.e. shouldn't the feature have been detected in
> all cases where SLD can actually be disabled? If so, checking for
> availability first instead of catching a #GP(0) if it is not implemented
> seems to be the cleaner solution.

I understand what you want. i.e., X86_FEATURE_SPLIT_LOCK_DETECT is 
independent from sld_off. I guess you have to make tglx accept it first.

> 
>> The first rdmsrl_safe() should short circuit split_lock_verify_msr() if
>> the RDMSR faults, i.e. it might fault, but it shouldn't WARN.  Are you
>> seeing issues or was this found via code inspection?
> 
> We stumbled across this issue because the x86 version of our VMM is not
> yet ready for production and when accessing unimplemented MSRs would
> simply return 0 and issue a warning. This is of course a deviation from
> rdmsr as defined in the SDM and the pieces are ours to keep, it will be
> changed to generate a #GP(0) once the last missing MSRs are emulated
> properly.
> 

Got it. you are running linux guest in your own VMM and there is warning 
in the VMM.

If you really want to avoid the MSR access on the platform without SLD. 
You could make the default sld_state as sld_unsupported. It can only be 
changed to other value in split_lock_setup() when SLD is enumerated. So 
in split_lock_init(), we can use if (sld_state == sld_unsupported) to 
skip the MSR_TEST_CTRL access.

[PATCH v2 0/1] x86/split_lock: check split lock support on initialization

[Benjamin Lamowski]

> It's for the case that SLD is explicitly disabled by kernel params
> "split_lock_detect=off". You know, BIOS may turn SLD on for itself. So
> if user uses "split_lock_detect=off", we have to clear the SLD bit in
> case BIOS forgets to clear it.

Ah, I forgot that split_lock_setup() returns early if sld is disabled.
Thanks for explaining!

[...]
> If you really want to avoid the MSR access on the platform without SLD.
> You could make the default sld_state as sld_unsupported. It can only be
> changed to other value in split_lock_setup() when SLD is enumerated. So
> in split_lock_init(), we can use if (sld_state == sld_unsupported) to
> skip the MSR_TEST_CTRL access.

Attached is a new version of my patch that implements your suggestion.

Thanks,

Ben

-- 
Benjamin Lamowski - +49.351.41883235
Operating Systems Engineer - https://www.kernkonzept.com

Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
Geschäftsführer: Dr.-Ing. Michael Hohmuth

[PATCH v2 1/1] x86/split_lock: check split lock support on initialization

[Benjamin Lamowski]

While the sld setup code is run only if the TEST_CTRL MSR is available,
the current sld initialization code unconditionally resets it even on
systems where this architectural MSR is not available.

This commit introduces a new default sld state sld_unsupported, which is
changed in split_lock_setup() only if sld is available; and checks for
split lock detect support before initializing it.

Fixes: dbaba47085b0c ("x86/split_lock: Rework the initialization flow of split lock detection")
Signed-off-by: Benjamin Lamowski <benjamin.lamowski@kernkonzept.com>
Suggested-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 9a26e972cdea..e6aff24e7168 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -34,17 +34,18 @@
 #endif
 
 enum split_lock_detect_state {
-	sld_off = 0,
+	sld_unsupported = 0,
+	sld_off,
 	sld_warn,
 	sld_fatal,
 };
 
 /*
- * Default to sld_off because most systems do not support split lock detection
- * split_lock_setup() will switch this to sld_warn on systems that support
- * split lock detect, unless there is a command line override.
+ * Default to sld_unsupported because most systems do not support split lock
+ * detection. split_lock_setup() will switch this to sld_warn on systems that
+ * support split lock detect, unless there is a command line override.
  */
-static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
+static enum split_lock_detect_state sld_state __ro_after_init = sld_unsupported;
 static u64 msr_test_ctrl_cache __ro_after_init;
 
 /*
@@ -1063,7 +1064,8 @@ static void sld_update_msr(bool on)
 
 static void split_lock_init(void)
 {
-	split_lock_verify_msr(sld_state != sld_off);
+	if (sld_state != sld_unsupported)
+		split_lock_verify_msr(sld_state != sld_off);
 }
 
 bool handle_user_split_lock(struct pt_regs *regs, long error_code)
-- 
2.25.1

[PATCH v3 1/1] x86/split_lock: check split lock support on initialization

[Benjamin Lamowski]

While the sld setup code is run only if the TEST_CTRL MSR is available,
the current sld initialization code unconditionally resets it even on
systems where this architectural MSR is not available.

This commit introduces a new default sld state sld_unsupported, which is
changed in split_lock_setup() only if sld is available; and checks for
split lock detect support before initializing it.

Fixes: dbaba47085b0c ("x86/split_lock: Rework the initialization flow of split lock detection")
Signed-off-by: Benjamin Lamowski <benjamin.lamowski@kernkonzept.com>
Suggested-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 9a26e972cdea..de45ba1089c1 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -34,17 +34,18 @@
 #endif
 
 enum split_lock_detect_state {
-	sld_off = 0,
+	sld_unsupported = 0,
+	sld_off,
 	sld_warn,
 	sld_fatal,
 };
 
 /*
- * Default to sld_off because most systems do not support split lock detection
- * split_lock_setup() will switch this to sld_warn on systems that support
- * split lock detect, unless there is a command line override.
+ * Default to sld_unsupported because most systems do not support split lock
+ * detection. split_lock_setup() will switch this to sld_warn on systems that
+ * support split lock detect, unless there is a command line override.
  */
-static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
+static enum split_lock_detect_state sld_state __ro_after_init = sld_unsupported;
 static u64 msr_test_ctrl_cache __ro_after_init;
 
 /*
@@ -1033,6 +1034,9 @@ static void __init split_lock_setup(void)
 	case sld_fatal:
 		pr_info("sending SIGBUS on user-space split_locks\n");
 		break;
+	case sld_unsupported:
+		/* Can't happen, just to keep the compiler happy */
+		break;
 	}
 
 	rdmsrl(MSR_TEST_CTRL, msr_test_ctrl_cache);
@@ -1063,7 +1067,8 @@ static void sld_update_msr(bool on)
 
 static void split_lock_init(void)
 {
-	split_lock_verify_msr(sld_state != sld_off);
+	if (sld_state != sld_unsupported)
+		split_lock_verify_msr(sld_state != sld_off);
 }
 
 bool handle_user_split_lock(struct pt_regs *regs, long error_code)
-- 
2.25.1

Re: [PATCH 0/1] x86/split_lock: check split lock feature on initialization

[Thomas Gleixner]

Benjamin Lamowski <benjamin.lamowski@kernkonzept.com> writes:
> During regression testing of our hypervisor[1] with the current git tip,
> we got writes to the TEST_CTRL MSR on hardware that does not support
> split lock detection. While the original split_lock implementation does
> not exhibit this behavior, the reworked initialization from
> dbaba47085b0c unconditionally calls split_lock_verify_msr() from
> split_lock_init().
>
> After the elaborate checks in cpu_set_core_cap_bits() this seems like an
> oversight. The following simple patch fixes our regression by checking
> for X86_FEATURE_SPLIT_LOCK_DETECT before accessing the TEST_CTRL MSR.

No. It's not an oversight, it's a simplification and it's perfectly
legit. rdsmrl_safe() on a unimplemented MSR results in a #GP which is
caught and fixed up. Nothing to see here.

Thanks,

        tglx

Re: [PATCH v3 1/1] x86/split_lock: check split lock support on initialization

[Thomas Gleixner]

Benjamin Lamowski <benjamin.lamowski@kernkonzept.com> writes:

> While the sld setup code is run only if the TEST_CTRL MSR is available,
> the current sld initialization code unconditionally resets it even on
> systems where this architectural MSR is not available.
>
> This commit introduces a new default sld state sld_unsupported, which is
> changed in split_lock_setup() only if sld is available; and checks for
> split lock detect support before initializing it.

What for? You explain what the patch is doing, but not WHY. See
Documentation/process and while at it please search for 'This patch' as
well.

> Fixes: dbaba47085b0c ("x86/split_lock: Rework the initialization flow
> of split lock detection")

That's blantantly wrong. It fixes your hypervisor which fails to raise a
#GP when an unknown MSR is accessed. 

Are we going to see that kind of 'fixes' everytime we decide to probe an
MSR for simplicity from now on?

Please fix the root cause and not the symptom.

Thanks,

        tglx