| Message ID | 1555536851-17462-11-git-send-email-fenghua.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/split_lock: Enable split lock detection | expand |
On Wed, 17 Apr 2019, Fenghua Yu wrote: > MSR TEST_CTL (0x33) value is cached in per CPU variable msr_test_ctl_cache. It _is_ cached? How so? > The cached value will be used in virutalization to avoid costly MSR read. > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> That SOB chain is bogus. > --- > arch/x86/include/asm/cpu.h | 1 + > arch/x86/kernel/cpu/intel.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > index 4e03f53fc079..cd7493f20234 100644 > --- a/arch/x86/include/asm/cpu.h > +++ b/arch/x86/include/asm/cpu.h > @@ -40,6 +40,7 @@ int mwait_usable(const struct cpuinfo_x86 *); > unsigned int x86_family(unsigned int sig); > unsigned int x86_model(unsigned int sig); > unsigned int x86_stepping(unsigned int sig); > +DECLARE_PER_CPU(u64, msr_test_ctl_cache); > #ifdef CONFIG_CPU_SUP_INTEL > void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c); > #else > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index 62f61a961eb6..997d683d3c27 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -31,6 +31,9 @@ > #include <asm/apic.h> > #endif > > +DEFINE_PER_CPU(u64, msr_test_ctl_cache); > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache); Contrary to things like cpufeatures or MSR bits, it's pretty useless to have a separate patch for this. Please fold this into the place which actualy uses it. Thanks, tglx
On Thu, Apr 18, 2019 at 12:14:12AM +0200, Thomas Gleixner wrote: > On Wed, 17 Apr 2019, Fenghua Yu wrote: > > > MSR TEST_CTL (0x33) value is cached in per CPU variable msr_test_ctl_cache. > > It _is_ cached? How so? > > > The cached value will be used in virutalization to avoid costly MSR read. > > > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > That SOB chain is bogus. > > > --- > > arch/x86/include/asm/cpu.h | 1 + > > arch/x86/kernel/cpu/intel.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > > index 4e03f53fc079..cd7493f20234 100644 > > --- a/arch/x86/include/asm/cpu.h > > +++ b/arch/x86/include/asm/cpu.h > > @@ -40,6 +40,7 @@ int mwait_usable(const struct cpuinfo_x86 *); > > unsigned int x86_family(unsigned int sig); > > unsigned int x86_model(unsigned int sig); > > unsigned int x86_stepping(unsigned int sig); > > +DECLARE_PER_CPU(u64, msr_test_ctl_cache); > > #ifdef CONFIG_CPU_SUP_INTEL > > void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c); > > #else > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > > index 62f61a961eb6..997d683d3c27 100644 > > --- a/arch/x86/kernel/cpu/intel.c > > +++ b/arch/x86/kernel/cpu/intel.c > > @@ -31,6 +31,9 @@ > > #include <asm/apic.h> > > #endif > > > > +DEFINE_PER_CPU(u64, msr_test_ctl_cache); > > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache); > > Contrary to things like cpufeatures or MSR bits, it's pretty useless to > have a separate patch for this. Please fold this into the place which > actualy uses it. Can I fold this patch into the KVM patch 0013 which first uses (reads) the variable? But the variable will be set in later patches when enabling split lock feature (patch 0014) and when enabling/disabling split lock feature (patch 0015). Is this a right sequence to fit the variable in the patch set? Thanks. -Fenghua
On Wed, 17 Apr 2019, Fenghua Yu wrote: > On Thu, Apr 18, 2019 at 12:14:12AM +0200, Thomas Gleixner wrote: > > On Wed, 17 Apr 2019, Fenghua Yu wrote: > > > +DEFINE_PER_CPU(u64, msr_test_ctl_cache); > > > +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache); > > > > Contrary to things like cpufeatures or MSR bits, it's pretty useless to > > have a separate patch for this. Please fold this into the place which > > actualy uses it. > > Can I fold this patch into the KVM patch 0013 which first uses (reads) the > variable? But the variable will be set in later patches when enabling split > lock feature (patch 0014) and when enabling/disabling split lock feature > (patch 0015). > > Is this a right sequence to fit the variable in the patch set? As I said in the other reply, you are assuming that the content of that MSR is 0. Which might be true now, but is that true in a year from now? So you really want to at least initialize the variable by reading the MSR _before_ you make use of it in KVM. Thanks, tglx
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index 4e03f53fc079..cd7493f20234 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -40,6 +40,7 @@ int mwait_usable(const struct cpuinfo_x86 *); unsigned int x86_family(unsigned int sig); unsigned int x86_model(unsigned int sig); unsigned int x86_stepping(unsigned int sig); +DECLARE_PER_CPU(u64, msr_test_ctl_cache); #ifdef CONFIG_CPU_SUP_INTEL void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c); #else diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 62f61a961eb6..997d683d3c27 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -31,6 +31,9 @@ #include <asm/apic.h> #endif +DEFINE_PER_CPU(u64, msr_test_ctl_cache); +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache); + /* * Just in case our CPU detection goes bad, or you have a weird system, * allow a way to override the automatic disabling of MPX.