From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eu-smtp-delivery-143.mimecast.com (eu-smtp-delivery-143.mimecast.com [207.82.80.143]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4935B1A1E29 for ; Wed, 10 Aug 2016 10:38:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=YCbtYzQXdXEEOMm4KAx2YByvqZsZS9HOqrSsqyJ/Y/s=; b=i5VwqzKpSM1Sp8LEpq5pdjJYTtkzzjXYyDy3iTn5WH7sogpLmDThqJdm7mcgsbFG4uuC+pLMiYct3VEv6NWv6EQF5oxbYRiniKMU05e7H3cVqqJhlPyeE7bh7jAvO7dVH7WwVlOLh4arJwwORfRQGp6hucBneXLuYr5+VGib7os= Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-am5eur03lp0114.outbound.protection.outlook.com [213.199.154.114]) (Using TLS) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-34-XBU5w8vEOxityVrfKhotlw-1; Wed, 10 Aug 2016 18:38:14 +0100 Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com (10.169.247.16) by AM5PR0801MB1956.eurprd08.prod.outlook.com (10.168.158.7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.549.15; Wed, 10 Aug 2016 17:38:11 +0000 Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com ([10.169.247.16]) by AM5PR0801MB1762.eurprd08.prod.outlook.com ([10.169.247.16]) with mapi id 15.01.0549.025; Wed, 10 Aug 2016 17:38:11 +0000 From: Evan Lloyd To: Ard Biesheuvel , "Cohen, Eugene" , Heyi Guo CC: Alexei Fedorov , "edk2-devel@lists.01.org" , Leif Lindholm , "Andrew Fish (afish@apple.com)" , Girish Pathak Thread-Topic: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt Thread-Index: AQHR77v+vQhZyb7Ox0Kay7WE3n2Ek6A+3v8AgAAB14CAAAJkAIAAAQKAgAAA+4CAAABHgIAAAjIAgAAAjACAAAIqAIAAAHqAgAAMAwCAABlhAIAABakAgAACYYCAA024QA== Date: Wed, 10 Aug 2016 17:38:11 +0000 Message-ID: References: <20160805165911.14744-1-evan.lloyd@arm.com> In-Reply-To: Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [217.140.96.140] x-ms-office365-filtering-correlation-id: bf0f6676-8a1c-413c-a0bc-08d3c14519c5 x-microsoft-exchange-diagnostics: 1; AM5PR0801MB1956; 20:4MRql4MK63szNSlcuqMi3oieOHKVEWsXjTBKbH759qODv9794Smi84FV1U739EFfj33lGp69AAOQHJ4hr0cDMUFTi7tevPfel5ifElnUuwbdRfSC+KLt+h/wHnIGzKxKs9nkE3eHukZD5tMOV4T2KcBJdped1HWnWZpVnBhg71Q= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM5PR0801MB1956; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(162533806227266)(1553240931313)(31960201722614)(73583498263828); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040166)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026); SRVR:AM5PR0801MB1956; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0801MB1956; x-forefront-prvs: 0030839EEE x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(199003)(51914003)(189002)(13464003)(52044002)(40434004)(24454002)(74316002)(8676002)(106356001)(586003)(7736002)(7696003)(68736007)(7846002)(86362001)(5890100001)(3280700002)(81166006)(81156014)(93886004)(8666005)(2906002)(33656002)(76576001)(305945005)(97736004)(189998001)(8936002)(5001770100001)(105586002)(3846002)(15975445007)(11100500001)(77096005)(4326007)(2950100001)(50986999)(92566002)(102836003)(54356999)(2900100001)(76176999)(6116002)(10400500002)(5002640100001)(66066001)(87936001)(101416001)(19580405001)(19580395003)(3660700001)(122556002)(9686002)(106116001)(7059030)(217873001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0801MB1956; H:AM5PR0801MB1762.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Aug 2016 17:38:11.5335 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1956 X-MC-Unique: XBU5w8vEOxityVrfKhotlw-1 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: Wed, 10 Aug 2016 17:38:18 -0000 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Hi. (Note - this is not "top-posting", it is a discussion point, with reference= s appended.) I'd like to re-think our GIC EIOR changes in the light of comments from Hey= i Guo. (inserted before Eugene's e-mail below) Despite Eugene's cogent advocacy of the change, and the fact that Alexei's = fix is now accepted, I have come to the conclusion that it is not the best = thing to do. As Heyi points out, it opens a race condition where the Timer= interrupt line is still asserted after the GIC EIOR has been written. The timer IRQ needs to be de-asserted before the EIOR write, or the GIC see= s the IRQ and latches it ("active and pending"). This is clearly an error, and a minor mystery is that we do not see that on= our Juno boards. A more elegant solution is for the GIC register access to be done as part o= f the GIC handling (i.e. revert the GIC code, and remove the EIOR from the = Timer handling.) This also caters for any surreptitious use of other inter= rupts "under the covers". As an additional benefit, it clearly partitions the peripheral specific han= dling from the GIC interface. This is very much at odds with Eugene's position (which, BTW, is not a sta= nce I find comfortable). Further, it implies removing any EIOR writes from extant interrupt handlers= - however, since they already have the "double write" problem, that is not= really a concern. (BTW, the GIC spec is very clear that "A write to this = register must correspond to the most recent valid read from an Interrupt Ac= knowledge Register... otherwise the system behavior is UNPREDICTABLE", so t= he double write is not good.) Does anyone know of a concrete reason why we should not restore the EOI to = the GIC handling and remove it from the timer ISR? Eugene - you express a strongly reasoned case - can I talk you round here? Regards, Evan > Hi Alexei, > > Thanks for your explanation. However, when I tested it on D02, it didn't = act as expected, i.e. we did see a subsequent interrupt triggered >immediat= ely when gBS->RestoreTPL was called in which IRQ is enabled, and I had set = timer interrupt period to some fairly large value (e.g. 5 >seconds). > > So I'd like to confirm, is it declared in GIC specification that clearing= interrupt source will also clear pending status in GICD? > > Thanks and regards. > > Heyi ... > From: Linaro-uefi [mailto:linaro-uefi-bounces@lists.linaro.org] On Behalf= Of Heyi Guo > Sent: 02 August 2016 02:28 > To: Ard Biesheuvel > Cc: Linaro UEFI Mailman List > Subject: Re: [Linaro-uefi] [linaro-uefi] Two write to EOIR in a single in= terrupt > > Hi Leif and Ard, > > There might be another issue in timer interrupt handler. Timer interrupt = seems to be level triggered, so is it OK to write EOIR before clearing the = > > > interrupt source, i.e. updating compare value register? > > Heyi >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard >Biesheuvel >Sent: 08 August 2016 14:51 >To: Cohen, Eugene >Cc: Alexei Fedorov; edk2-devel@lists.01.org; Leif Lindholm; Andrew Fish >(afish@apple.com); Heyi Guo >Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrup= t > >On 8 August 2016 at 15:42, Ard Biesheuvel wrot= e: >> On 8 August 2016 at 15:22, Cohen, Eugene wrote: >>> Guys, sorry to join so late, something about timezones... Let me try t= o >provide some context and history. >>> >>>> > it does change the contract we have with registered interrupt handle= rs >>>> >>>> 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 >>>> responsible >>>> for clearing interrupt sources from individual devices." >>> >>> I think you are reading more deeply into this verbiage than was intende= d. >>From a separation-of-concerns perspective one driver is concerned with wri= ting >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 touche= s the >interrupt controller is implemented in the driver that publishes this prot= ocol". It >does not say how this code is activated, only who is responsible for pokin= g the >register. >>> >>> The historical expectation is that the handler driver calls the EOI int= erface in >the protocol. (If it was the opposite then this interface wouldn't even e= xist >since the interrupt controller driver could just do it implicitly.) You'r= e next >question will probably be why it was designed this way - for that we'll ha= ve to >ask Andrew Fish (added). >>> >>> I did a little digging and see that the PC-AT chipset implemented an 82= 59 >interrupt protocol (IntelFrameworkPkg\Include\Protocol\Legacy8259.h) that = is >quite similar to HwInterrupt. Notice the explicit EndOfInterrupt interfac= e here >and how it's used by the timer driver at >PcAtChipsetPkg\8254TimerDxe\Timer.c(88). >>> >>> Given this I asked that you keep the EndOfInterrupt in the handler driv= er(s) >and remove the auto-EOI in the interrupt controller driver, at least for c= ases >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 >_______________________________________________ >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 confid= ential and may also be privileged. If you are not the intended recipient, p= lease 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.