linux-kernel - Re: [PATCH v4 07/17] x86/split_lock: Enumerate #AC for split lock by MSR IA32_CORE

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]

Date:   Mon, 4 Mar 2019 10:59:22 -0800
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.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>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, kvm@...r.kernel.org
Subject: Re: [PATCH v4 07/17] x86/split_lock: Enumerate #AC for split lock by
 MSR IA32_CORE_CAPABILITY


On Mon, Mar 04, 2019 at 10:58:01AM -0800, Dave Hansen wrote:
> >  	 * Clear/Set all flags overridden by options, after probe.
> > diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> > index 2c0bd38a44ab..5ba11ce99f92 100644
> > --- a/arch/x86/kernel/cpu/cpuid-deps.c
> > +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> > @@ -59,6 +59,7 @@ static const struct cpuid_dep cpuid_deps[] = {
> >  	{ X86_FEATURE_AVX512_4VNNIW,	X86_FEATURE_AVX512F   },
> >  	{ X86_FEATURE_AVX512_4FMAPS,	X86_FEATURE_AVX512F   },
> >  	{ X86_FEATURE_AVX512_VPOPCNTDQ, X86_FEATURE_AVX512F   },
> > +	{ X86_FEATURE_SPLIT_LOCK_DETECT, X86_FEATURE_CORE_CAPABILITY},
> >  	{}
> >  };
> 
> Please reindent the rest of the structure whenever you break the record
> for name length.

Ok. Will do it.

> 
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index fc3c07fe7df5..0c44c49f6005 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -1029,3 +1029,24 @@ static const struct cpu_dev intel_cpu_dev = {
> >  
> >  cpu_dev_register(intel_cpu_dev);
> >  
> > +/**
> > + * init_core_capability - enumerate features supported in IA32_CORE_CAPABILITY
> > + * @c: pointer to cpuinfo_x86
> > + *
> > + * Return: void
> > + */
> > +void init_core_capability(struct cpuinfo_x86 *c)
> > +{
> > +	/*
> > +	 * If MSR_IA32_CORE_CAPABILITY exists, enumerate features that are
> > +	 * reported in the MSR.
> > +	 */
> > +	if (c == &boot_cpu_data && cpu_has(c, X86_FEATURE_CORE_CAPABILITY)) {
> 
> First of all, please endeavor to leave the main flow of the function at
> the first indent.
> 
> *If* you were to do this, it would look like:
> 
> 
> 	if (c != &boot_cpu_data)
> 		return;
> 	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITY))
> 		return;
> 
> 	rdmsrl(...);

Ok. Will do it.

> 
> But, it goes unmentioned why you do the boot-cpu-only restriction here.
>  It doesn't match the behavior of the two functions called before
> init_core_capability():
> 
>         init_scattered_cpuid_features(c);
>         init_speculation_control(c);
> 
> So why is this new function special?

The function sets the split_lock_detect feature which needs to be
applied before BSP calls apply_enforced_caps() in get_cpu_cap(). And I
want to enable split lock detection in earlier phase to detect split
lock earlier.

Thanks.

-Fenghua