Re: [PATCH] x86 apic: Ack all pending irqs when crashed/on kexec - V5

[Eric W. Biederman]

"H. Peter Anvin" <hpa@xxxxxxxxx> writes:

> On 06/16/2010 02:11 PM, Pan, Jacob jun wrote:
>> 
>> W.R.T. the loop limits, is it possible to use a default max_loops value in
>> case when cpu_khz is not set? The reason is that on Moorestown platform
>> we need to do an early APIC setup before tsc_init(), so cpu_khz is 0 at the
>> time we setup local APIC. The result is that we hit WARN_ON(max_loops<= 0)
>> on Moorestown for early APIC setup.

So you get a nasty warning but the system still boots?

>> The early APIC setup is needed because Moorestown does not have a PIT and the
>> system timer interrupts are routed via IOAPIC.
>> 
>
> Can't MRST install a quick ballpark value into cpu_khz?

Looking at the code the initialization order in init/main.c is:

early_irq_init()
init_IRQ()
init_timers()
time_init()
    tsc_init()

local_irq_enable()

So arguably if we simply switched those lines around we could make
this work, as you must be initializing the tsc with interrupts
disabled.

That said given the current ordering it appears that using the tsc
in setup_local_APIC is just broken because if it is properly called
from init_IRQ() the code doesn't work properly.

The question is what do we consider more important the current code
initialization ordering, or virtual processors having such an expensive
apic_read that we need a 1 second timeout.

I think the virtual processor concern is silly.  Most of the time
this loop should execute exactly once after having confirmed there
is nothing to do.  On a bad day this loop should execute at most twice.
If the vitalization is too expensive that this loop cannot execute
twice, bailing out early is a correctness concern.

I think we should set max_loops to 5.  Leave the WARN_ON, and call it
good.

Does the following patch work cleanly on Moorestown?

Eric