From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22e.google.com (mail-io0-x22e.google.com [IPv6:2607:f8b0:4001:c06::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E2CB91A1E18 for ; Mon, 8 Aug 2016 03:49:06 -0700 (PDT) Received: by mail-io0-x22e.google.com with SMTP id m101so353518090ioi.2 for ; Mon, 08 Aug 2016 03:49:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=k/5FYCkpPY0qq1BvnfnB1Csy40s9P41V6xtv/QYvihA=; b=N/rVR743VYuJQT9JOEMD1ukJ+KTHYU0ESI9SX2Wkkg2EKp1k4ZvLf/YaavUH8FoFLu rw/+LhYdCCAxWEWCsTLtszvhxw/tJEvajFeUa47ZpAvjrSjHWTgC816rQ6HyFKJp7+IT 7Vj4y4V6TGmDshyl4ubZz+jtpVKMLo4Rp0CbQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=k/5FYCkpPY0qq1BvnfnB1Csy40s9P41V6xtv/QYvihA=; b=kicw8ZbkHh8bmQeSunTZbpkSzku1NLpn/nHRntvHRH2LG7qQl+GfzaY/DGSf4k09II k1gH+Ad2k6i+pcXL7rHThKdXx2k55DPyy7fyLTxmdAsQVgx1HuFoypEZZwCOUZpp32QE STVN9XOEKKr10ahilQLoKiN7VcaYbIyfmdPLHd/ZVw11SDNKdIycpyJSEhEIo0vCuL3v o/L3M5GMzgUGo153QdvGQFK01oiVADjxSdOSBnqfi5YaUCXs2f44wi/fW8+yOLolUK1B +sxFEsWBNNX8wwZLsDnSRA3pQntFyl24DX5p5ZOLgts36nD3Yb6xC3G/m8/NKUluvvpQ ireA== X-Gm-Message-State: AEkoouuelVFV6k+9XMklTIdX3Vjcctu+ciZ0Tbu4646uUNuoiNToxIovHGgAb0UjS7WOOTK019UPagej2mdn7ydS X-Received: by 10.107.40.133 with SMTP id o127mr89194516ioo.183.1470653346200; Mon, 08 Aug 2016 03:49:06 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 8 Aug 2016 03:49:05 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 12:49:05 +0200 Message-ID: To: Alexei Fedorov Cc: Evan Lloyd , "Cohen, Eugene" , "edk2-devel@lists.01.org" , Heyi Guo , Leif Lindholm Subject: Re: [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Aug 2016 10:49:07 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 8 August 2016 at 12:48, Alexei Fedorov 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 th= e >>> interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is respons= ible >>> for clearing interrupt sources from individual devices." --=20 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 Li= ndholm > Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interru= pt > > On 8 August 2016 at 12:40, Alexei Fedorov wrote: >> The interrupt is cleared in TimerInterruptHandler() (ArmPkg\Drivers\Time= rDxe\TimerDxe.c) which is HARDWARE_INTERRUPT_HANDLER parameter passed to g= Interrupt->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 c= leared 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 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 th= e >>> interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is respons= ible >>> for clearing interrupt sources from individual devices." >>> >> >> Thanks for digging that up! >> >> So after this change, the driver implementing the hardware interrupt pro= tocol no longer clears the pending interrupt in the interrupt routing hardw= are. 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, wrote: >>>> From: Alexei >>>> >>>> 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 in= terrupt. >>>> GicV(2|3)IrqInterruptHandler() calls the Interrupt Handler and then >>>> GicV(2|3)EndOfInterrupt() on exit: >>>> >>>> InterruptHandler =3D gRegisteredInterruptHandlers[GicInterrupt]; >>>> if (InterruptHandler !=3D 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 contr= act 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 >>>> Signed-off-by: Evan Lloyd >>>> --- >>>> >>>> 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.
Portions copyright (c) 2010, Apple Inc. All rights >>>> reserved.
-Portions copyright (c) 2011-2015, ARM Ltd. All rights >>>> reserved.
>>>> +Portions copyright (c) 2011-2016, ARM Ltd. All rights reserved.
>>>> >>>> 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 co= nfidential and may also be privileged. If you are not the intended recipien= t, 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 con= fidential 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 conf= idential and may also be privileged. If you are not the intended recipient,= please notify the sender immediately and do not disclose the contents to a= ny other person, use it for any purpose, or store or copy the information i= n any medium. Thank you.