Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to cache MSR TEST_CTL

[Fenghua Yu]

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@xxxxxxxxx>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxxxxxxxx>
> 
> 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