From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22b.google.com (mail-it0-x22b.google.com [IPv6:2607:f8b0:4001:c0b::22b]) (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 071F01A1E18 for ; Mon, 8 Aug 2016 03:32:27 -0700 (PDT) Received: by mail-it0-x22b.google.com with SMTP id u186so70191388ita.0 for ; Mon, 08 Aug 2016 03:32:26 -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=755prAf27NQf5wtbq8xFij3ga1Jw3R5tzLxi1Vr+9hs=; b=jeWWxwR+SgiemiQ8A5Uu9LUah681eQFedfYjXO1KRnSq+CLJBZzuiKkJkTu1vGb5MW UDy8FPT/5iHhG6VVFeS3B0j9Pei9axFdfaUP+70U8l0LlvzGl1QB+eLCnAcV6jimGfvm 9egQVelJ+/jSPCRilrqVjxePJR2rT8zavl3PI= 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=755prAf27NQf5wtbq8xFij3ga1Jw3R5tzLxi1Vr+9hs=; b=N4S1ppBbMdfwJLBB5PnEzNjCzS8w6AAic/ufIWO0N9ykADYwJGqXkDJji35GL1jm9U gNh1myal+j+EdlZn+psEJgRINOLQGu0AVROZNeQReqRjR7WMLGu2uwtd8NrPWPFNgeh3 sRaZBoxV38+aY50oGvXjxiGMTCMEGvbQPgeCDbkqOKR2Fo/uH6leZfbKWyklj3swjxzF MM46RTi1UoJb/LZzgN/gmmpVcJDGXbcOa09YIwysCr3XfqPi6r4KuywIDUslyBIU+NzC 3oEyNjOKdfFW7iOvWtObKZDUdosgJ2zPGFXp7N8U1CXRb+dOi+LAiduYBWIvkbP6Brho PYyg== X-Gm-Message-State: AEkoouuRoyrRCT5+wmnHfI8IFx5BmK/CVdP2gB+1Nxep7H8VtZa6kjAEsP5+OW6clinr65fDxIS8TK/XrHi0M1KJ X-Received: by 10.36.214.193 with SMTP id o184mr13734062itg.5.1470652346256; Mon, 08 Aug 2016 03:32:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 8 Aug 2016 03:32:25 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 12:32:25 +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:32:27 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 the > interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is responsib= le > 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-bounces@lists.01.org] On Behalf Of Ar= d 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 interru= pt > > (+ 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 inte= rrupt. >> 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 contrac= t we have with registered interrupt handlers, and we don't know how this ma= y be used out of tree. I know UEFI only supports polling for drivers, but a= re 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..34d4be3867647e0fdad7356c949a >> 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..983936f3738a74bb5d5e08e01297 >> 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 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. >