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@xxxxxxxxx>
> > Originally-by: Tony Luck <tony.luck@xxxxxxxxx>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > ---
> >   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