Re: [PATCH v3 2/8] x86/split_lock: Ensure X86_FEATURE_SPLIT_LOCK_DETECT means the existence of feature

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 3/4/2020 3:41 AM, Sean Christopherson wrote:
> On Tue, Mar 03, 2020 at 10:55:24AM -0800, Sean Christopherson wrote:
>> On Thu, Feb 06, 2020 at 03:04:06PM +0800, Xiaoyao Li wrote:
>>> When flag X86_FEATURE_SPLIT_LOCK_DETECT is set, it should ensure the
>>> existence of MSR_TEST_CTRL and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit.
>>
>> The changelog confused me a bit.  "When flag X86_FEATURE_SPLIT_LOCK_DETECT
>> is set" makes it sound like the logic is being applied after the feature
>> bit is set.  Maybe something like:
>>
>> ```
>> Verify MSR_TEST_CTRL.SPLIT_LOCK_DETECT can be toggled via WRMSR prior to
>> setting the SPLIT_LOCK_DETECT feature bit so that runtime consumers,
>> e.g. KVM, don't need to worry about WRMSR failure.
>> ```
>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>>   arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++----------------
>>>   1 file changed, 23 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>>> index 2b3874a96bd4..49535ed81c22 100644
>>> --- a/arch/x86/kernel/cpu/intel.c
>>> +++ b/arch/x86/kernel/cpu/intel.c
>>> @@ -702,7 +702,8 @@ static void init_intel(struct cpuinfo_x86 *c)
>>>   	if (tsx_ctrl_state == TSX_CTRL_DISABLE)
>>>   		tsx_disable();
>>>   
>>> -	split_lock_init();
>>> +	if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
>>> +		split_lock_init();
>>>   }
>>>   
>>>   #ifdef CONFIG_X86_32
>>> @@ -986,9 +987,26 @@ static inline bool match_option(const char *arg, int arglen, const char *opt)
>>>   
>>>   static void __init split_lock_setup(void)
>>>   {
>>> +	u64 test_ctrl_val;
>>>   	char arg[20];
>>>   	int i, ret;
>>> +	/*
>>> +	 * Use the "safe" versions of rdmsr/wrmsr here to ensure MSR_TEST_CTRL
>>> +	 * and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit do exist. Because there may
>>> +	 * be glitches in virtualization that leave a guest with an incorrect
>>> +	 * view of real h/w capabilities.
>>> +	 */
>>> +	if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
>>> +		return;
>>> +
>>> +	if (wrmsrl_safe(MSR_TEST_CTRL,
>>> +			test_ctrl_val | MSR_TEST_CTRL_SPLIT_LOCK_DETECT))
>>> +		return;
>>> +
>>> +	if (wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val))
>>> +		return;a
>>
>> Probing the MSR should be skipped if SLD is disabled in sld_options, i.e.
>> move this code (and setup_force_cpu_cap() etc...) down below the
>> match_option() logic.  The above would temporarily enable SLD even if the
>> admin has explicitly disabled it, e.g. makes the kernel param useless for
>> turning off the feature due to bugs.
> 
> Hmm, but this prevents KVM from exposing SLD to a guest when it's off in
> the kernel, which would be a useful debug/testing scenario.
> 
> Maybe add another SLD state to forcefully disable SLD?  That way the admin
> can turn of SLD in the host kernel but still allow KVM to expose it to its
> guests.  E.g.

I don't think we need do this.

IMO, this a the bug of split_lock_init(), which assume the initial value 
of MSR_TEST_CTRL is zero, at least bit SPLIT_LOCK of which is zero.
This is problem, it's possible that BIOS has set this bit.

split_lock_setup() here, is to check if the feature really exists. So 
probing MSR_TEST_CTRL and bit MSR_TEST_CTRL_SPLIT_LOCK_DETECT here. If 
there all exist, setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT) to 
indicate feature does exist.
Only when feature exists, there is a need to parse the command line 
config of split_lock_detect.

In split_lock_init(), we should explicitly clear 
MSR_TEST_CTRL_SPLIT_LOCK_DETECT bit if sld_off.

> static const struct {
>          const char                      *option;
>          enum split_lock_detect_state    state;
> } sld_options[] __initconst = {
> 	{ "disable",	sld_disable },
>          { "off",        sld_off     },
>          { "warn",       sld_warn    },
>          { "fatal",      sld_fatal   },
> };
> 
> 
> Then the new setup() becomes:
> 
> static void __init split_lock_setup(void)
> {
>          u64 test_ctrl_val;
>          char arg[20];
>          int i, ret;
> 
>          sld_state = sld_warn;
> 
>          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;
>                                  break;
>                          }
>                  }
>          }
> 
>          if (sld_state == sld_disable)
>                  goto log_sld;
> 
>          /*
>           * Use the "safe" versions of rdmsr/wrmsr here to ensure MSR_TEST_CTRL
>           * and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit do exist. Because there may
>           * be glitches in virtualization that leave a guest with an incorrect
>           * view of real h/w capabilities.
>           */
>          if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val))
>                  goto sld_broken;
> 
>          if (wrmsrl_safe(MSR_TEST_CTRL,
>                          test_ctrl_val | MSR_TEST_CTRL_SPLIT_LOCK_DETECT))
>                  goto sld_broken;
> 
>          if (wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val))
>                  goto sld_broken;
> 
>          setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
> 
> log_sld:
>          switch (sld_state) {
>          case sld_disable:
>                  pr_info("split_lock detection disabled\n");
>                  break;
>          case sld_off:
>                  pr_info("split_lock detection off in kernel\n");
>                  break;
>          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;
>          }
> 
>          return;
> 
> sld_broken:
>          sld_state = sld_disable;
>          pr_err("split_lock detection disabled, MSR access faulted\n");
> }
> 
>> And with that, IMO failing any of RDMSR/WRSMR here warrants a pr_err().
>> The CPU says it supports split lock and the admin hasn't explicitly turned
>> it off, so failure to enable should be logged.
>>
>>> +
>>>   	setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
>>>   	sld_state = sld_warn;
>>>   
>>> @@ -1022,24 +1040,19 @@ static void __init split_lock_setup(void)
>>>    * 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.
>>>    */
>>> -static bool __sld_msr_set(bool on)
>>> +static void __sld_msr_set(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)
>>> @@ -1047,15 +1060,7 @@ 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;
>>> +	__sld_msr_set(true);
>>>   }
>>>   
>>>   bool handle_user_split_lock(unsigned long ip)
>>> -- 
>>> 2.23.0
>>>