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
