On 8 August 2016 at 12:40, Alexei Fedorov <alexei.fedo...@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.biesheu...@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.fedo...@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-boun...@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.ll...@arm.com> wrote: >>> From: Alexei <alexei.fedo...@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.fedo...@arm.com> >>> Signed-off-by: Evan Lloyd <evan.ll...@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..34d4be3867647e0fdad7356c949 >>> 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..983936f3738a74bb5d5e08e0129 >>> 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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
