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

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

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