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 2:55 AM, 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.
> 
> 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.

It is not about to enable split lock detection here, but to parse the 
kernel booting parameter "split_lock_detect".

If probing MSR or MSR bit fails, it indicates the CPU doesn't has 
feature X86_FEATURE_SPLIT_LOCK_DETECT. So don't set feature flag and 
there is no need to parse "split_lock_detect", just return.

Then, as the change at the beginning of this patch, we should call 
split_lock_init() based on X86_FEATURE_SPLIT_LOCK_DETECT bit.

>> +
>>   	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
>>