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

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

[Isaku Yamahata]

- the trigger mode is edge at first
- 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.
- 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.
After raising edige, clear the bit of irr.

> 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 |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 569327d..8557e5c 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -164,6 +164,7 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
             if (level) {
                 s->irr |= mask;
                 ioapic_service(s);
+                s->irr &= ~mask;
             }
         }
     }
-- 
1.7.1.1

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

[Aurelien Jarno]

On Wed, Mar 16, 2011 at 06:05:01PM +0900, Isaku Yamahata wrote:
> - the trigger mode is edge at first
> - 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.
> - 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.
> After raising edige, clear the bit of irr.
> 
> > 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 |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Thanks, applied.

> diff --git a/hw/ioapic.c b/hw/ioapic.c
> index 569327d..8557e5c 100644
> --- a/hw/ioapic.c
> +++ b/hw/ioapic.c
> @@ -164,6 +164,7 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>              if (level) {
>                  s->irr |= mask;
>                  ioapic_service(s);
> +                s->irr &= ~mask;
>              }
>          }
>      }
> -- 
> 1.7.1.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Isaku Yamahata]

Thank you for applying. But I found that the patch is wrong and
I'm preparing the new one. Can you please revert it?

On Sun, Apr 03, 2011 at 09:53:14PM +0200, Aurelien Jarno wrote:
> On Wed, Mar 16, 2011 at 06:05:01PM +0900, Isaku Yamahata wrote:
> > - the trigger mode is edge at first
> > - 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.
> > - 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.
> > After raising edige, clear the bit of irr.
> > 
> > > 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 |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> Thanks, applied.
> 
> > diff --git a/hw/ioapic.c b/hw/ioapic.c
> > index 569327d..8557e5c 100644
> > --- a/hw/ioapic.c
> > +++ b/hw/ioapic.c
> > @@ -164,6 +164,7 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
> >              if (level) {
> >                  s->irr |= mask;
> >                  ioapic_service(s);
> > +                s->irr &= ~mask;
> >              }
> >          }
> >      }
> > -- 
> > 1.7.1.1
> > 
> > 
> > 
> 
> -- 
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
> 

-- 
yamahata

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

[Isaku Yamahata]

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

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

[Aurelien Jarno]

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?

Reverted for now, I'll ait for some comments on the new patch.

> On Sun, Apr 03, 2011 at 09:53:14PM +0200, Aurelien Jarno wrote:
> > On Wed, Mar 16, 2011 at 06:05:01PM +0900, Isaku Yamahata wrote:
> > > - the trigger mode is edge at first
> > > - 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.
> > > - 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.
> > > After raising edige, clear the bit of irr.
> > > 
> > > > 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 |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > Thanks, applied.
> > 
> > > diff --git a/hw/ioapic.c b/hw/ioapic.c
> > > index 569327d..8557e5c 100644
> > > --- a/hw/ioapic.c
> > > +++ b/hw/ioapic.c
> > > @@ -164,6 +164,7 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
> > >              if (level) {
> > >                  s->irr |= mask;
> > >                  ioapic_service(s);
> > > +                s->irr &= ~mask;
> > >              }
> > >          }
> > >      }
> > > -- 
> > > 1.7.1.1
> > > 
> > > 
> > > 
> > 
> > -- 
> > Aurelien Jarno                          GPG: 1024D/F1BCDB73
> > aurelien@aurel32.net                 http://www.aurel32.net
> > 
> 
> -- 
> yamahata
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

On 2011-04-04 04:15, Isaku Yamahata wrote:
> 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.

...and that is the actual problem. The spec says: "Interrupt Mask—R/W.
When this bit is 1, the interrupt signal is masked. Edge-sensitive
interrupts signaled on a masked interrupt pin are ignored (i.e., not
delivered or held pending)."

So this should do the trick in a correct way (untested, please
validate):

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 569327d..6663f41 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -161,7 +161,7 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
             }
         } else {
             /* edge triggered */
-            if (level) {
+            if (level && !(entry & IOAPIC_LVT_MASKED)) {
                 s->irr |= mask;
                 ioapic_service(s);
             }

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Isaku Yamahata]

On Sat, Apr 09, 2011 at 10:38:10AM +0200, Jan Kiszka wrote:
> On 2011-04-04 04:15, Isaku Yamahata wrote:
> > 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.
> 
> ...and that is the actual problem. The spec says: "Interrupt Mask?R/W.
> When this bit is 1, the interrupt signal is masked. Edge-sensitive
> interrupts signaled on a masked interrupt pin are ignored (i.e., not
> delivered or held pending)."
> 
> So this should do the trick in a correct way (untested, please
> validate):

Thank you for referring the spec. It works.
Here's the updated patch with your signed-off-by and my tested-by.


>From a6c92855357a24da6a0d8d6e76dcca735a4be885 Mon Sep 17 00:00:00 2001
Message-Id: <a6c92855357a24da6a0d8d6e76dcca735a4be885.1302346833.git.yamahata@valinux.co.jp>
In-Reply-To: <cover.1302346833.git.yamahata@valinux.co.jp>
References: <cover.1302346833.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Wed, 16 Mar 2011 14:00:13 +0900
Subject: [PATCH] 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.
  the bit must not be set when masked
- 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.

>From the specification, the masked edge triggered interrupt must
be ingored as follows.
  3.4.2 I/O redirection table registers: Interrupt Mask
  Edge-sensitive interrupts signaled on a masked interrupt pin
  are ignored (i.e., not delivered or held pending).

> 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: Jan Kiszka <jan.kiszka@web.de>
Tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/ioapic.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 569327d..42c5037 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -160,8 +160,13 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
                 s->irr &= ~mask;
             }
         } else {
-            /* edge triggered */
-            if (level) {
+            /*
+             * edge triggered
+             * 3.4.2 I/O redirection table registers: Interrupt Mask
+             * Edge-sensitive interrupts signaled on a masked interrupt pin
+             * are ignored (i.e., not delivered or held pending).
+             */
+            if (level && !(entry & IOAPIC_LVT_MASKED)) {
                 s->irr |= mask;
                 ioapic_service(s);
             }
-- 
1.7.1.1



-- 
yamahata

[Qemu-devel] [PATCH] ioapic: Do not set irr for masked edge IRQs

[Jan Kiszka]

On 2011-04-09 13:05, Isaku Yamahata wrote:
> On Sat, Apr 09, 2011 at 10:38:10AM +0200, Jan Kiszka wrote:
>> On 2011-04-04 04:15, Isaku Yamahata wrote:
>>> 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.
>>
>> ...and that is the actual problem. The spec says: "Interrupt Mask?R/W.
>> When this bit is 1, the interrupt signal is masked. Edge-sensitive
>> interrupts signaled on a masked interrupt pin are ignored (i.e., not
>> delivered or held pending)."
>>
>> So this should do the trick in a correct way (untested, please
>> validate):
> 
> Thank you for referring the spec. It works.
> Here's the updated patch with your signed-off-by and my tested-by.

Thanks for testing. I would prefer the following more compact wordings.

Jan

---------8<----------

From: Jan Kiszka <jan.kiszka@siemens.com>

So far we set IRR for edge IRQs even if the pin is masked. If the guest
later on unmasks and switches the pin to level-triggered mode, irr will
remain set, causing an IRQ storm. The point is that setting IRR is not
correct in this case according to the spec, and avoiding this resolves
the issue.

Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioapic.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 569327d..6c26e82 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
                 s->irr &= ~mask;
             }
         } else {
-            /* edge triggered */
-            if (level) {
+            /* According to the 82093AA manual, we must ignore edge requests
+             * if the input pin is masked. */
+            if (level && !(entry & IOAPIC_LVT_MASKED)) {
                 s->irr |= mask;
                 ioapic_service(s);
             }
-- 
1.7.1

Re: [PATCH] ioapic: Do not set irr for masked edge IRQs

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On 2011-04-09 13:18, Jan Kiszka wrote:
> On 2011-04-09 13:05, Isaku Yamahata wrote:
>> On Sat, Apr 09, 2011 at 10:38:10AM +0200, Jan Kiszka wrote:
>>> On 2011-04-04 04:15, Isaku Yamahata wrote:
>>>> 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.
>>>
>>> ...and that is the actual problem. The spec says: "Interrupt Mask?R/W.
>>> When this bit is 1, the interrupt signal is masked. Edge-sensitive
>>> interrupts signaled on a masked interrupt pin are ignored (i.e., not
>>> delivered or held pending)."
>>>
>>> So this should do the trick in a correct way (untested, please
>>> validate):
>>
>> Thank you for referring the spec. It works.
>> Here's the updated patch with your signed-off-by and my tested-by.
> 
> Thanks for testing. I would prefer the following more compact wordings.
> 
> Jan
> 
> ---------8<----------
> 
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> So far we set IRR for edge IRQs even if the pin is masked. If the guest
> later on unmasks and switches the pin to level-triggered mode, irr will
> remain set, causing an IRQ storm. The point is that setting IRR is not
> correct in this case according to the spec, and avoiding this resolves
> the issue.
> 
> Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/ioapic.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ioapic.c b/hw/ioapic.c
> index 569327d..6c26e82 100644
> --- a/hw/ioapic.c
> +++ b/hw/ioapic.c
> @@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>                  s->irr &= ~mask;
>              }
>          } else {
> -            /* edge triggered */
> -            if (level) {
> +            /* According to the 82093AA manual, we must ignore edge requests
> +             * if the input pin is masked. */
> +            if (level && !(entry & IOAPIC_LVT_MASKED)) {
>                  s->irr |= mask;
>                  ioapic_service(s);
>              }

On first glance, it looks like KVM's in-kernel IOAPIC model is affected
by the same issue. As you have the test case at hand, could you run it
against qemu-kvm which stresses the kernel version?

TIA,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH] ioapic: Do not set irr for masked edge IRQs

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On 2011-04-09 13:18, Jan Kiszka wrote:
> On 2011-04-09 13:05, Isaku Yamahata wrote:
>> On Sat, Apr 09, 2011 at 10:38:10AM +0200, Jan Kiszka wrote:
>>> On 2011-04-04 04:15, Isaku Yamahata wrote:
>>>> 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.
>>>
>>> ...and that is the actual problem. The spec says: "Interrupt Mask?R/W.
>>> When this bit is 1, the interrupt signal is masked. Edge-sensitive
>>> interrupts signaled on a masked interrupt pin are ignored (i.e., not
>>> delivered or held pending)."
>>>
>>> So this should do the trick in a correct way (untested, please
>>> validate):
>>
>> Thank you for referring the spec. It works.
>> Here's the updated patch with your signed-off-by and my tested-by.
> 
> Thanks for testing. I would prefer the following more compact wordings.
> 
> Jan
> 
> ---------8<----------
> 
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> So far we set IRR for edge IRQs even if the pin is masked. If the guest
> later on unmasks and switches the pin to level-triggered mode, irr will
> remain set, causing an IRQ storm. The point is that setting IRR is not
> correct in this case according to the spec, and avoiding this resolves
> the issue.
> 
> Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/ioapic.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ioapic.c b/hw/ioapic.c
> index 569327d..6c26e82 100644
> --- a/hw/ioapic.c
> +++ b/hw/ioapic.c
> @@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>                  s->irr &= ~mask;
>              }
>          } else {
> -            /* edge triggered */
> -            if (level) {
> +            /* According to the 82093AA manual, we must ignore edge requests
> +             * if the input pin is masked. */
> +            if (level && !(entry & IOAPIC_LVT_MASKED)) {
>                  s->irr |= mask;
>                  ioapic_service(s);
>              }

On first glance, it looks like KVM's in-kernel IOAPIC model is affected
by the same issue. As you have the test case at hand, could you run it
against qemu-kvm which stresses the kernel version?

TIA,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] ioapic: Do not set irr for masked edge IRQs

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On 2011-04-09 13:26, Jan Kiszka wrote:
> On 2011-04-09 13:18, Jan Kiszka wrote:
>> On 2011-04-09 13:05, Isaku Yamahata wrote:
>>> On Sat, Apr 09, 2011 at 10:38:10AM +0200, Jan Kiszka wrote:
>>>> On 2011-04-04 04:15, Isaku Yamahata wrote:
>>>>> 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.
>>>>
>>>> ...and that is the actual problem. The spec says: "Interrupt Mask?R/W.
>>>> When this bit is 1, the interrupt signal is masked. Edge-sensitive
>>>> interrupts signaled on a masked interrupt pin are ignored (i.e., not
>>>> delivered or held pending)."
>>>>
>>>> So this should do the trick in a correct way (untested, please
>>>> validate):
>>>
>>> Thank you for referring the spec. It works.
>>> Here's the updated patch with your signed-off-by and my tested-by.
>>
>> Thanks for testing. I would prefer the following more compact wordings.
>>
>> Jan
>>
>> ---------8<----------
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> So far we set IRR for edge IRQs even if the pin is masked. If the guest
>> later on unmasks and switches the pin to level-triggered mode, irr will
>> remain set, causing an IRQ storm. The point is that setting IRR is not
>> correct in this case according to the spec, and avoiding this resolves
>> the issue.
>>
>> Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/ioapic.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ioapic.c b/hw/ioapic.c
>> index 569327d..6c26e82 100644
>> --- a/hw/ioapic.c
>> +++ b/hw/ioapic.c
>> @@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>>                  s->irr &= ~mask;
>>              }
>>          } else {
>> -            /* edge triggered */
>> -            if (level) {
>> +            /* According to the 82093AA manual, we must ignore edge requests
>> +             * if the input pin is masked. */
>> +            if (level && !(entry & IOAPIC_LVT_MASKED)) {
>>                  s->irr |= mask;
>>                  ioapic_service(s);
>>              }
> 
> On first glance, it looks like KVM's in-kernel IOAPIC model is affected
> by the same issue. As you have the test case at hand, could you run it
> against qemu-kvm which stresses the kernel version?

On second glance, it should be fine as it clears irr unconditionally on
the falling edge. Both approaches look valid to me as irr is just an
internal state.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH] ioapic: Do not set irr for masked edge IRQs

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On 2011-04-09 13:26, Jan Kiszka wrote:
> On 2011-04-09 13:18, Jan Kiszka wrote:
>> On 2011-04-09 13:05, Isaku Yamahata wrote:
>>> On Sat, Apr 09, 2011 at 10:38:10AM +0200, Jan Kiszka wrote:
>>>> On 2011-04-04 04:15, Isaku Yamahata wrote:
>>>>> 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.
>>>>
>>>> ...and that is the actual problem. The spec says: "Interrupt Mask?R/W.
>>>> When this bit is 1, the interrupt signal is masked. Edge-sensitive
>>>> interrupts signaled on a masked interrupt pin are ignored (i.e., not
>>>> delivered or held pending)."
>>>>
>>>> So this should do the trick in a correct way (untested, please
>>>> validate):
>>>
>>> Thank you for referring the spec. It works.
>>> Here's the updated patch with your signed-off-by and my tested-by.
>>
>> Thanks for testing. I would prefer the following more compact wordings.
>>
>> Jan
>>
>> ---------8<----------
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> So far we set IRR for edge IRQs even if the pin is masked. If the guest
>> later on unmasks and switches the pin to level-triggered mode, irr will
>> remain set, causing an IRQ storm. The point is that setting IRR is not
>> correct in this case according to the spec, and avoiding this resolves
>> the issue.
>>
>> Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/ioapic.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ioapic.c b/hw/ioapic.c
>> index 569327d..6c26e82 100644
>> --- a/hw/ioapic.c
>> +++ b/hw/ioapic.c
>> @@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>>                  s->irr &= ~mask;
>>              }
>>          } else {
>> -            /* edge triggered */
>> -            if (level) {
>> +            /* According to the 82093AA manual, we must ignore edge requests
>> +             * if the input pin is masked. */
>> +            if (level && !(entry & IOAPIC_LVT_MASKED)) {
>>                  s->irr |= mask;
>>                  ioapic_service(s);
>>              }
> 
> On first glance, it looks like KVM's in-kernel IOAPIC model is affected
> by the same issue. As you have the test case at hand, could you run it
> against qemu-kvm which stresses the kernel version?

On second glance, it should be fine as it clears irr unconditionally on
the falling edge. Both approaches look valid to me as irr is just an
internal state.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] ioapic: Do not set irr for masked edge IRQs

[Isaku Yamahata]

On Sat, Apr 09, 2011 at 01:26:07PM +0200, Jan Kiszka wrote:
> On first glance, it looks like KVM's in-kernel IOAPIC model is affected
> by the same issue. 

Agreed.


> As you have the test case at hand, could you run it
> against qemu-kvm which stresses the kernel version?

Unfortunately I don't have kvm test case yet because I found
this issue when trying to change IOAPIC irq routing.
-- 
yamahata

[Qemu-devel] Re: [PATCH] ioapic: Do not set irr for masked edge IRQs

[Isaku Yamahata]

On Sat, Apr 09, 2011 at 01:26:07PM +0200, Jan Kiszka wrote:
> On first glance, it looks like KVM's in-kernel IOAPIC model is affected
> by the same issue. 

Agreed.


> As you have the test case at hand, could you run it
> against qemu-kvm which stresses the kernel version?

Unfortunately I don't have kvm test case yet because I found
this issue when trying to change IOAPIC irq routing.
-- 
yamahata

Re: [Qemu-devel] [PATCH] ioapic: Do not set irr for masked edge IRQs

[Jan Kiszka]

On 2011-04-09 13:18, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> So far we set IRR for edge IRQs even if the pin is masked. If the guest
> later on unmasks and switches the pin to level-triggered mode, irr will
> remain set, causing an IRQ storm. The point is that setting IRR is not
> correct in this case according to the spec, and avoiding this resolves
> the issue.
> 
> Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/ioapic.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ioapic.c b/hw/ioapic.c
> index 569327d..6c26e82 100644
> --- a/hw/ioapic.c
> +++ b/hw/ioapic.c
> @@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>                  s->irr &= ~mask;
>              }
>          } else {
> -            /* edge triggered */
> -            if (level) {
> +            /* According to the 82093AA manual, we must ignore edge requests
> +             * if the input pin is masked. */
> +            if (level && !(entry & IOAPIC_LVT_MASKED)) {
>                  s->irr |= mask;
>                  ioapic_service(s);
>              }

Ping?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] ioapic: Do not set irr for masked edge IRQs

[Aurelien Jarno]

On Tue, Apr 26, 2011 at 03:00:30PM +0200, Jan Kiszka wrote:
> On 2011-04-09 13:18, Jan Kiszka wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > So far we set IRR for edge IRQs even if the pin is masked. If the guest
> > later on unmasks and switches the pin to level-triggered mode, irr will
> > remain set, causing an IRQ storm. The point is that setting IRR is not
> > correct in this case according to the spec, and avoiding this resolves
> > the issue.
> > 
> > Reported-and-tested-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >  hw/ioapic.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ioapic.c b/hw/ioapic.c
> > index 569327d..6c26e82 100644
> > --- a/hw/ioapic.c
> > +++ b/hw/ioapic.c
> > @@ -160,8 +160,9 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
> >                  s->irr &= ~mask;
> >              }
> >          } else {
> > -            /* edge triggered */
> > -            if (level) {
> > +            /* According to the 82093AA manual, we must ignore edge requests
> > +             * if the input pin is masked. */
> > +            if (level && !(entry & IOAPIC_LVT_MASKED)) {
> >                  s->irr |= mask;
> >                  ioapic_service(s);
> >              }
> 
> Ping?
> 

Done.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net