Re: [GIT PULL] x86/apic changes for v3.14

[Maciej W. Rozycki]

On Mon, 20 Jan 2014, Ingo Molnar wrote:

> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index d278736..7f26c9a 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1982,21 +1989,20 @@ static inline void __smp_error_interrupt(struct pt_regs *regs)
>  	};
>  
>  	/* First tickle the hardware, only then report what went on. -- REW */
> -	v0 = apic_read(APIC_ESR);
>  	apic_write(APIC_ESR, 0);
> -	v1 = apic_read(APIC_ESR);
> +	v = apic_read(APIC_ESR);
>  	ack_APIC_irq();
>  	atomic_inc(&irq_err_count);
>  
> -	apic_printk(APIC_DEBUG, KERN_DEBUG "APIC error on CPU%d: %02x(%02x)",
> -		    smp_processor_id(), v0 , v1);
> +	apic_printk(APIC_DEBUG, KERN_DEBUG "APIC error on CPU%d: %02x",
> +		    smp_processor_id(), v);
>  
> -	v1 = v1 & 0xff;
> -	while (v1) {
> -		if (v1 & 0x1)
> +	v &= 0xff;
> +	while (v) {
> +		if (v & 0x1)
>  			apic_printk(APIC_DEBUG, KERN_CONT " : %s", error_interrupt_reason[i]);
>  		i++;
> -		v1 >>= 1;
> +		v >>= 1;
>  	}
>  
>  	apic_printk(APIC_DEBUG, KERN_CONT "\n");

 Sorry to come up with this so late, I've been cleaning up my mailbox from 
old mailing traffic and came across your change only now.

 I'm afraid this interferes with an old Pentium APIC erratum:

"3AP. Writes to Error Register Clears Register

PROBLEM: The APIC Error register is intended to only be read.  If there is 
a write to this register the data in the APIC Error register will be 
cleared and lost.

IMPLICATION: There is a possibility of clearing the Error register status 
since the write to the register is not specifically blocked.

WORKAROUND: Writes should not occur to the Pentium processor APIC Error 
register.

STATUS: For the steppings affected see the Summary Table of Changes at the 
beginning of this section."

The steppings affected are actually: B1, B3 and B5.  Do we want to keep 
supporting them?  I think yes, we already handle the erratum elsewhere 
(lapic_setup_esr).  So how about:

	if (lapic_get_maxlvt() > 3)	/* Due to the Pentium erratum 3AP. */
		apic_write(APIC_ESR, 0);
	v = apic_read(APIC_ESR);

instead?  I can make a patch if that would make your life easier.

 There's room for optimisation here, but I think it's not worth the effort 
as this is a slow path, APIC error interrupts are not supposed to happen 
and are I believe extremely uncommon with FSB message delivery.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/