[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20190628014326.GA3887@ranerica-svr.sc.intel.com>
Date: Thu, 27 Jun 2019 18:43:26 -0700
From: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...e.de>,
Alan Cox <alan.cox@...el.com>, Tony Luck <tony.luck@...el.com>,
"H. Peter Anvin" <hpa@...or.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Andi Kleen <andi.kleen@...el.com>,
Hans de Goede <hdegoede@...hat.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jordan Borgner <mail@...dan-borgner.de>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
Mohammad Etemadi <mohammad.etemadi@...el.com>,
Ricardo Neri <ricardo.neri@...el.com>,
LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
Andy Shevchenko <andriy.shevchenko@...el.com>,
Andi Kleen <ak@...ux.intel.com>,
Peter Feiner <pfeiner@...gle.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH 1/2] x86/cpu/intel: Clear cache self-snoop capability in
CPUs with known errata
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
Powered by blists - more mailing lists