From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x234.google.com (mail-it0-x234.google.com [IPv6:2607:f8b0:4001:c0b::234]) (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 2D3CE1A1E18 for ; Mon, 8 Aug 2016 03:58:56 -0700 (PDT) Received: by mail-it0-x234.google.com with SMTP id j124so78788810ith.1 for ; Mon, 08 Aug 2016 03:58:56 -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+SK+l9/32VtaCysZA2Xboh+IcUqHfT6ggaDw2+zs/A=; b=DNsO1G2pMjwjGWyxJrtD8Zoow4hp3QRH5aZuEMyOKONfu6mlwL16n3hlSZCbmMz66c 5Dwsb2kpFmujC21rywBhuqYhVY2kYXmoHb4rrw+wH8zLyJuVEy47vURha5U88kyat9OE QrBA5Y6MEvTdncR6Ska+ZVPif1qwTY1jL4U00= 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+SK+l9/32VtaCysZA2Xboh+IcUqHfT6ggaDw2+zs/A=; b=DFLR9fi3w7ry/IaBy/b/8ns/qpnBAZZ6cTsw3Rmoge2sSHR0gMGX1p7/HKrP/GGCxt I5B6v3kacE0zbnZgNQLbczJHKBi7JWqvAedT+enEekGTDdWNVDP60TvV8fiq4510sjsM xPo6/nNpsrBsDK4F1HxKMp/rPzzKtzUuUdIgxBE+LYzPr33+pMLSclooXLf5N1jzaug8 CNoUM9Kp9gBO5GFSOToxqeR59Aq/clRN2G1icx1JDb4Sr9Jc56I2pixcJQcQNekrdlWW WTMVSLyixVIy2IBlUku8wwAQkBo9l2uv3PwykRypfh1NMvweQV8E/kbFYK+czvRvGQtD Bc9g== X-Gm-Message-State: AEkoout/ji4PD45L3gzvGbwxh2+hgLuAgox3jTefWxG0O7Np2GLPne8B490xEwII2e633laED9AicADz6gdMifCj X-Received: by 10.36.107.211 with SMTP id v202mr16543080itc.51.1470653935372; Mon, 08 Aug 2016 03:58:55 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 8 Aug 2016 03:58:54 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 12:58:54 +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:58:56 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 8 August 2016 at 12:56, Alexei Fedorov wrote: > I quoted: > > "The HARDWARE_INTERRUPT_HANDLER is responsible for clearing interrupt sou= rces 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 t= he >>>> interrupt routing hardware. The GIC driver is responsible for calling EndOfInterrupt(), not the handler= . --=20 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 Li= ndholm > Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interru= pt > > 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 t= he >>>> interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is respon= sible >>>> 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 L= indholm >> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interr= upt >> >> On 8 August 2016 at 12:40, Alexei Fedorov wrote= : >>> The interrupt is cleared in TimerInterruptHandler() (ArmPkg\Drivers\Tim= erDxe\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 i= s 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 wrot= e: >>>> >>>>> 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 t= he >>>> interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is respon= sible >>>> for clearing interrupt sources from individual devices." >>>> >>> >>> Thanks for digging that up! >>> >>> So after this change, the driver implementing the hardware interrupt pr= otocol no longer clears the pending interrupt in the interrupt routing hard= ware. This means that we are not only changing the existing contract, we ar= e 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 i= nterrupt. >>>>> 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 cont= ract 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, bu= t 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 c= onfidential and may also be privileged. If you are not the intended recipie= nt, please notify the sender immediately and do not disclose the contents t= o any other person, use it for any purpose, or store or copy the informatio= n in any medium. Thank you. >>>> >>> >>> 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.