| Message ID | 20200206070412.17400-4-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm/split_lock: Add feature split lock detection support in kvm | expand |
On Thu, Feb 06, 2020 at 03:04:07PM +0800, Xiaoyao Li wrote: > Cache the value of MSR_TEST_CTRL in percpu data msr_test_ctrl_cache, > which will be used by KVM module. > > It also avoids an expensive RDMSR instruction if SLD needs to be context > switched. > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/include/asm/cpu.h | 2 ++ > arch/x86/kernel/cpu/intel.c | 19 ++++++++++++------- > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > index ff567afa6ee1..2b20829db450 100644 > --- a/arch/x86/include/asm/cpu.h > +++ b/arch/x86/include/asm/cpu.h > @@ -27,6 +27,8 @@ struct x86_cpu { > }; > > #ifdef CONFIG_HOTPLUG_CPU > +DECLARE_PER_CPU(u64, msr_test_ctrl_cache); > + Why does this depend on HOTPLUG_CPU?
On 2/7/2020 4:23 AM, Arvind Sankar wrote: > On Thu, Feb 06, 2020 at 03:04:07PM +0800, Xiaoyao Li wrote: >> Cache the value of MSR_TEST_CTRL in percpu data msr_test_ctrl_cache, >> which will be used by KVM module. >> >> It also avoids an expensive RDMSR instruction if SLD needs to be context >> switched. >> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> arch/x86/include/asm/cpu.h | 2 ++ >> arch/x86/kernel/cpu/intel.c | 19 ++++++++++++------- >> 2 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h >> index ff567afa6ee1..2b20829db450 100644 >> --- a/arch/x86/include/asm/cpu.h >> +++ b/arch/x86/include/asm/cpu.h >> @@ -27,6 +27,8 @@ struct x86_cpu { >> }; >> >> #ifdef CONFIG_HOTPLUG_CPU >> +DECLARE_PER_CPU(u64, msr_test_ctrl_cache); >> + > > Why does this depend on HOTPLUG_CPU? > Sorry, my bad, It should be under CONFIG_CPU_SUP_INTEL
On Thu, Feb 06, 2020 at 03:04:07PM +0800, Xiaoyao Li wrote: > Cache the value of MSR_TEST_CTRL in percpu data msr_test_ctrl_cache, > which will be used by KVM module. > > It also avoids an expensive RDMSR instruction if SLD needs to be context > switched. > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/include/asm/cpu.h | 2 ++ > arch/x86/kernel/cpu/intel.c | 19 ++++++++++++------- > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > index ff567afa6ee1..2b20829db450 100644 > --- a/arch/x86/include/asm/cpu.h > +++ b/arch/x86/include/asm/cpu.h > @@ -27,6 +27,8 @@ struct x86_cpu { > }; > > #ifdef CONFIG_HOTPLUG_CPU > +DECLARE_PER_CPU(u64, msr_test_ctrl_cache); > + > extern int arch_register_cpu(int num); > extern void arch_unregister_cpu(int); > extern void start_cpu0(void); > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index 49535ed81c22..ff27d026cb4a 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -46,6 +46,9 @@ enum split_lock_detect_state { > */ > static enum split_lock_detect_state sld_state = sld_off; > > +DEFINE_PER_CPU(u64, msr_test_ctrl_cache); > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctrl_cache); > + > /* > * Processors which have self-snooping capability can handle conflicting > * memory type across CPUs by snooping its own cache. However, there exists > @@ -1043,20 +1046,22 @@ static void __init split_lock_setup(void) > */ > static void __sld_msr_set(bool on) > { > - u64 test_ctrl_val; > - > - rdmsrl(MSR_TEST_CTRL, test_ctrl_val); > - > if (on) > - test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT; > + this_cpu_or(msr_test_ctrl_cache, MSR_TEST_CTRL_SPLIT_LOCK_DETECT); > else > - test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT; > + this_cpu_and(msr_test_ctrl_cache, ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT); Updating the cache is at best unnecessary, and at worst dangerous, e.g. it incorrectly implies that the cached value of SPLIT_LOCK_DETECT is reliable. Tony's patch[*] is more what I had in mind, the only question is whether the kernel should be paranoid about other bits in MSR_TEST_CTL. [*] 20200206004944.GA11455@agluck-desk2.amr.corp.intel.com > - wrmsrl(MSR_TEST_CTRL, test_ctrl_val); > + wrmsrl(MSR_TEST_CTRL, this_cpu_read(msr_test_ctrl_cache)); > } > > static void split_lock_init(void) > { > + u64 test_ctrl_val; > + > + /* Cache MSR TEST_CTRL */ > + rdmsrl(MSR_TEST_CTRL, test_ctrl_val); > + this_cpu_write(msr_test_ctrl_cache, test_ctrl_val); > + > if (sld_state == sld_off) > return; > > -- > 2.23.0 >
On 3/4/2020 3:18 AM, Sean Christopherson wrote: > On Thu, Feb 06, 2020 at 03:04:07PM +0800, Xiaoyao Li wrote: >> Cache the value of MSR_TEST_CTRL in percpu data msr_test_ctrl_cache, >> which will be used by KVM module. >> >> It also avoids an expensive RDMSR instruction if SLD needs to be context >> switched. >> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> arch/x86/include/asm/cpu.h | 2 ++ >> arch/x86/kernel/cpu/intel.c | 19 ++++++++++++------- >> 2 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h >> index ff567afa6ee1..2b20829db450 100644 >> --- a/arch/x86/include/asm/cpu.h >> +++ b/arch/x86/include/asm/cpu.h >> @@ -27,6 +27,8 @@ struct x86_cpu { >> }; >> >> #ifdef CONFIG_HOTPLUG_CPU >> +DECLARE_PER_CPU(u64, msr_test_ctrl_cache); >> + >> extern int arch_register_cpu(int num); >> extern void arch_unregister_cpu(int); >> extern void start_cpu0(void); >> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c >> index 49535ed81c22..ff27d026cb4a 100644 >> --- a/arch/x86/kernel/cpu/intel.c >> +++ b/arch/x86/kernel/cpu/intel.c >> @@ -46,6 +46,9 @@ enum split_lock_detect_state { >> */ >> static enum split_lock_detect_state sld_state = sld_off; >> >> +DEFINE_PER_CPU(u64, msr_test_ctrl_cache); >> +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctrl_cache); >> + >> /* >> * Processors which have self-snooping capability can handle conflicting >> * memory type across CPUs by snooping its own cache. However, there exists >> @@ -1043,20 +1046,22 @@ static void __init split_lock_setup(void) >> */ >> static void __sld_msr_set(bool on) >> { >> - u64 test_ctrl_val; >> - >> - rdmsrl(MSR_TEST_CTRL, test_ctrl_val); >> - >> if (on) >> - test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT; >> + this_cpu_or(msr_test_ctrl_cache, MSR_TEST_CTRL_SPLIT_LOCK_DETECT); >> else >> - test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT; >> + this_cpu_and(msr_test_ctrl_cache, ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT); > > Updating the cache is at best unnecessary, and at worst dangerous, e.g. it > incorrectly implies that the cached value of SPLIT_LOCK_DETECT is reliable. > > Tony's patch[*] is more what I had in mind, the only question is whether the > kernel should be paranoid about other bits in MSR_TEST_CTL. OK, I'll use Tony's patch. > [*] 20200206004944.GA11455@agluck-desk2.amr.corp.intel.com > >> - wrmsrl(MSR_TEST_CTRL, test_ctrl_val); >> + wrmsrl(MSR_TEST_CTRL, this_cpu_read(msr_test_ctrl_cache)); >> } >> >> static void split_lock_init(void) >> { >> + u64 test_ctrl_val; >> + >> + /* Cache MSR TEST_CTRL */ >> + rdmsrl(MSR_TEST_CTRL, test_ctrl_val); >> + this_cpu_write(msr_test_ctrl_cache, test_ctrl_val); >> + >> if (sld_state == sld_off) >> return; >> >> -- >> 2.23.0 >>
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index ff567afa6ee1..2b20829db450 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -27,6 +27,8 @@ struct x86_cpu { }; #ifdef CONFIG_HOTPLUG_CPU +DECLARE_PER_CPU(u64, msr_test_ctrl_cache); + extern int arch_register_cpu(int num); extern void arch_unregister_cpu(int); extern void start_cpu0(void); diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 49535ed81c22..ff27d026cb4a 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -46,6 +46,9 @@ enum split_lock_detect_state { */ static enum split_lock_detect_state sld_state = sld_off; +DEFINE_PER_CPU(u64, msr_test_ctrl_cache); +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctrl_cache); + /* * Processors which have self-snooping capability can handle conflicting * memory type across CPUs by snooping its own cache. However, there exists @@ -1043,20 +1046,22 @@ static void __init split_lock_setup(void) */ static void __sld_msr_set(bool on) { - u64 test_ctrl_val; - - rdmsrl(MSR_TEST_CTRL, test_ctrl_val); - if (on) - test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT; + this_cpu_or(msr_test_ctrl_cache, MSR_TEST_CTRL_SPLIT_LOCK_DETECT); else - test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT; + this_cpu_and(msr_test_ctrl_cache, ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT); - wrmsrl(MSR_TEST_CTRL, test_ctrl_val); + wrmsrl(MSR_TEST_CTRL, this_cpu_read(msr_test_ctrl_cache)); } static void split_lock_init(void) { + u64 test_ctrl_val; + + /* Cache MSR TEST_CTRL */ + rdmsrl(MSR_TEST_CTRL, test_ctrl_val); + this_cpu_write(msr_test_ctrl_cache, test_ctrl_val); + if (sld_state == sld_off) return;
Cache the value of MSR_TEST_CTRL in percpu data msr_test_ctrl_cache, which will be used by KVM module. It also avoids an expensive RDMSR instruction if SLD needs to be context switched. Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/include/asm/cpu.h | 2 ++ arch/x86/kernel/cpu/intel.c | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 7 deletions(-)