On 8 August 2016 at 12:56, Alexei Fedorov <[email protected]> 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:[email protected]] > Sent: 08 August 2016 11:49 > To: Alexei Fedorov > Cc: Evan Lloyd; Cohen, Eugene; [email protected]; 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 <[email protected]> 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:[email protected]] >> Sent: 08 August 2016 11:45 >> To: Alexei Fedorov >> Cc: Evan Lloyd; Cohen, Eugene; [email protected]; 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 <[email protected]> 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:[email protected]] >>> Sent: 08 August 2016 11:32 >>> To: Alexei Fedorov >>> Cc: Evan Lloyd; Cohen, Eugene; [email protected]; 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 <[email protected]> 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:[email protected]] On Behalf >>>> Of Ard Biesheuvel >>>> Sent: 06 August 2016 09:25 >>>> To: Evan Lloyd; Cohen, Eugene >>>> Cc: [email protected]; Heyi Guo; Leif Lindholm >>>> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per >>>> interrupt >>>> >>>> (+ Eugene) >>>> >>>> On 5 August 2016 at 18:59, <[email protected]> wrote: >>>>> From: Alexei <[email protected]> >>>>> >>>>> 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 <[email protected]> >>>>> Signed-off-by: Evan Lloyd <[email protected]> >>>>> --- >>>>> >>>>> 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 >>>> [email protected] >>>> 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. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
