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 [146.101.78.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 CDC1D1A1DF8 for ; Wed, 10 Aug 2016 13:31:17 -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=cg1KN0vOA8KSxkCNwEoa60t5Op56CqKsXEU26jI+hzA=; b=kLTkACGvIdYwj/FbGJ7mEyJN/89fjKhZIp+Wf2fEU8GtEreR4mCrvbroyFpNpxdgwDdu4Aw8aVfPanfBG5fHfsF/0oj5HmiGG6SpAMC37NyLRUBxWXVOf2Ozq7aqgxrQOY5t61VC/tAWI6WkPDwmfO/fZwHj2CyNOU9//9d+A94= Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01lp0215.outbound.protection.outlook.com [213.199.154.215]) (Using TLS) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-31-4HIRi3O7OBCzCrXQbeVOSQ-1; Wed, 10 Aug 2016 21:31:14 +0100 Received: from AM5PR0801MB1762.eurprd08.prod.outlook.com (10.169.247.16) by AM5PR0801MB1953.eurprd08.prod.outlook.com (10.168.157.149) 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 20:31:10 +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 20:31:10 +0000 From: Evan Lloyd To: "Cohen, Eugene" , 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: AQHR77v+vQhZyb7Ox0Kay7WE3n2Ek6A+3v8AgAAB14CAAAJkAIAAAQKAgAAA+4CAAABHgIAAAjIAgAAAjACAAAIqAIAAAHqAgAAMAwCAABlhAIAABakAgAACYYCAA024QIAANu6AgAAFmoA= Date: Wed, 10 Aug 2016 20:31:10 +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: b0ff3ea8-4d8f-402e-a578-08d3c15d43b7 x-microsoft-exchange-diagnostics: 1; AM5PR0801MB1953; 20:vkkFkQrtl53X6RpGaX5xYafAxq5sSrTwPkyECrEveC52oFdlwtYbY1TReSK2eCGlD9ENg3euh8c5fSOmHXfLvju48M6DcPu1u2C63mhW2bYgWZNB7BGeAhdBsatDERXlDDENcF3LUpIIwbdyYGEu9MhPtYmVquTNYtmT338sE0M= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM5PR0801MB1953; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(162533806227266)(31960201722614)(73583498263828); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040168)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026); SRVR:AM5PR0801MB1953; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0801MB1953; x-forefront-prvs: 0030839EEE x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(40434004)(199003)(13464003)(189002)(8666005)(97736004)(101416001)(105586002)(5001770100001)(77096005)(106116001)(106356001)(2900100001)(2950100001)(74316002)(7696003)(7736002)(305945005)(7846002)(189998001)(76576001)(93886004)(86362001)(50986999)(54356999)(76176999)(5002640100001)(3660700001)(33656002)(3280700002)(19580395003)(122556002)(19580405001)(87936001)(10400500002)(2906002)(11100500001)(5890100001)(586003)(68736007)(3846002)(102836003)(6116002)(8936002)(81166006)(81156014)(8676002)(9686002)(66066001)(4326007)(92566002)(7059030)(217873001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0801MB1953; 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 20:31:10.0659 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1953 X-MC-Unique: 4HIRi3O7OBCzCrXQbeVOSQ-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 20:31:18 -0000 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Hi Eugene. Some responses inline below. >-----Original Message----- >From: Cohen, Eugene [mailto:eugene@hp.com] >Sent: 10 August 2016 20:34 >To: Evan Lloyd; Ard Biesheuvel; Heyi Guo >Cc: Alexei Fedorov; edk2-devel@lists.01.org; Andrew Fish (afish@apple.com)= ; >Leif Lindholm >Subject: RE: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrup= t > >Evan, > >> 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 n= ot >> 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 = pending >only based on the immediate state of the signal, there is no latching, 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-sen= sitive >interrupt, the includes pending status >either: > * is latched on a write to the GICD_ISPENDRn > * follows the state of the interrupt signal to the GIC, without any latc= hing. >" > >So there's no "memory" for level sensitive interrupts and an edge triggere= d >interrupt will only latch on an edge, not an EOI so I'm not sure there's a= n issue. My description was not very clear, and the point is academic if you are hap= py with the solution. However: The GIC spec has a state machine diagram (Figure 4.3), where: Transition D, pending to active and pending This transition occurs on acknowledgement of the interrupt by the PE for le= vel-sensitive SPIs, SGIs, and PPIs. Transition B1 or B2, remove pending state This transition occurs when the interrupt has been deasserted by the periph= eral, if the interrupt is a level-sensitive interrupt, or when software has changed the pending state. Transition E1 or E2, remove active state This transition occurs when software deactivates an interrupt for SPIs, SGI= s, and PPIs. I suspect that because we EOI ("deactivate" for E1/E2) without "deasserting= " the peripheral interrupt then the GIC may restore the pending state (tran= sition E2 instead of B2 then E1), which will look remarkably like a latch. > >>From what I can tell the primary purpose of EOI is for interrupt priority >management - when you issue the EOI a priority drop occurs allowing the ne= xt- >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 interru= pt >processing) I don't think the EOI sequencing matters. This is based on ab= out 10 >minutes of me reading this GIC spec so please correct any confusion 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 th= e >BeagleBoard/Omap35xx code from 2010 where the NEWIRQAGR register was >written multiple times as well. This code actually writes the EOI-like >NEWIRQAGR 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 referr= ing 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 th= at >maintains compatibility with the existing HardwareInterrupt protocol defin= ition >- maybe return EFI_UNSUPPORTED (or perhaps just EFI_SUCCESS) for the EOI >protocol interface on systems that don't want to expose this functionality= . >Then an old driver could try to use the EOI interface and on old systems i= t will >issue the EOI and on new systems it will do nothing, deferring the real EO= I to >when the ISR returns. That looks like a pretty sensible way forward. I'll vote for EFI_SUCCESS, = white lies are sometimes needed. > >Eugene 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.