Re: [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 August 2016 at 12:56, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> I quoted:
>
> "The HARDWARE_INTERRUPT_HANDLER is responsible for clearing interrupt sources from individual devices".
> Are reading this as " spec clearly states that it is the GIC driver that should be doing it"?
>
>  TimerInterruptHandler() is HARDWARE_INTERRUPT_HANDLER and it clears its interrupt by calling gInterrupt->EndOfInterrupt().
>

No, I mean the other sentence:

>>>>   ...The driver implementing
>>>>   this protocol is responsible for clearing the pending interrupt in the
>>>>   interrupt routing hardware.

The GIC driver is responsible for calling EndOfInterrupt(), not the handler.

-- 
Ard.


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 08 August 2016 11:49
> To: Alexei Fedorov
> Cc: Evan Lloyd; Cohen, Eugene; edk2-devel@lists.01.org; Heyi Guo; Leif Lindholm
> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt
>
> On 8 August 2016 at 12:48, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
>> Please provide the link to the spec which "clearly states that it is the GIC driver that should be doing it."
>>
>
> it is not in the spec, but in the comment that you quoted yourself:
>
>>>> Looks like it does not:
>>>> From edk2\EmbeddedPkg\Include\Protocol\HardwareInterrupt.h:
>>>>
>>>> " Abstraction for hardware based interrupt routine
>>>>
>>>>   ...The driver implementing
>>>>   this protocol is responsible for clearing the pending interrupt in the
>>>>   interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is responsible
>>>>   for clearing interrupt sources from individual devices."
>
> --
> Ard.
>
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 08 August 2016 11:45
>> To: Alexei Fedorov
>> Cc: Evan Lloyd; Cohen, Eugene; edk2-devel@lists.01.org; Heyi Guo; Leif Lindholm
>> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt
>>
>> On 8 August 2016 at 12:40, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
>>> The interrupt is cleared in TimerInterruptHandler() (ArmPkg\Drivers\TimerDxe\TimerDxe.c)  which is HARDWARE_INTERRUPT_HANDLER parameter passed to gInterrupt->RegisterInterruptSource().
>>
>> Indeed. So now, the HARDWARE_INTERRUPT_HANDLER is clearing the interrupt in the interrupt routing hardware, while the spec clearly states that it is the GIC driver that should be doing it.
>>
>>
>>> Spurious interrupts which don't have registered interrupt handlers are cleared in GicV(2|3)IrqInterruptHandler().
>>>
>>
>> Yes, that is correct according to the text you quoted.
>>
>> --
>> Ard.
>>
>>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: 08 August 2016 11:32
>>> To: Alexei Fedorov
>>> Cc: Evan Lloyd; Cohen, Eugene; edk2-devel@lists.01.org; Heyi Guo; Leif
>>> Lindholm
>>> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per
>>> interrupt
>>>
>>> On 8 August 2016 at 12:25, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
>>>>
>>>>> it does change the contract we have with registered interrupt
>>>>> handlers
>>>>
>>>> Looks like it does not:
>>>> From edk2\EmbeddedPkg\Include\Protocol\HardwareInterrupt.h:
>>>>
>>>> " Abstraction for hardware based interrupt routine
>>>>
>>>>   ...The driver implementing
>>>>   this protocol is responsible for clearing the pending interrupt in the
>>>>   interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is responsible
>>>>   for clearing interrupt sources from individual devices."
>>>>
>>>
>>> Thanks for digging that up!
>>>
>>> So after this change, the driver implementing the hardware interrupt protocol no longer clears the pending interrupt in the interrupt routing hardware. This means that we are not only changing the existing contract, we are also violating the spec.
>>>
>>>
>>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>>>> Of Ard Biesheuvel
>>>> Sent: 06 August 2016 09:25
>>>> To: Evan Lloyd; Cohen, Eugene
>>>> Cc: edk2-devel@lists.01.org; Heyi Guo; Leif Lindholm
>>>> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per
>>>> interrupt
>>>>
>>>> (+ Eugene)
>>>>
>>>> On 5 August 2016 at 18:59,  <evan.lloyd@arm.com> wrote:
>>>>> From: Alexei <Alexei.Fedorov@arm.com>
>>>>>
>>>>> This commit fixes a bug in the GIC v2 and v3 drivers where the
>>>>> GICC_EOIR (End Of Interrupt Register) is written twice for a single interrupt.
>>>>> GicV(2|3)IrqInterruptHandler() calls the Interrupt Handler and then
>>>>> GicV(2|3)EndOfInterrupt() on exit:
>>>>>
>>>>>  InterruptHandler = gRegisteredInterruptHandlers[GicInterrupt];
>>>>>  if (InterruptHandler != NULL) {
>>>>>    // Call the registered interrupt handler.
>>>>>    InterruptHandler (GicInterrupt, SystemContext);  } else {
>>>>>    DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n",
>>>>> GicInterrupt));  }
>>>>>
>>>>>  GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
>>>>>
>>>>> , although gInterrupt->EndOfInterrupt() has already been called by
>>>>> InterruptHandler().
>>>>>
>>>>> The fix moves the EndOfInterrupt() call inside the else case for
>>>>> unregistered/spurious interrupts.  This removes a potential race
>>>>> condition that might have lost interrupts.
>>>>>
>>>>
>>>> I understand that this solves the problem, but it does change the contract we have with registered interrupt handlers, and we don't know how this may be used out of tree. I know UEFI only supports polling for drivers, but are there any other cases (debug?) where we may break other people's code by doing this?
>>>>
>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>
>>>>> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>>>>> ---
>>>>>
>>>>> Code is available at:
>>>>> https://github.com/EvanLloyd/tianocore/tree/EOIR_v1
>>>>>
>>>>>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 5 ++---
>>>>> ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 5 ++---
>>>>>  2 files changed, 4 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>>>>> b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>>>>> index
>>>>> 036eb5cd6bf6845dd2b03b62c933c1dedaef7251..34d4be3867647e0fdad7356c94
>>>>> 9
>>>>> a
>>>>> f8cd3ede7164 100644
>>>>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>>>>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>>>>> @@ -2,7 +2,7 @@
>>>>>
>>>>>  Copyright (c) 2009, Hewlett-Packard Company. All rights
>>>>> reserved.<BR> Portions copyright (c) 2010, Apple Inc. All rights
>>>>> reserved.<BR> -Portions copyright (c) 2011-2015, ARM Ltd. All rights
>>>>> reserved.<BR>
>>>>> +Portions copyright (c) 2011-2016, ARM Ltd. All rights reserved.<BR>
>>>>>
>>>>>  This program and the accompanying materials  are licensed and made
>>>>> available under the terms and conditions of the BSD License @@
>>>>> -178,9
>>>>> +178,8 @@ GicV2IrqInterruptHandler (
>>>>>      InterruptHandler (GicInterrupt, SystemContext);
>>>>>    } else {
>>>>>      DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n",
>>>>> GicInterrupt));
>>>>> +    GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol,
>>>>> + GicInterrupt);
>>>>>    }
>>>>> -
>>>>> -  GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol,
>>>>> GicInterrupt); }
>>>>>
>>>>>  //
>>>>> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>>>> b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>>>> index
>>>>> 106c669fcb8777dfaad609c0ce9f5b572727a3ff..983936f3738a74bb5d5e08e012
>>>>> 9
>>>>> 7
>>>>> 3df240958a8b 100644
>>>>> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>>>> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
>>>>> @@ -1,6 +1,6 @@
>>>>>  /** @file
>>>>>  *
>>>>> -*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
>>>>> +*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
>>>>>  *
>>>>>  *  This program and the accompanying materials
>>>>>  *  are licensed and made available under the terms and conditions
>>>>> of the BSD License @@ -169,9 +169,8 @@ GicV3IrqInterruptHandler (
>>>>>      InterruptHandler (GicInterrupt, SystemContext);
>>>>>    } else {
>>>>>      DEBUG ((EFI_D_ERROR, "Spurious GIC interrupt: 0x%x\n",
>>>>> GicInterrupt));
>>>>> +    GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol,
>>>>> + GicInterrupt);
>>>>>    }
>>>>> -
>>>>> -  GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol,
>>>>> GicInterrupt); }
>>>>>
>>>>>  //
>>>>> --
>>>>> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>>
>>>
>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.