lapic_suspend/lapic_resume wrong?

lapic_suspend/lapic_resume wrong?

[Juergen Gross]

[-- 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);

Re: lapic_suspend/lapic_resume wrong?

[Ingo Molnar]

* 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

Re: lapic_suspend/lapic_resume wrong?

[Juergen Gross]

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

Re: lapic_suspend/lapic_resume wrong?

[Borislav Petkov]

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)
-- 

Re: lapic_suspend/lapic_resume wrong?

[Juergen Gross]

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

Re: lapic_suspend/lapic_resume wrong?

[kbuild test robot]

[-- 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 --]