From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on072f.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe48::72f]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D8A771A1E0F for ; Mon, 8 Aug 2016 06:22:13 -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; Mon, 8 Aug 2016 13:22:10 +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; Mon, 8 Aug 2016 13:22:10 +0000 From: "Cohen, Eugene" To: Ard Biesheuvel , Alexei Fedorov , "Andrew Fish (afish@apple.com)" CC: "edk2-devel@lists.01.org" , Heyi Guo , Leif Lindholm Thread-Topic: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt Thread-Index: AQHR77wChUqQlDPPikqPhQBsdmEhTaA+3v8AgAAB14CAAAJkAIAAAQKAgAAA+4CAAABHgIAAAjIAgAAAjACAAAIqAIAAAHqAgAAMAwCAABT+UA== Date: Mon, 8 Aug 2016 13:22:10 +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: [174.19.99.119] x-ms-office365-filtering-correlation-id: 3ba26c1e-a61d-498d-d22e-08d3bf8f00e1 x-microsoft-exchange-diagnostics: 1; AT5PR84MB0289; 6:HeewfDZ6TJYkEuS8hG2ITTEokkPW6biIaaeX4y11+QK6ZyouWcBkUCVQliYfZhTcoU6ruR4v7m29k6JGG9UEHop2i9gh0751iJyZxRYNwrUeD5ym4AoEXlplU2Ox+eQdXnmeg/Et//W0nO5d5Fn8J5IcMDEba+eAbkCTVtHMCo+QCplsfl8mdFzXeyizQI2Q6sD+LBjcouNSGuls80DXPi1Q0tixfuRflobZrmSg+prOtFK2kuhOS6TqgZt93jCPsHtdR2ysVWvNx21AOAe46YhqxEYWNv23ASFwKOzVOBc=; 5:oZZu1A4YaHEKE76e2sCoki0KL9z9B7fCVNEkom5MNkDzd6EipsnprzHUkaJqS0M/7ATIyRdIZUxLbIg/8gPOsG5d48c8lUDONKqNGiza1gRxDQEzIXw4m7vvoACyW/V1v+JzIxh2cKNtemU6akgS4Q==; 24:comYQJ//VL+Lg+SZjuVGsJ8KTllrp0gViTWiTtMJZY2jdKecNraTDk7TTeq5ZRwyfzzzzIY2ze/SlaGlG3owQ3Hiqw2I8OcLAGIIqDmZvQ0=; 7:/YwtDaa07WAQm5qXtdr4hScLZFKSdtZeH4HmNzemRZy6GQUI7l2aovJMTdQifiYixrlc1rPmMzjK6ZhVEYuUda4FybtIFKw9+DFDpV/DWXYoLewuUlyvaKlxZvghrJO+gLdjqWo+nMlUO2shkfqtCERiQY+uCxRh6aXi/OIDw+VSZ+eOistk2Wz/jCDeFr2rBFHyAOoI5+qDanZElSAiLNFPfDZhtwXKnHqc6FYK8aId4hriMtbY6q+MqBwy+54P x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AT5PR84MB0289; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(162533806227266)(73583498263828); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001); SRVR:AT5PR84MB0289; BCL:0; PCL:0; RULEID:; SRVR:AT5PR84MB0289; x-forefront-prvs: 00286C0CA6 x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(6009001)(7916002)(6602003)(199003)(377454003)(24454002)(13464003)(52044002)(189002)(5002640100001)(6116002)(3846002)(102836003)(2906002)(586003)(189998001)(93886004)(122556002)(11100500001)(2900100001)(87936001)(33656002)(105586002)(66066001)(2950100001)(99286002)(4326007)(3280700002)(81156014)(3660700001)(92566002)(54356999)(50986999)(10400500002)(77096005)(68736007)(74316002)(305945005)(9686002)(8676002)(7696003)(81166006)(575784001)(5001770100001)(7846002)(97736004)(86362001)(7736002)(8666005)(76176999)(15975445007)(19580395003)(19580405001)(106356001)(106116001)(8936002)(101416001)(7059030)(217873001); 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: 08 Aug 2016 13:22:10.3668 (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: Mon, 08 Aug 2016 13:22:14 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Guys, sorry to join so late, something about timezones... Let me try to pr= ovide some context and history. > > it does change the contract we have with registered interrupt handlers >=20 > Looks like it does not: > From edk2\EmbeddedPkg\Include\Protocol\HardwareInterrupt.h: >=20 > " Abstraction for hardware based interrupt routine >=20 > ...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 writ= ing to the hardware that generates the interrupt (handler) and another is c= oncerned with writing to the hardware for the interrupt controller to signa= l the end of interrupt. So all this is saying is that "the code that touch= es the interrupt controller is implemented in the driver that publishes thi= s protocol". It does not say how this code is activated, only who is respo= nsible for poking the register. The historical expectation is that the handler driver calls the EOI interfa= ce in the protocol. (If it was the opposite then this interface wouldn't e= ven exist since the interrupt controller driver could just do it implicitly= .) You're next question will probably be why it was designed this way - fo= r that we'll have to ask Andrew Fish (added). I did a little digging and see that the PC-AT chipset implemented an 8259 i= nterrupt 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 driver(s= ) and remove the auto-EOI in the interrupt controller driver, at least for = cases where a driver handled the interrupt. Feel free to clarify the text in the protocol header to align with this - t= he current wording is not very clear. Thanks, Eugene > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Ard Biesheuvel > Sent: Monday, August 08, 2016 5:51 AM > To: Alexei Fedorov > Cc: edk2-devel@lists.01.org ; Heyi Guo > ; Leif Lindholm ; Cohen, > Eugene > Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interru= pt >=20 > On 8 August 2016 at 13:08, Ard Biesheuvel > wrote: > > On 8 August 2016 at 13:06, Alexei Fedorov > wrote: > >> Timer's pending interrupt is cleared HARDWARE_INTERRUPT_HANDLER > TimerInterruptHandler() in when timer is re-programmed for the next shot= . > > > > Yes, that is the timer side. > > > >> Does it mean that TimerDxe driver is part > EFI_HARDWARE_INTERRUPT_PROTOCOL? > >> > > > > No. The peripheral and the GIC each have their own mask and enable > > registers for the interrupt. That is exactly why the comment describes > > in detail which part is the responsibility of the GIC, and which is > > the responsibility of the peripheral. > > >=20 > ... and actually, looking at TimerInterruptHandler (), I don't see the po= int of > re-enabling the interrupt early, given that >=20 > 308: OriginalTPL =3D gBS->RaiseTPL (TPL_HIGH_LEVEL); >=20 > disables interrupts globally, and only re-enables them on line 346, at wh= ich > point the mTimerNotifyFunction() has already returned. >=20 > So I propose we simply do >=20 > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > index 1169d426b255..f0fcb05757ac 100644 > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > @@ -308,10 +308,7 @@ TimerInterruptHandler ( > OriginalTPL =3D gBS->RaiseTPL (TPL_HIGH_LEVEL); >=20 > // Check if the timer interrupt is active > - if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) > { > - > - // Signal end of interrupt early to help avoid losing subsequent > ticks from long duration handlers > - gInterrupt->EndOfInterrupt (gInterrupt, Source); > + while ((ArmGenericTimerGetTimerCtrlReg () ) & > ARM_ARCH_TIMER_ISTATUS) > + { >=20 > if (mTimerNotifyFunction) { > mTimerNotifyFunction (mTimerPeriod * mElapsedPeriod); >=20 > so that if the condition exists that we know will trigger the interrupt > immediately as soon as we unmask it, we simply enter the loop again just = like > we would when taking the [nested] interrupt. >=20 > @Heyi: any thoughts? >=20 > -- > Ard. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel