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

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 --]

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