From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM01-SN1-obe.outbound.protection.outlook.com (mail-sn1nam01on0729.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe40::729]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A1D761A1DF8 for ; Wed, 10 Aug 2016 12:34:32 -0700 (PDT) Received: from AT5PR84MB0291.NAMPRD84.PROD.OUTLOOK.COM (10.162.138.25) by AT5PR84MB0289.NAMPRD84.PROD.OUTLOOK.COM (10.162.138.23) 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 19:34:29 +0000 Received: from AT5PR84MB0291.NAMPRD84.PROD.OUTLOOK.COM ([10.162.138.25]) by AT5PR84MB0291.NAMPRD84.PROD.OUTLOOK.COM ([10.162.138.25]) with mapi id 15.01.0549.025; Wed, 10 Aug 2016 19:34:29 +0000 From: "Cohen, Eugene" To: Evan Lloyd , Ard Biesheuvel , Heyi Guo CC: Alexei Fedorov , "edk2-devel@lists.01.org" , "Andrew Fish (afish@apple.com)" , Leif Lindholm Thread-Topic: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt Thread-Index: AQHR77wChUqQlDPPikqPhQBsdmEhTaA+3v8AgAAB14CAAAJkAIAAAQKAgAAA+4CAAABHgIAAAjIAgAAAjACAAAIqAIAAAHqAgAAMAwCAABT+UIAACgwAgAACYYCAA2QngIAAE5Ug Date: Wed, 10 Aug 2016 19:34:29 +0000 Message-ID: References: <20160805165911.14744-1-evan.lloyd@arm.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=eugene@hp.com; x-originating-ip: [15.65.254.4] x-ms-office365-filtering-correlation-id: 33cbd34b-1788-418f-6ce2-08d3c15558e7 x-microsoft-exchange-diagnostics: 1; AT5PR84MB0289; 6:iGLF/aXE79+gM640DFfrGK77zf/NrVGEgdzNCidBdipEz797SDjIpwotNx9PpT1q2BvyJOK56IEO/6AfDc35Y+971CxoamYiCW+Yw93pn7jbmR7GAYIa6fS32gZqfzWmtqjaY4IgoUIVOBeceWBixn2x5FTQtwz6CIWA9c9WcNCjhbEsOd9ot3C1AFOTncYmHfeWQ7FL6yJt3FijPJgLimXvnGZ1YLEissDYW4BM4wlo7aKX77HfmlJns0XurXI4K57t9ddQn2ci6GxlbxQ0dB6Bxvil2ktAhfEcm+Js5I0=; 5:m1gk6KKlPT1g0o/jt63YDSY8iiZyD/tU2AWTvaVe7/Lt0KJ1fCThOs6ErHmfiYzfS+YpD3c7zWJL0m448GjlZOhlE2LTcZjwChSAUWrwkZQtskjuTO2XtcfCcqHkshp8Nk3wx9UZfOvOCYOfBNxAEg==; 24:hdIdnAetZrR4qs+XQzN4h5UJGfKrxfDrqHflxQQv8GjQSF4OQfXMQ3RJvnCnnIV1TJlvST0A+sjZ8yUtsFJrrxWUiRGqWDiQ76vQ46zdrhA=; 7:V7a0PtrEgFtUy2Wnrksp85PxiGRgw2KGCQ18Yst8rCiaQs1u/KT8CBr1hRvkt/4glcF6HPsUeJbANuhVAKmxixTWskzsG0oVXPtJaASzkQKYNH+4i7gGIH5LvD5vcxHWe7rh3O1hpsIsUzlx7nZJgFPPxuvAMp5oxFYS0ds15hEafh+szV2otEmkpR9DRWgCSwn3JiRwFGdOBABXhm79EIaQawJ75tVTADq/SGXKd1YqrRvq6zGTQcELTMT8iuFF x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AT5PR84MB0289; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046); SRVR:AT5PR84MB0289; BCL:0; PCL:0; RULEID:; SRVR:AT5PR84MB0289; x-forefront-prvs: 0030839EEE x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(6009001)(7916002)(6602003)(199003)(189002)(74316002)(86362001)(189998001)(8936002)(8666005)(9686002)(68736007)(106356001)(87936001)(2900100001)(2950100001)(106116001)(10400500002)(586003)(33656002)(81166006)(7846002)(5001770100001)(77096005)(7736002)(7696003)(305945005)(122556002)(81156014)(8676002)(97736004)(101416001)(102836003)(76176999)(6116002)(50986999)(3846002)(54356999)(4326007)(11100500001)(5002640100001)(66066001)(93886004)(92566002)(2906002)(3280700002)(105586002)(99286002)(3660700001)(7059030)(217873001)(19627235001); DIR:OUT; SFP:1102; SCL:1; SRVR:AT5PR84MB0289; H:AT5PR84MB0291.NAMPRD84.PROD.OUTLOOK.COM; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: hp.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: hp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Aug 2016 19:34:29.4942 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: ca7981a2-785a-463d-b82a-3db87dfc3ce6 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AT5PR84MB0289 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 19:34:33 -0000 Content-Language: en-US Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Evan, =20 > I'd like to re-think our GIC EIOR changes in the light of comments from > Heyi 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 no= t > 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 sees 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. According to the GIC architecture spec for level-sensitive interrupts are p= ending only based on the immediate state of the signal, there is no latchin= g, see "Control of the pending status of level-sensitive interrupts" in the= GICv2 Architecture Specification: " For an edge-triggered interrupt, the includes pending status is latched on = either a write to the GICD_ISPENDRn or the assertion of the interrupt signal to the GIC. However, for a level-sens= itive interrupt, the includes pending status either: =95 is latched on a write to the GICD_ISPENDRn =95 follows the state of the interrupt signal to the GIC, without any lat= ching. " So there's no "memory" for level sensitive interrupts and an edge triggered= interrupt will only latch on an edge, not an EOI so I'm not sure there's a= n issue. >>From what I can tell the primary purpose of EOI is for interrupt priority m= anagement - when you issue the EOI a priority drop occurs allowing the next= -highest priority interrupt to be serviced, if any. But since we are not m= aking use of nested interrupts (i.e. IRQ is masked the whole time during in= terrupt processing) I don't think the EOI sequencing matters. This is base= d on about 10 minutes of me reading this GIC spec so please correct any con= fusion I may have. I agree the double-EOI has to go away no matter what. > This is very much at odds with Eugene's position (which, BTW, is not a > stance 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 Acknowledge Register... otherwise the system > behavior is UNPREDICTABLE", so the double write is not good.) I did some detective work and the double-write problem goes way back to the= BeagleBoard/Omap35xx code from 2010 where the NEWIRQAGR register was writt= en multiple times as well. This code actually writes the EOI-like NEWIRQAG= R register two times in addition to the additional EOI interface write: // Needed to prevent infinite nesting when Time Driver lowers TPL MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR); ArmDataSynchronizationBarrier (); InterruptHandler =3D gRegisteredInterruptHandlers[Vector]; if (InterruptHandler !=3D NULL) { // Call the registered interrupt handler. InterruptHandler (Vector, SystemContext); } // Needed to clear after running the handler MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR); ArmDataSynchronizationBarrier (); Note the comment about infinite nesting - I don't know what this is referri= ng to since I would expect TPL to remain at HIGH_LEVEL for the duration of = timer interrupt processing. I'd really like to get Mr. Fish to chime in on= this one. > A more elegant solution is for the GIC register access to be done as > part of 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 interrupts "under the covers". > As an additional benefit, it clearly partitions the peripheral specific > handling from the GIC interface. I have no specific objection to this - let's just find a way to do this tha= t maintains compatibility with the existing HardwareInterrupt protocol defi= nition - maybe return EFI_UNSUPPORTED (or perhaps just EFI_SUCCESS) for the= EOI protocol interface on systems that don't want to expose this functiona= lity. Then an old driver could try to use the EOI interface and on old sys= tems it will issue the EOI and on new systems it will do nothing, deferring= the real EOI to when the ISR returns. Eugene