From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754005AbbKWJcj (ORCPT ); Mon, 23 Nov 2015 04:32:39 -0500 Received: from mx2.suse.de ([195.135.220.15]:39188 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753518AbbKWJch (ORCPT ); Mon, 23 Nov 2015 04:32:37 -0500 Subject: Re: lapic_suspend/lapic_resume wrong? To: Ingo Molnar References: <5652C472.1010201@suse.com> <20151123080120.GA20696@gmail.com> Cc: Linux Kernel Mailing List , the arch/x86 maintainers , Thomas Gleixner , "H. Peter Anvin" , Borislav Petkov From: Juergen Gross X-Enigmail-Draft-Status: N1110 Message-ID: <5652DD33.1020303@suse.com> Date: Mon, 23 Nov 2015 10:32:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151123080120.GA20696@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23/11/15 09:01, Ingo Molnar wrote: > > * Juergen Gross wrote: > >> Hi, >> >> while trying to find the reason for a hanging kernel during resume >> handling I found a strange inconsistency in arch/x86/kernel/apic/apic.c >> regarding usage of config options. >> >> Attached patch addresses this, no test done as I'm not sure whether >> this is a correct approach. Can you have a look at it, please? >> >> >> Juergen >> >> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c >> index 2f69e3b..bc06c9d 100644 >> --- a/arch/x86/kernel/apic/apic.c >> +++ b/arch/x86/kernel/apic/apic.c >> @@ -2270,6 +2270,7 @@ static struct { >> unsigned int apic_tmict; >> unsigned int apic_tdcr; >> unsigned int apic_thmr; >> + unsigned int apic_cmci; >> } apic_pm_state; >> >> static int lapic_suspend(void) >> @@ -2299,6 +2300,10 @@ static int lapic_suspend(void) >> if (maxlvt >= 5) >> apic_pm_state.apic_thmr = apic_read(APIC_LVTTHMR); >> #endif >> +#ifdef CONFIG_X86_MCE_INTEL >> + if (maxlvt >= 6) >> + apic_pm_state.apic_cmci = apic_read(APIC_LVTCMCI); >> +#endif >> >> local_irq_save(flags); >> disable_local_APIC(); >> @@ -2355,10 +2360,14 @@ static void lapic_resume(void) >> apic_write(APIC_SPIV, apic_pm_state.apic_spiv); >> apic_write(APIC_LVT0, apic_pm_state.apic_lvt0); >> apic_write(APIC_LVT1, apic_pm_state.apic_lvt1); >> -#if defined(CONFIG_X86_MCE_INTEL) >> +#if defined(CONFIG_X86_THERMAL_VECTOR) >> if (maxlvt >= 5) >> apic_write(APIC_LVTTHMR, apic_pm_state.apic_thmr); >> #endif >> +#if defined(CONFIG_X86_MCE_INTEL) >> + if (maxlvt >= 6) >> + apic_write(APIC_LVTCMCI, apic_pm_state.apic_cmci); >> +#endif >> if (maxlvt >= 4) >> apic_write(APIC_LVTPC, apic_pm_state.apic_lvtpc); >> apic_write(APIC_LVTT, apic_pm_state.apic_lvtt); > > the x86 bit looks absolutely sensible to me. Thanks. I'll give it a suspend/resume test and send out a patch. > Have you checked whether we indeed lose this value over S/R, or is this mostly > working fine by accident, due to us executing the CMCI vector initialization via: > > mce_syscore_resume()->__mcheck_cpu_init_vendor()->mce_intel_feature_init()->intel_init_cmci() > > on every resume event? I don't know. I was more concerned what might happen in a kernel configured with CONFIG_X86_MCE_INTEL but not CONFIG_X86_THERMAL_VECTOR. I guess on such a kernel the THMR vector could be set to zero causing some pain (enabled, vector 0?). > The Xen fix is unrelated, just put into the same patch, right? Uuh, yes, sorry. Just a relict of testing another fix. Juergen