* lapic_suspend/lapic_resume wrong?
@ 2015-11-23 7:46 Juergen Gross
2015-11-23 8:01 ` Ingo Molnar
2015-11-24 4:16 ` kbuild test robot
0 siblings, 2 replies; 6+ messages in thread
From: Juergen Gross @ 2015-11-23 7:46 UTC (permalink / raw)
To: Linux Kernel Mailing List, the arch/x86 maintainers,
Thomas Gleixner, H. Peter Anvin, Ingo Molnar, Borislav Petkov
[-- Attachment #1: Type: text/plain, Size: 321 bytes --]
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
[-- Attachment #2: lapic.patch --]
[-- Type: text/x-patch, Size: 2151 bytes --]
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);
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 849500e..524c221 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -39,6 +39,7 @@
#include <asm/irq.h>
#include <asm/idle.h>
#include <asm/io_apic.h>
+#include <asm/i8259.h>
#include <asm/xen/pci.h>
#endif
#include <asm/sync_bitops.h>
@@ -420,7 +421,7 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi)
return xen_allocate_irq_dynamic();
/* Legacy IRQ descriptors are already allocated by the arch. */
- if (gsi < NR_IRQS_LEGACY)
+ if (gsi < nr_legacy_irqs())
irq = gsi;
else
irq = irq_alloc_desc_at(gsi, -1);
@@ -446,7 +447,7 @@ static void xen_free_irq(unsigned irq)
kfree(info);
/* Legacy IRQ descriptors are managed by the arch. */
- if (irq < NR_IRQS_LEGACY)
+ if (irq < nr_legacy_irqs())
return;
irq_free_desc(irq);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: lapic_suspend/lapic_resume wrong?
2015-11-23 7:46 lapic_suspend/lapic_resume wrong? Juergen Gross
@ 2015-11-23 8:01 ` Ingo Molnar
2015-11-23 9:32 ` Juergen Gross
2015-11-24 4:16 ` kbuild test robot
1 sibling, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2015-11-23 8:01 UTC (permalink / raw)
To: Juergen Gross
Cc: Linux Kernel Mailing List, the arch/x86 maintainers,
Thomas Gleixner, H. Peter Anvin, Borislav Petkov
* Juergen Gross <jgross@suse.com> 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.
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?
The Xen fix is unrelated, just put into the same patch, right?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: lapic_suspend/lapic_resume wrong?
2015-11-23 8:01 ` Ingo Molnar
@ 2015-11-23 9:32 ` Juergen Gross
2015-11-23 9:50 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2015-11-23 9:32 UTC (permalink / raw)
To: Ingo Molnar
Cc: Linux Kernel Mailing List, the arch/x86 maintainers,
Thomas Gleixner, H. Peter Anvin, Borislav Petkov
On 23/11/15 09:01, Ingo Molnar wrote:
>
> * Juergen Gross <jgross@suse.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: lapic_suspend/lapic_resume wrong?
2015-11-23 9:32 ` Juergen Gross
@ 2015-11-23 9:50 ` Borislav Petkov
2015-11-23 9:57 ` Juergen Gross
0 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2015-11-23 9:50 UTC (permalink / raw)
To: Juergen Gross
Cc: Ingo Molnar, Linux Kernel Mailing List, the arch/x86 maintainers,
Thomas Gleixner, H. Peter Anvin
On Mon, Nov 23, 2015 at 10:32:35AM +0100, Juergen Gross wrote:
> 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.
How can you even do that without patching Kconfig?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: lapic_suspend/lapic_resume wrong?
2015-11-23 9:50 ` Borislav Petkov
@ 2015-11-23 9:57 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2015-11-23 9:57 UTC (permalink / raw)
To: Borislav Petkov
Cc: Ingo Molnar, Linux Kernel Mailing List, the arch/x86 maintainers,
Thomas Gleixner, H. Peter Anvin
On 23/11/15 10:50, Borislav Petkov wrote:
> On Mon, Nov 23, 2015 at 10:32:35AM +0100, Juergen Gross wrote:
>> 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.
>
> How can you even do that without patching Kconfig?
I didn't check it. You are right, can't happen (today).
The code is wrong nevertheless.
Juergen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: lapic_suspend/lapic_resume wrong?
2015-11-23 7:46 lapic_suspend/lapic_resume wrong? Juergen Gross
2015-11-23 8:01 ` Ingo Molnar
@ 2015-11-24 4:16 ` kbuild test robot
1 sibling, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2015-11-24 4:16 UTC (permalink / raw)
To: Juergen Gross
Cc: kbuild-all, Linux Kernel Mailing List, the arch/x86 maintainers,
Thomas Gleixner, H. Peter Anvin, Ingo Molnar, Borislav Petkov
[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]
Hi Juergen,
[auto build test ERROR on v4.4-rc2]
[also build test ERROR on next-20151123]
[cannot apply to tip/x86/core]
url: https://github.com/0day-ci/linux/commits/Juergen-Gross/lapic_suspend-lapic_resume-wrong/20151123-155328
config: arm64-allyesconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
drivers/xen/events/events_base.c: In function 'xen_allocate_irq_gsi':
>> drivers/xen/events/events_base.c:424:2: error: implicit declaration of function 'nr_legacy_irqs' [-Werror=implicit-function-declaration]
if (gsi < nr_legacy_irqs())
^
cc1: some warnings being treated as errors
vim +/nr_legacy_irqs +424 drivers/xen/events/events_base.c
418 * space.
419 */
420 if (xen_pv_domain() && !xen_initial_domain())
421 return xen_allocate_irq_dynamic();
422
423 /* Legacy IRQ descriptors are already allocated by the arch. */
> 424 if (gsi < nr_legacy_irqs())
425 irq = gsi;
426 else
427 irq = irq_alloc_desc_at(gsi, -1);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46959 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-24 4:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-23 7:46 lapic_suspend/lapic_resume wrong? Juergen Gross
2015-11-23 8:01 ` Ingo Molnar
2015-11-23 9:32 ` Juergen Gross
2015-11-23 9:50 ` Borislav Petkov
2015-11-23 9:57 ` Juergen Gross
2015-11-24 4:16 ` kbuild test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox