From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (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 171361A1E0F for ; Mon, 8 Aug 2016 06:51:00 -0700 (PDT) Received: by mail-wm0-x233.google.com with SMTP id o80so140676348wme.1 for ; Mon, 08 Aug 2016 06:51:00 -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=SgWqZ7OgwOGFJ9xApCujruMDSmCZ4YhohbLmstPD9Ok=; b=Z0rMJTaGJLGr9pBv2dYLqCZlOQfP4v1JyDdsXB9PrXE09zUfEi0gv6PsdCxd8j/axx qhuapS3hdJwwsNnA9qgif/fbVVfEv4FIg3Edlh5UJSH0MJScwqitpjlzQdhaiQ3QsCGL jfXLEdZSAFI0pkIJ+wGRku0aDp88pdIibWo1k= 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=SgWqZ7OgwOGFJ9xApCujruMDSmCZ4YhohbLmstPD9Ok=; b=HhlIjodDsfr574AfRFUsKMKo2hyMZUK5q51pnddFT7aWDCxjwoWly8uW1SXBGMspQ7 ioafcIyifxWwpYjeEUg63S28dOpu5KJvmMnnuLHkpEYEw5JNOm2eRiMFCrj1k4o/Xqou BIykY9mKuATcszSbhYfjGjPw2RXj2mdFzc+xB2WlntgQQqLvEV6Ilm+acXAhNcfi1w5L JYLiAIMvUr73hNWQPkcWqeSLyJ/fcrPc8OBgQ5xIa7C97FQ9xSyN+2uSnDOqCd6rbJvB xuS+wDqmXDob1c/Kusb8D8q/5VDS/7k/JpSjOCQmdh6cSkWue8ce5LavV6lbY5nK3GLn dh3A== X-Gm-Message-State: AEkoousMwpls841xPDhlWk7mzZCTQBRj/9I4CHMbrnq14G8zSupNzRWrABVo7XUzjO9yI4GB0KAdxQIoUCLPETFr X-Received: by 10.25.138.5 with SMTP id m5mr24790522lfd.213.1470664258545; Mon, 08 Aug 2016 06:50:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.157.204 with HTTP; Mon, 8 Aug 2016 06:50:57 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 15:50:57 +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:51:00 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 8 August 2016 at 15:42, Ard Biesheuvel wrote= : > 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 handler= s >>> >>> 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 >>> 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 w= riting to the hardware that generates the interrupt (handler) and another i= s concerned with writing to the hardware for the interrupt controller to si= gnal the end of interrupt. So all this is saying is that "the code that to= uches the interrupt controller is implemented in the driver that publishes = this protocol". It does not say how this code is activated, only who is re= sponsible for poking the register. >> >> The historical expectation is that the handler driver calls the EOI inte= rface in the protocol. (If it was the opposite then this interface wouldn'= t even exist since the interrupt controller driver could just do it implici= tly.) 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 825= 9 interrupt protocol (IntelFrameworkPkg\Include\Protocol\Legacy8259.h) that= is quite similar to HwInterrupt. Notice the explicit EndOfInterrupt inter= face here and how it's used by the timer driver at PcAtChipsetPkg\8254Timer= Dxe\Timer.c(88). >> >> Given this I asked that you keep the EndOfInterrupt in the handler drive= r(s) and remove the auto-EOI in the interrupt controller driver, at least f= or 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 :-) > Pushed as 7989300df7