From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760451AbbIDRA6 (ORCPT ); Fri, 4 Sep 2015 13:00:58 -0400 Received: from mga14.intel.com ([192.55.52.115]:49723 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760193AbbIDRA4 (ORCPT ); Fri, 4 Sep 2015 13:00:56 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,470,1437462000"; d="scan'208";a="555353358" Date: Fri, 4 Sep 2015 13:59:31 -0400 From: "Raj, Ashok" To: Borislav Petkov Cc: linux-kernel@vger.kernel.org, Boris Petkov , linux-edac@vger.kernel.org, Tony Luck , Serge Ayoun Subject: Re: [Patch V0] x86, mce: Don't clear global error reporting banks during cpu_offline Message-ID: <20150904175931.GA7069@otc-brkl-03.jf.intel.com> References: <1441304224-5747-1-git-send-email-ashok.raj@intel.com> <20150904115029.GA23550@nazgul.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150904115029.GA23550@nazgul.tnic> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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