[PATCH 0/2] apic: Fix error interrupt report at all APs

[PATCH 0/2] apic: Fix error interrupt report at all APs

[Youquan Song]

Recently, customer report that once machine boot, there are many error interrupt
reported with exact number of all APs. 

The root cause is Local APIC will generate error interrupt when it detect
the illegal vector (one in 0 ~ 15) in an interrupt message received or
interrupt generate from local vector table or self IPI. SDM3A.chapter 10.

The thermal sensor register will be reset to 0x10000, current thermal throttling
driver will first restore AP with the thermal sensor register value of geting
from BSP,but BSP thermal sensor register is also set 0x10000.  value 0x10000
means the interrupt vector is zero. After writing 0x10000 to thermal sensor LVT,
the processor will recieve the error interrupt report if the APIC error
interrupt is also set.

Restore thermal sensor value of BSP is useless because it will soon be set to
correct value included legal vector information.  This patch remove the restore
process. So the agony noise of error interrupt will dismiss when machine boot.  

In patchset also add print out error interrupt detail debug information. 

[PATCH 1/2] apic: Fix error interrupt report at all APs

[Youquan Song]

Recently, customer report that once machine boot, there are many error interrupt
reported with exact number of all APs. 

The root cause is Local APIC will generate error interrupt when it detect
the illegal vector (one in 0 ~ 15) in an interrupt message received or
interrupt generate from local vector table or self IPI. SDM3A.chapter 10.

The thermal sensor register will be reset to 0x10000, current thermal throttling
driver will first restore AP with the thermal sensor register value of geting
from BSP,but BSP thermal sensor register is also set 0x10000.  value 0x10000
means the interrupt vector is zero. After writing 0x10000 to thermal sensor LVT,
the processor will recieve the error interrupt report if the APIC error
interrupt is also set.

Restore thermal sensor value of BSP is useless because it will soon be set to
correct value included legal vector information.  This patch remove the restore
process. So the agony noise of error interrupt will be dismiss when machine boot   

Signed-off-by: Youquan Song <youquan.song@intel.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..1658483 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -405,17 +405,6 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 	 */
 	rdmsr(MSR_IA32_MISC_ENABLE, l, h);
 
-	/*
-	 * The initial value of thermal LVT entries on all APs always reads
-	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
-	 * sequence to them and LVT registers are reset to 0s except for
-	 * the mask bits which are set to 1s when APs receive INIT IPI.
-	 * Always restore the value that BIOS has programmed on AP based on
-	 * BSP's info we saved since BIOS is always setting the same value
-	 * for all threads/cores
-	 */
-	apic_write(APIC_LVTTHMR, lvtthmr_init);
-
 	h = lvtthmr_init;
 
 	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
-- 
1.6.4.2

[PATCH 2/2] apic: Add print error interrupt reason

[Youquan Song]

End user worry about the error interrupt information and intend to know what
kind of error interrupts are generated, so this patch add printing out the
detail debug information of error interrupt. 

Signed-off-by: Youquan Song <youquan.song@intel.com>
---
 arch/x86/kernel/apic/apic.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 3f838d5..f5538d5 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1794,6 +1794,31 @@ void smp_spurious_interrupt(struct pt_regs *regs)
 	irq_exit();
 }
 
+static const char *error_interrupt_reason[] = {
+	"Send CS error",
+	"Receive CS error",
+	"Send accept error",
+	"Receive accept error",
+	"Redirectable IPI",
+	"Send illegal vector",
+	"Received illegal vector",
+	"Illegal register address",
+};
+
+static void print_error_interrupt_reason(u32 reason)
+{
+	u32 i = 0;
+	reason = reason & 0xff;
+	do {
+		if (reason & 0x1)
+			pr_debug(" : %s", error_interrupt_reason[i]);
+		i++;
+		reason >>= 1;
+	} while (reason > 0);
+
+	pr_debug("\n");
+}
+
 /*
  * This interrupt should never happen with our APIC/SMP architecture
  */
@@ -1821,8 +1846,9 @@ void smp_error_interrupt(struct pt_regs *regs)
 	 * 6: Received illegal vector
 	 * 7: Illegal register address
 	 */
-	pr_debug("APIC error on CPU%d: %02x(%02x)\n",
+	pr_debug("APIC error on CPU%d: %02x(%02x)",
 		smp_processor_id(), v , v1);
+	print_error_interrupt_reason(v1);
 	irq_exit();
 }
 
-- 
1.6.4.2

Re: [PATCH 2/2] apic: Add print error interrupt reason

[Joe Perches]

On Thu, 2011-01-06 at 11:28 +0800, Youquan Song wrote:
> End user worry about the error interrupt information and intend to know what
> kind of error interrupts are generated, so this patch add printing out the
> detail debug information of error interrupt. 
> 
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
>  arch/x86/kernel/apic/apic.c |   28 +++++++++++++++++++++++++++-
>  1 files changed, 27 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
[]
> +static const char *error_interrupt_reason[] = {

static const char * const etc[]

> +	"Send CS error",
> +	"Receive CS error",
> +	"Send accept error",
> +	"Receive accept error",
> +	"Redirectable IPI",
> +	"Send illegal vector",
> +	"Received illegal vector",
> +	"Illegal register address",
> +};
> +
> +static void print_error_interrupt_reason(u32 reason)
> +{
> +	u32 i = 0;
> +	reason = reason & 0xff;
> +	do {
> +		if (reason & 0x1)
> +			pr_debug(" : %s", error_interrupt_reason[i]);

This isn't correct as it will emit <7> for each reason.

You want pr_cont surrounded by #ifdef DEBUG
or some new #define pr_debug_cont in printk.h
but that won't play well with dynamic_debug.

Jason?  Got a cure for this?

Re: [PATCH 2/2] apic: Add print error interrupt reason

[Jason Baron]

On Wed, Jan 05, 2011 at 08:34:51PM -0800, Joe Perches wrote:
> On Thu, 2011-01-06 at 11:28 +0800, Youquan Song wrote:
> > End user worry about the error interrupt information and intend to know what
> > kind of error interrupts are generated, so this patch add printing out the
> > detail debug information of error interrupt. 
> > 
> > Signed-off-by: Youquan Song <youquan.song@intel.com>
> > ---
> >  arch/x86/kernel/apic/apic.c |   28 +++++++++++++++++++++++++++-
> >  1 files changed, 27 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> []
> > +static const char *error_interrupt_reason[] = {
> 
> static const char * const etc[]
> 
> > +	"Send CS error",
> > +	"Receive CS error",
> > +	"Send accept error",
> > +	"Receive accept error",
> > +	"Redirectable IPI",
> > +	"Send illegal vector",
> > +	"Received illegal vector",
> > +	"Illegal register address",
> > +};
> > +
> > +static void print_error_interrupt_reason(u32 reason)
> > +{
> > +	u32 i = 0;
> > +	reason = reason & 0xff;
> > +	do {
> > +		if (reason & 0x1)
> > +			pr_debug(" : %s", error_interrupt_reason[i]);
> 
> This isn't correct as it will emit <7> for each reason.
> 
> You want pr_cont surrounded by #ifdef DEBUG
> or some new #define pr_debug_cont in printk.h
> but that won't play well with dynamic_debug.
> 
> Jason?  Got a cure for this?
> 
> 

hmmm...right, we could introduce a pr_debug_cont(), but I think it would
be complex to implement, since we would have to pass a reference to
where we are continued from...the #ifdef DEBUG isn't great either, since
we need this for every pr_debug() statement which might be followed by a
pr_cont()...

perhaps, the simplest thing is just to code this explicitly as:
printk(KERN_DEBUG, blah), followed by the regular pr_cont()?

thanks,

-Jason

Re: [PATCH 1/2] apic: Fix error interrupt report at all APs

[Yong Wang]

On Thu, Jan 06, 2011 at 11:28:51AM +0800, Youquan Song wrote:
> Recently, customer report that once machine boot, there are many error interrupt
> reported with exact number of all APs. 
> 
> The root cause is Local APIC will generate error interrupt when it detect
> the illegal vector (one in 0 ~ 15) in an interrupt message received or
> interrupt generate from local vector table or self IPI. SDM3A.chapter 10.
> 
> The thermal sensor register will be reset to 0x10000, current thermal throttling
> driver will first restore AP with the thermal sensor register value of geting
> from BSP,but BSP thermal sensor register is also set 0x10000.  value 0x10000
> means the interrupt vector is zero. After writing 0x10000 to thermal sensor LVT,
> the processor will recieve the error interrupt report if the APIC error
> interrupt is also set.
> 

If the thermal interrupt vector of all CPUs is 0 and you are seeing
LAPIC error interrupts, the correct way to fix it is to disable digital
thermal sensors from generating interrupts.

> Restore thermal sensor value of BSP is useless because it will soon be set to
> correct value included legal vector information.  This patch remove the restore
> process. So the agony noise of error interrupt will be dismiss when machine boot   
> 
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |   11 -----------
>  1 files changed, 0 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index 4b68326..1658483 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -405,17 +405,6 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
>  	 */
>  	rdmsr(MSR_IA32_MISC_ENABLE, l, h);
>  
> -	/*
> -	 * The initial value of thermal LVT entries on all APs always reads
> -	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
> -	 * sequence to them and LVT registers are reset to 0s except for
> -	 * the mask bits which are set to 1s when APs receive INIT IPI.
> -	 * Always restore the value that BIOS has programmed on AP based on
> -	 * BSP's info we saved since BIOS is always setting the same value
> -	 * for all threads/cores
> -	 */
> -	apic_write(APIC_LVTTHMR, lvtthmr_init);
> -

NACK. Please take a look at the commit msg adding the above code you are
trying to delete. If the code is deleted, the bug that
a2202aa29289db64ca7988b12343158b67b27f10 solved will pop up again.

Btw, please try your best to find and copy the person whose code you
wanna delete in your patch.

-Yong

Re: [PATCH 1/2] apic: Fix error interrupt report at all APs

[Youquan Song]

> If the thermal interrupt vector of all CPUs is 0 and you are seeing
> LAPIC error interrupts, the correct way to fix it is to disable digital
> thermal sensors from generating interrupts.
Thanks for your feedback Yong!

No. Write LVT with vector(0~15) will always generate error interrupt if error
interrupt is set. it has nothing with digital thermal device.
Since you find some platform will cause issue because BIOS possible take
over the thermal interrupt.

So my prefered patch is like following, move the APIC_LVTTHMR restore to
vecotor check domain, which following your origin logic.

Can I consider BIOS under control thermal interrupt will not be illegal vector?


diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 4b68326..a2cb9bd 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -405,30 +405,28 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 	 */
 	rdmsr(MSR_IA32_MISC_ENABLE, l, h);
 
-	/*
-	 * The initial value of thermal LVT entries on all APs always reads
-	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
-	 * sequence to them and LVT registers are reset to 0s except for
-	 * the mask bits which are set to 1s when APs receive INIT IPI.
-	 * Always restore the value that BIOS has programmed on AP based on
-	 * BSP's info we saved since BIOS is always setting the same value
-	 * for all threads/cores
-	 */
-	apic_write(APIC_LVTTHMR, lvtthmr_init);
-
 	h = lvtthmr_init;
-
-	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
-		printk(KERN_DEBUG
-		       "CPU%d: Thermal monitoring handled by SMI\n", cpu);
-		return;
-	}
-
 	/* Check whether a vector already exists */
 	if (h & APIC_VECTOR_MASK) {
 		printk(KERN_DEBUG
 		       "CPU%d: Thermal LVT vector (%#x) already installed\n",
 		       cpu, (h & APIC_VECTOR_MASK));
+		/*
+	 	 * The initial value of thermal LVT entries on all APs always reads
+	 	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
+	 	 * sequence to them and LVT registers are reset to 0s except for
+	 	 * the mask bits which are set to 1s when APs receive INIT IPI.
+	 	 * Always restore the value that BIOS has programmed on AP based on
+	 	 * BSP's info we saved since BIOS is always setting the same value
+	 	 * for all threads/cores
+	 	*/
+		apic_write(APIC_LVTTHMR, h);
+		return;
+	}
+
+	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
+		printk(KERN_DEBUG
+		       "CPU%d: Thermal monitoring handled by SMI\n", cpu);
 		return;
 	}
 

Re: [PATCH 1/2] apic: Fix error interrupt report at all APs

[Suresh Siddha]

On Thu, 2011-01-06 at 18:04 -0800, Youquan Song wrote:
> > If the thermal interrupt vector of all CPUs is 0 and you are seeing
> > LAPIC error interrupts, the correct way to fix it is to disable digital
> > thermal sensors from generating interrupts.
> Thanks for your feedback Yong!
> 
> No. Write LVT with vector(0~15) will always generate error interrupt if error
> interrupt is set. it has nothing with digital thermal device.
> Since you find some platform will cause issue because BIOS possible take
> over the thermal interrupt.
> 
> So my prefered patch is like following, move the APIC_LVTTHMR restore to
> vecotor check domain, which following your origin logic.
> 
> Can I consider BIOS under control thermal interrupt will not be illegal vector?

In that case, bios will specify the delivery mode as SMI and the vector
field will be (most likely) set to zero. So what you need to do is,
check if the delivery mode is !fixed and then restore the LVTTHMR to
that value.

> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index 4b68326..a2cb9bd 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -405,30 +405,28 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
>  	 */
>  	rdmsr(MSR_IA32_MISC_ENABLE, l, h);
>  
> -	/*
> -	 * The initial value of thermal LVT entries on all APs always reads
> -	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
> -	 * sequence to them and LVT registers are reset to 0s except for
> -	 * the mask bits which are set to 1s when APs receive INIT IPI.
> -	 * Always restore the value that BIOS has programmed on AP based on
> -	 * BSP's info we saved since BIOS is always setting the same value
> -	 * for all threads/cores
> -	 */
> -	apic_write(APIC_LVTTHMR, lvtthmr_init);
> -
>  	h = lvtthmr_init;
> -
> -	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
> -		printk(KERN_DEBUG
> -		       "CPU%d: Thermal monitoring handled by SMI\n", cpu);
> -		return;
> -	}
> -
>  	/* Check whether a vector already exists */
>  	if (h & APIC_VECTOR_MASK) {
>  		printk(KERN_DEBUG
>  		       "CPU%d: Thermal LVT vector (%#x) already installed\n",
>  		       cpu, (h & APIC_VECTOR_MASK));
> +		/*
> +	 	 * The initial value of thermal LVT entries on all APs always reads
> +	 	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
> +	 	 * sequence to them and LVT registers are reset to 0s except for
> +	 	 * the mask bits which are set to 1s when APs receive INIT IPI.
> +	 	 * Always restore the value that BIOS has programmed on AP based on
> +	 	 * BSP's info we saved since BIOS is always setting the same value
> +	 	 * for all threads/cores
> +	 	*/
> +		apic_write(APIC_LVTTHMR, h);
> +		return;
> +	}
> +
> +	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
> +		printk(KERN_DEBUG
> +		       "CPU%d: Thermal monitoring handled by SMI\n", cpu);
>  		return;
>  	}


So it should be more like?

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index e12246f..f89a649 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -445,6 +445,7 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 	 */
 	rdmsr(MSR_IA32_MISC_ENABLE, l, h);
 
+	h = lvtthmr_init;
 	/*
 	 * The initial value of thermal LVT entries on all APs always reads
 	 * 0x10000 because APs are woken up by BSP issuing INIT-SIPI-SIPI
@@ -454,9 +455,8 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 	 * BSP's info we saved since BIOS is always setting the same value
 	 * for all threads/cores
 	 */
-	apic_write(APIC_LVTTHMR, lvtthmr_init);
-
-	h = lvtthmr_init;
+	if (!(h & APIC_DM_FIXED))
+		apic_write(APIC_LVTTHMR, h);
 
 	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
 		printk(KERN_DEBUG

Can you check if this works and send an updated patch with the correct description etc.

thanks,
suresh