Re: [PATCH 1/2] x86/cpu/intel: Clear cache self-snoop capability in CPUs with known errata

[Ricardo Neri <ricardo.neri-calderon@linux.intel.com>]

On Thu, Jun 27, 2019 at 10:38:13PM +0200, Thomas Gleixner wrote:
> Ricardo,
> 
> On Thu, 27 Jun 2019, Ricardo Neri wrote:
> >  
> > +/*
> > + * Processors which have self-snooping capability can handle conflicting
> > + * memory type across CPUs by snooping its own cache. However, there exists
> > + * CPU models in which having conflicting memory types still leads to
> > + * unpredictable behavior, machine check errors, or hangs. Clear this feature
> > + * to prevent its use. For instance, the algorithm to program the Memory Type
> > + * Region Registers and the Page Attribute Table MSR can skip expensive cache
> > + * flushes if self-snooping is supported.
> 
> I appreciate informative comments, but this is the part which disables a
> feature on errata inflicted CPUs. So the whole information about what
> self-snooping helps with is not that interesting here. It's broken, we
> disable it and be done with it.

Sure, Thomas. I will move the the usefulness of self-snooping to the
MTRR programming function as you mention below.
> 
> > + */
> > +static void check_memory_type_self_snoop_errata(struct cpuinfo_x86 *c)
> > +{
> > +	switch (c->x86_model) {
> > +	case INTEL_FAM6_CORE_YONAH:
> > +	case INTEL_FAM6_CORE2_MEROM:
> > +	case INTEL_FAM6_CORE2_MEROM_L:
> > +	case INTEL_FAM6_CORE2_PENRYN:
> > +	case INTEL_FAM6_CORE2_DUNNINGTON:
> > +	case INTEL_FAM6_NEHALEM:
> > +	case INTEL_FAM6_NEHALEM_G:
> > +	case INTEL_FAM6_NEHALEM_EP:
> > +	case INTEL_FAM6_NEHALEM_EX:
> > +	case INTEL_FAM6_WESTMERE:
> > +	case INTEL_FAM6_WESTMERE_EP:
> > +	case INTEL_FAM6_SANDYBRIDGE:
> > +		setup_clear_cpu_cap(X86_FEATURE_SELFSNOOP);
> > +	}
> > +}
> > +
> 
> But looking at the actual interesting part of the 2nd patch:
> 
> > @@ -743,7 +743,9 @@ static void prepare_set(void) __acquires(set_atomicity_lock)
> >        /* Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */
> >        cr0 = read_cr0() | X86_CR0_CD;
> >        write_cr0(cr0);
> > -       wbinvd();
> > +
> > +       if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
> > +               wbinvd();
> 
> This part lacks any form of explanation. So I'd rather have the comment
> about why we can avoid the wbindv() here. I''d surely never would look at
> that errata handling function to get that information.
> 
> Other than that detail, the patches are well done!

Thank you, Thomas!

BR,
Ricardo