| Message ID | 4DA040A3.2070309@web.de |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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.
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); }