linux-kernel - Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to cache MSR TEST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190418012848.GC18776@romley-ivt3.sc.intel.com>
Date:   Wed, 17 Apr 2019 18:28:49 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        Christopherson Sean J <sean.j.christopherson@...el.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Michael Chan <michael.chan@...adcom.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, kvm@...r.kernel.org,
        netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        Xiaoyao Li <xiaoyao.li@...ux.intel.com>
Subject: Re: [PATCH v7 10/21] x86/split_lock: Define per CPU variable to
 cache MSR TEST_CTL

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@...el.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@...ux.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