Re: [PATCH 45/79] [PATCH] fix apic acking of irqs

[Glauber Costa <gcosta@redhat.com>]

Maciej W. Rozycki wrote:
> On Tue, 25 Mar 2008, Glauber Costa wrote:
> 
>> the only ESR mention I see in setup_local_APIC() is this:
>>
>>          /* Pound the ESR really hard over the head with a big hammer - mbligh
>> */
>>         if (esr_disable) {
>>                 apic_write(APIC_ESR, 0);
>>                 apic_write(APIC_ESR, 0);
>>                 apic_write(APIC_ESR, 0);
>>                 apic_write(APIC_ESR, 0);
>>         }
>> which seems more like a disablement.
> 
>  There is more later on...
> 
>> the bootup code does clean it, tough, by writing and reading the ESR.
> 
> ... basically for the original Pentium and Pentium/MMX APIC you only had 
> to read the ESR to get at the bits.  The read would clear them as well as 
> a side-effect.  Although at that stage already it was mentioned in the 
> spec that for future compatibility a write of zero beforehand (ignored as 
> the register was r/o) should be performed.  Which indeed became a 
> requirement from PentiumPro onwards as with these processors it was the 
> write that copied the internal error latches into the visible ESR 
> register.  Except that some Pentium APICs had an erratum, where ESR was 
> indeed r/w and the leading write of zero would actually clear the register 
> losing the recorded state, so it had to be avoided despite the 
> recommendation.  Hence the code you can see within:
> 
> if (integrated && !esr_disable) {
> }
> 
> I suppose other APIC implementers were not that keen on keeping bug 
> compatibility, so chances are other APIC core work just fine as specified 
> by the architecture (for whatever the meaning of "fine" is).
> 
>  Note the usual APIC error interrupt handler is smp_error_interrupt().
> 
>>>  I have asked this question already: what kind of CPU are you running on?
>>> Do you really need to have CONFIG_X86_GOOD_APIC clear with it?
>>>
>> My testings that triggered that were with qemu, with randconfigs. Probably it
>> has a good apic, but it is good that it triggered anyway. Otherwise I'd never
>> see it.
> 
>  Ah, I see -- it may be worth checking what actual hardware does and 
> fixing QEMU if necessary for it to match reality then. ;-)
> 
>  OTOH, if actual modern hardware triggered such an error, then for the 
> sake of a generic "runs everywhere" kernel either ack_APIC_irq() or even 
> apic_write_around() could be modified to perform a run-time check if 
> configured with !CONFIG_X86_GOOD_APIC and avoid the read if unnecessary; 
> it's an erratum workaround after all and SMP Pentium systems suffering 
> from this bug (UP Pentium systems did not nor had a way to enable the 
> local APIC normally) are probably an insignificant minority if any at all 
> left these days.  Therefore it should be a negligible sacrifice of 
> performance.
I just tested in some real i386 that I have around here, and the reading
of EOI does not seem to be illegal. (Well, at least in those I've tested).

OTOH, ignoring the read in qemu makes the tree boot just okay. So I 
agree with you now, we might well fix qemu, and revert this patch.

Ingo, any word on that?



>   Maciej