linux-kernel - Re: [Patch V0] x86, mce: Don't clear global error reporting banks during cpu

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

Message-ID: <20150904175931.GA7069@otc-brkl-03.jf.intel.com>
Date:	Fri, 4 Sep 2015 13:59:31 -0400
From:	"Raj, Ashok" <ashok.raj@...el.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	linux-kernel@...r.kernel.org, Boris Petkov <bp@...e.de>,
	linux-edac@...r.kernel.org, Tony Luck <tony.luck@...el.com>,
	Serge Ayoun <serge.ayoun@...el.com>
Subject: Re: [Patch V0] x86, mce: Don't clear global error reporting banks
 during cpu_offline

Hi Boris

On Fri, Sep 04, 2015 at 01:50:29PM +0200, Borislav Petkov wrote:
> > Intel Secure Guard eXtentions will be disabled when these controls are cleared
> > from a security perspective. This patch enables SGX to work across
> > suspend/resume.
> 
> What does that mean? What does SGX have to do with MCI_CTL registers?
> Explain that in the commit message so that !Intel people can understand.

I will add something more in the commit log and resubmit. Tony had the right 
explanation in his response.. 


> For the whole patch text do:
> 
> s/cpu/CPU/
> 
> s/CPU's/CPUs/ and s/MSR's/MSRs/ if you mean plural. Also spellcheck all text.

Will do.

> > +static void _vendor_disable_error_reporting(void)
> 
> Why the "_" prepended here?

Hummm.. just tried to keep the style as other parts of mcheck_cpu_init()
where we have _mcheck_cpu_init_vendor(). Its not a rule, but sometimes 
local static functions have "__" varient.

Maybe it should have been the double "__"? 

> 
> > +{
> > +	struct cpuinfo_x86 *c = &boot_cpu_data;
> > +
> > +	switch (c->x86_vendor) {
> > +	case X86_VENDOR_INTEL:
> > +		/*
> > +		 * Don't clear on Intel CPU's. Some of these MSR's are
> > +		 * socket wide. Disabling them for just a single cpu offline
> > +		 * is bad, since it will inhibit reporting for all shared
> > +		 * resources.. such as LLC, iMC for e.g.
> > +		 */
> > +		break;
> > +	default:
> > +		/*
> > +		 * Disble MCE reporting for all other CPU Vendor.
> > +		 * Don't want to break functionality on those
> > +		 */
> > +		mce_disable_error_reporting();
> > +	}
> 
> I think the switch-case makes this unnecessarily bloated as code. Just
> do:


Makes sense... i will switch it as suggested.

> 
> 	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> 		return;
> 
> 	mce_disable_error_reporting();
> 
> ...
> 

Cheers,
Ashok

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/