Re: [RFC][PATCH] kdump: x86_64 timer interrupt lockup due to pending interrupt

[ebiederm@xmission.com (Eric W. Biederman)]

Vivek Goyal <vgoyal@in.ibm.com> writes:

> On Tue, Mar 07, 2006 at 04:43:07PM -0700, Eric W. Biederman wrote:
>> Vivek Goyal <vgoyal@in.ibm.com> writes:
>> > On Mon, Mar 06, 2006 at 10:43:32PM +0100, Andi Kleen wrote:
>> >> On Mon, Mar 06, 2006 at 11:40:34AM -0500, Vivek Goyal wrote:
>> >> > 
>
> [..]
>> >> > 
>> >> > o In this patch, one extra EOI is being issued in check_timer() to unlock
>> > the
>> >> > vector. Please suggest if there is a better way to handle this situation.
>> >> 
>> >> Shouldn't we rather do this for all interrupts when the APIC is set up? 
>> >> I don't see how the timer is special here.
>> >>
>> >
>> > Timer is a special case here.
>> >
>> > In other cases, the moment interrupts are enabled on cpu, LAPIC pushes
> pending
>> > interrupts to cpu and it is ignored as bad irq using ack_bad_irq(). This
>> > still sends EOI to LAPIC if LPAIC support is compiled in.
>> >
>> > But for timer, the moment pending interrupt is pushed to cpu, it is handled
>> > as valid interrupt and cpu assumes that it came from 8259 and sends ack to
>> > 8259 and not to LAPIC. Hence leads to missing EOI for timer vector and 
>> > deadlock.
>> >
>> > But still doing it generic manner for all interrupts while setting up LAPIC
>> > probably makes more sense. Please find attached the patch.
>> 
>> A couple of questions. 
>> 
>> Does this need to be in #ifdef CONFIG_CRASS_DUMP?
>> If this code is truly safe I expect we could run it on every bootup
>> simply to be more robust.
>> 
>
> AFAIK, we can run this code safely on every bootup and can get rid of
> CONFIG_CRASH_DUMP. I have simply put it under it because I observed it
> only for crashdump scenarios. But removing this should be good as it
> protectets agains buggy boards. Modified patch is attached.
>
>
>> Why is APIC_ISR_NR a hard code?  I think there is an apic register
>> that tells the count.
>> 
>
> I did not find any such register. Basically ISR is a 256bit register. We
> are reading 32 bits at a time, so logically we can view it as 8, 32 bit
> registers. I had two options. Either I put a constant number in for()
> loop or #define it. I chose later one.
>
>> Does ack_APIC_irq take an argument?  I am confused that we are calling
>> ack_APIC_irq() potentially 8*32 times without passing it anything.
>> 
>
> It does not take any argument. Whenever a zero is written to EOI register
> LAPIC resets one ISR register bit corresponding to highest priority
> interrupt. So if all the ISR bits are set, we will call ack_APIC_irq()
> 8*32 times to reset them all.

Ok.  That makes sense.

Looks good to me.

Eric