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

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] [day] [month] [year] [list]

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