From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22e.google.com (mail-wm0-x22e.google.com [IPv6:2a00:1450:400c:c09::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 6216B1A1E0F for ; Mon, 8 Aug 2016 06:42:29 -0700 (PDT) Received: by mail-wm0-x22e.google.com with SMTP id q128so120141054wma.1 for ; Mon, 08 Aug 2016 06:42:29 -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=0gZhA9u911ZgDu7TTsaXFNlPjj7HL8/EQ/SnXXNl/80=; b=Zyn0FTniByMSdEbHJpEeUhV5KyWxhKxFMP5E79RLDmV97WH25Wgl+FYcPuszPSJyQX GLc/nBEhNeAaoE9/d6rHgGMhFuoanJ3+Y/T754aWKpI4tgb6get3pKa0d4NAAFgn7Zrb X5O6lb2arYg+FHoRWT10XSSDDFoyWG25v2XyQ= 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=0gZhA9u911ZgDu7TTsaXFNlPjj7HL8/EQ/SnXXNl/80=; b=OMZ1OORlB4sS4hMdL83qY/DKZkQSsmbRDij90zijDfxWXkXIGODT4Ziv08dOph2BAQ USrWVq61zPBpsiJzhwyv9B3nxEFBg12Sb/7ZD/j9k1VFFo4DYwvd1ar1HIWIDN1B5lUM 1uCG+Sh4cMYmQr1TP+Rot6MD5mxNUcyytx/yf72+O5rfu30Mx47fr/3e05lWFE+crJuz y6C9QMmuPoHNRUac8RT6ywiat5o4NWqdh74rp8K/U1w86UcNTjWqo4JsGt6kdBkXdSbn BaKDhhEgENRy7Wg6Imqz4utfDMPhDO9ebQNuZkQkvNhJOhW7uUKWnGdAeRmkwblrmfWx lknA== X-Gm-Message-State: AEkoouvcW5OYrffz8NVZGyqDrzY9QlR0LkXOojUbG2IOWVzziHyLC75QWmChuczcWssprdeIjRGELZI0yHYQCszZ X-Received: by 10.25.30.76 with SMTP id e73mr25332929lfe.202.1470663747451; Mon, 08 Aug 2016 06:42:27 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.157.204 with HTTP; Mon, 8 Aug 2016 06:42:26 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 15:42:26 +0200 Message-ID: To: "Cohen, Eugene" Cc: Alexei Fedorov , "Andrew Fish (afish@apple.com)" , "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 13:42:29 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 8 August 2016 at 15:22, Cohen, Eugene wrote: > Guys, sorry to join so late, something about timezones... Let me try to = provide some context and history. > >> > 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." > > I think you are reading more deeply into this verbiage than was intended.= From a separation-of-concerns perspective one driver is concerned with wr= iting to the hardware that generates the interrupt (handler) and another is= concerned with writing to the hardware for the interrupt controller to sig= nal the end of interrupt. So all this is saying is that "the code that tou= ches the interrupt controller is implemented in the driver that publishes t= his protocol". It does not say how this code is activated, only who is res= ponsible for poking the register. > > The historical expectation is that the handler driver calls the EOI inter= face in the protocol. (If it was the opposite then this interface wouldn't= even exist since the interrupt controller driver could just do it implicit= ly.) You're next question will probably be why it was designed this way - = for that we'll have to ask Andrew Fish (added). > > I did a little digging and see that the PC-AT chipset implemented an 8259= interrupt protocol (IntelFrameworkPkg\Include\Protocol\Legacy8259.h) that = is quite similar to HwInterrupt. Notice the explicit EndOfInterrupt interf= ace here and how it's used by the timer driver at PcAtChipsetPkg\8254TimerD= xe\Timer.c(88). > > Given this I asked that you keep the EndOfInterrupt in the handler driver= (s) and remove the auto-EOI in the interrupt controller driver, at least fo= r cases where a driver handled the interrupt. > > Feel free to clarify the text in the protocol header to align with this -= the current wording is not very clear. > Thanks for the context. I did some archaeology of my own, and it turns out that this code was introduced by Andrew in git commit 1bfda055dfbc52678 (svn #11293) more than 5 years ago. In any case, it appears we are in agreement that it is in fact incorrect (and deviates from the other implementations) to signal EOI in the GIC driver, and so I suppose Alexei's patch is good (and we only need to clarify the comment that he quoted in this thread). My primary concern was that we change the contract with existing handlers, but if there was a contract to begin with, we were already violating it, and so any out of tree breakage is not really our problem :-) Thanks all, Ard.