Re: [Qemu-devel] [PATCH] ioapic: when switches to level trigger mode, interrupts raised repeatedly.

[Isaku Yamahata <yamahata@valinux.co.jp>]

On Mon, Apr 04, 2011 at 08:42:07AM +0900, Isaku Yamahata wrote:
> Thank you for applying. But I found that the patch is wrong and
> I'm preparing the new one. Can you please revert it?

Here is the corrected patch. The first wrong patch clears the interrupts
bit unconditionally. Which caused losing interrupt.

>From 5ed177d35ab14f3b070a0eba2c49400279a3a14b Mon Sep 17 00:00:00 2001
Message-Id: <5ed177d35ab14f3b070a0eba2c49400279a3a14b.1301883258.git.yamahata@valinux.co.jp>
In-Reply-To: <cover.1301883258.git.yamahata@valinux.co.jp>
References: <cover.1301883258.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Wed, 16 Mar 2011 14:00:13 +0900
Subject: [PATCH 01/30] ioapic: when switches to level trigger mode, interrupts raised repeatedly.

- the trigger mode is edge at first by reset.
- During initializatoin, the interrupt is raised as edge which is masked.
  The corresponding bit of irr is set.
- Then the mode is switched to level and it's unmasked with the same write.
- the bit of irr is set, so the interrupt is raised repeatedly by
  ioapic_service().
- OS considers that the irq line is broken and falls back to polling mode.

This patch fixes the issues.
When unmasking the pending interrupt and changing the interrupt mode
from edge to level, raise the edge interrupt first and then change the
interrupt mode. Thus the pending edge interrupt is raised as edge.

> Bringing up interface eth0:
> Determining IP information for eth0...irq 18: nobody cared (try booting with the "irqpoll" option)
> Pid: 4126, comm: ip Not tainted 2.6.38-rc7 #1
> Call Trace:
>  <IRQ>  [<ffffffff8105b009>] ? __report_bad_irq+0x38/0x87
>  [<ffffffff8105b177>] ? note_interrupt+0x11f/0x188
>  [<ffffffff8105bacf>] ? handle_fasteoi_irq+0xa7/0xd1
>  [<ffffffff810046ff>] ? handle_irq+0x83/0x8c
>  [<ffffffff81003eb9>] ? do_IRQ+0x48/0xaf
>  [<ffffffff81300513>] ? ret_from_intr+0x0/0xe
>  [<ffffffff81031ab8>] ? __do_softirq+0x4f/0x114
>  [<ffffffff81002d6c>] ? call_softirq+0x1c/0x28
>  [<ffffffff81004647>] ? do_softirq+0x33/0x68
>  [<ffffffff810316fb>] ? irq_exit+0x36/0x38
>  [<ffffffff81015f2c>] ? smp_apic_timer_interrupt+0x88/0x96
>  [<ffffffff81002853>] ? apic_timer_interrupt+0x13/0x20
>  <EOI>  [<ffffffff810177ed>] ? __ioapic_set_affinity+0x68/0x7c
>  [<ffffffff813000f0>] ? _raw_spin_unlock_irqrestore+0x8/0xa
>  [<ffffffff8105a84f>] ? __setup_irq+0x224/0x2cb
>  [<ffffffff8120e3c5>] ? e1000_intr+0x0/0x103
>  [<ffffffff8105a9c7>] ? request_threaded_irq+0xd1/0x114
>  [<ffffffff8120e396>] ? e1000_request_irq+0x34/0x63
>  [<ffffffff8121237d>] ? e1000_open+0x81/0x11f
>  [<ffffffff8129097c>] ? call_netdevice_notifiers+0x45/0x4a
>  [<ffffffff81290d8d>] ? __dev_open+0x97/0xc4
>  [<ffffffff8128e9c5>] ? __dev_change_flags+0xb9/0x13d
>  [<ffffffff81290cc1>] ? dev_change_flags+0x1c/0x51
>  [<ffffffff812d0542>] ? devinet_ioctl+0x26e/0x594
>  [<ffffffff812d174c>] ? inet_ioctl+0x92/0xaa
>  [<ffffffff81281d75>] ? T.1003+0x13/0x32
>  [<ffffffff81282152>] ? sock_ioctl+0x1f2/0x1ff
>  [<ffffffff810ae2d3>] ? do_vfs_ioctl+0x498/0x4e7
>  [<ffffffff81281203>] ? sock_alloc_file+0xb3/0x115
>  [<ffffffff8109f79f>] ? fd_install+0x31/0x5d
>  [<ffffffff810ae364>] ? sys_ioctl+0x42/0x65
>  [<ffffffff81001f3b>] ? system_call_fastpath+0x16/0x1b
> handlers:
> [<ffffffff8120e3c5>] (e1000_intr+0x0/0x103)
> Disabling IRQ #18

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/ioapic.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 569327d..2f9e506 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -257,6 +257,33 @@ ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
                     s->ioredtbl[index] &= 0xffffffff;
                     s->ioredtbl[index] |= (uint64_t)val << 32;
                 } else {
+                    uint32_t mask = 1 << index;
+                    uint8_t trig_mode =
+                        ((s->ioredtbl[index] >>
+                          IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
+                    uint8_t trig_mode_new =
+                        ((val >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
+                    if ((s->irr & mask) && !(val & IOAPIC_LVT_MASKED) &&
+                        trig_mode == IOAPIC_TRIGGER_EDGE &&
+                        trig_mode_new == IOAPIC_TRIGGER_LEVEL) {
+                        /*
+                         * edge interrupt was raised and it has been masked.
+                         * Now it is unmasked and its mode is changed to level.
+                         *
+                         * The device thinks it raised the interrupts as edge,
+                         * thus they don't have to lower it.
+                         * So if we just handle this pending interrupts as
+                         * level, the interrupt will be never lowered.
+                         * So OS interprets this interrupt storm as
+                         * the interrupt line is broken.
+                         *
+                         * To avoid such interrupt storm, unmask the interrupt,
+                         * service the pending interrupts as edge,
+                         * and then change the trigger mode.
+                         */
+                        s->ioredtbl[index] &= ~IOAPIC_LVT_MASKED;
+                        ioapic_service(s);
+                    }
                     s->ioredtbl[index] &= ~0xffffffffULL;
                     s->ioredtbl[index] |= val;
                 }
-- 
1.7.1.1


-- 
yamahata