From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932578AbbIDLuT (ORCPT ); Fri, 4 Sep 2015 07:50:19 -0400 Received: from mail.skyhub.de ([78.46.96.112]:34575 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752801AbbIDLuR (ORCPT ); Fri, 4 Sep 2015 07:50:17 -0400 Date: Fri, 4 Sep 2015 13:50:29 +0200 From: Borislav Petkov To: Ashok Raj 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: <20150904115029.GA23550@nazgul.tnic> References: <1441304224-5747-1-git-send-email-ashok.raj@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1441304224-5747-1-git-send-email-ashok.raj@intel.com> 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 On Thu, Sep 03, 2015 at 02:17:04PM -0400, Ashok Raj wrote: > During CPU offline, or during suspend/resume operations, its not safe to > clear MCi_CTL. These MSR's are either thread scoped (meaning private to > thread), or core scoped (private to threads in that core only), or socket > scope i.e visible and controllable from all threads in the socket. > > When we turn off during CPU_OFFLINE, just offlining a single CPU will > stop signaling for all the socket wide resources, such as LLC, iMC for e.g. > > It is true for Intel CPU's. But there seems some history that other processors > may require to turn these off during every CPU offline. > > 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. > - Consolidated some code to use sharing > - Minor changes to some prototypes to fit usage. > - Left handling same for non-Intel CPU models to avoid any unknown regressions. 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. > > Signed-off-by: Ashok Raj > Reviewed-by: Tony Luck > Tested-by: Serge Ayoun > --- > arch/x86/kernel/cpu/mcheck/mce.c | 38 ++++++++++++++++++++++++++++---------- > 1 file changed, 28 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c > index d350858..5498a79 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce.c > +++ b/arch/x86/kernel/cpu/mcheck/mce.c > @@ -2100,7 +2100,7 @@ int __init mcheck_init(void) > * Disable machine checks on suspend and shutdown. We can't really handle > * them later. > */ > -static int mce_disable_error_reporting(void) > +static void mce_disable_error_reporting(void) > { > int i; > > @@ -2110,17 +2110,40 @@ static int mce_disable_error_reporting(void) > if (b->init) > wrmsrl(MSR_IA32_MCx_CTL(i), 0); > } > - return 0; > + return; > +} > + > +static void _vendor_disable_error_reporting(void) Why the "_" prepended here? > +{ > + 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: if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) return; mce_disable_error_reporting(); ... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. --