Re: [PATCH v4 07/17] x86/split_lock: Enumerate #AC for split lock by MSR IA32_CORE_CAPABILITY

[Fenghua Yu]

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