From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x235.google.com (mail-it0-x235.google.com [IPv6:2607:f8b0:4001:c0b::235]) (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 3CE171A1E18 for ; Mon, 8 Aug 2016 04:08:22 -0700 (PDT) Received: by mail-it0-x235.google.com with SMTP id x130so11887821ite.1 for ; Mon, 08 Aug 2016 04:08:22 -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=qVF9tEm+0T9mjUMKi1RVU9R6SGups5YirK6tsQNzcH0=; b=jaDmpgjOdkg7GCTHhT4+9Gkc2aYG3ihb9RDANiC8N44b6v1fRG1eOHIx25HKCeB91h hF1oVcFk0myhjQNNZHnQOJeMl+ffCSPQ/bf6OgD60FRNpNlaGKfm+UNCgb2XqMdbTLft HbheXKlM/9qw3GLu+Lq9pwmZKOhcnwzDj/ZMg= 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=qVF9tEm+0T9mjUMKi1RVU9R6SGups5YirK6tsQNzcH0=; b=Gy+S527ugOEffpzvEYPQS+jpR7++UwZko+tzkvnIz0MU6le78UlT/PrxRTegL5SYmp hPnTGdLx4RPdS9yghef0JsOA2HB+grgOjkYdbS9bJafYY+vD716HLN6EzbJ271q7SyVh rbwGqhvl9kaa1BfK9DQFXyg5ymgnBUWKnfVxHb2Hn3Tk6bdPsfVo9SgDmBW3ZDKvs/T6 pYkagS83j6POpj1uIBAElQaThSbGhCv14+qyaZ6y5b6W2+jwt0yLWs12K2ggMz0bo6uj 1XYXCJ18Ln8HJV6yRJu4VnCnANDfDLXRO492tjf6sRkxnIiqz1NtPJdLjs+Wcsm5YIGt ljig== X-Gm-Message-State: AEkoouviZU2QZ7hX+dCsFtFg5TKYLp9QLjDgGajm+hoGkjn2B9YzLYY1GHj3q1kvq+RdHoAEHEs6Q7Rf9XByZwsh X-Received: by 10.36.57.215 with SMTP id l206mr18195456ita.5.1470654501487; Mon, 08 Aug 2016 04:08:21 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 8 Aug 2016 04:08:21 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 13:08:21 +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 11:08:22 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 8 August 2016 at 13:06, Alexei Fedorov wrote: > Timer's pending interrupt is cleared HARDWARE_INTERRUPT_HANDLER TimerInt= erruptHandler() in when timer is re-programmed for the next shot. Yes, that is the timer side. > Does it mean that TimerDxe driver is part EFI_HARDWARE_INTERRUPT_PROTOCOL= ? > No. The peripheral and the GIC each have their own mask and enable registers for the interrupt. That is exactly why the comment describes in detail which part is the responsibility of the GIC, and which is the responsibility of the peripheral. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 08 August 2016 11:59 > 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:56, Alexei Fedorov wrote: >> I quoted: >> >> "The HARDWARE_INTERRUPT_HANDLER is responsible for clearing interrupt so= urces 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 handl= er. > > -- > 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 L= indholm >> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interr= upt >> >> On 8 August 2016 at 12:48, Alexei Fedorov wrote= : >>> Please provide the link to the spec which "clearly states that it is th= e 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 respo= nsible >>>>> 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 inter= rupt >>> >>> On 8 August 2016 at 12:40, Alexei Fedorov wrot= e: >>>> The interrupt is cleared in TimerInterruptHandler() (ArmPkg\Drivers\Ti= merDxe\TimerDxe.c) which is HARDWARE_INTERRUPT_HANDLER parameter passed to= gInterrupt->RegisterInterruptSource(). >>> >>> Indeed. So now, the HARDWARE_INTERRUPT_HANDLER is clearing the interrup= t 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 wro= te: >>>>> >>>>>> 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 respo= nsible >>>>> for clearing interrupt sources from individual devices." >>>>> >>>> >>>> Thanks for digging that up! >>>> >>>> So after this change, the driver implementing the hardware interrupt p= rotocol no longer clears the pending interrupt in the interrupt routing har= dware. This means that we are not only changing the existing contract, we a= re 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 = interrupt. >>>>>> 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 con= tract we have with registered interrupt handlers, and we don't know how thi= s may be used out of tree. I know UEFI only supports polling for drivers, b= ut are there any other cases (debug?) where we may break other people's cod= e 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 = confidential and may also be privileged. If you are not the intended recipi= ent, 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 informati= on in any medium. Thank you. >>>>> >>>> >>>> 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.