lkml.org 
[lkml]   [2020]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 2/2] x86/split_lock: Avoid runtime reads of the TEST_CTRL MSR
From
Date
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
>>

\
 
 \ /
  Last update: 2020-03-29 11:14    [W:0.352 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site