From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by mx.groups.io with SMTP id smtpd.web10.3302.1570613938236761636 for ; Wed, 09 Oct 2019 02:38:58 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: solarflare.com, ip: 67.231.154.164, mailfrom: tpilar@solarflare.com) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us5.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id AB12C4005C; Wed, 9 Oct 2019 09:38:56 +0000 (UTC) Received: from ukex01.SolarFlarecom.com (10.17.10.4) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 9 Oct 2019 10:38:52 +0100 Received: from ukex01.SolarFlarecom.com ([fe80::35fe:bff1:83ad:65d6]) by ukex01.SolarFlarecom.com ([fe80::35fe:bff1:83ad:65d6%14]) with mapi id 15.00.1395.000; Wed, 9 Oct 2019 10:38:52 +0100 From: "Tomas Pilar (tpilar)" To: "Fu, Siyuan" , Devel EDK2 , "maciej.rabeda@intel.com" CC: "Wu, Jiaxin" Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Thread-Topic: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Thread-Index: AQHVffPE9l6eNQXeGk6gzBeF61zzc6dRgA0AgABwdICAAB3IoQ== Date: Wed, 9 Oct 2019 09:38:51 +0000 Message-ID: <1570613931814.64401@solarflare.com> References: <20191008161629.14668-1-maciej.rabeda@intel.com> <20191008161629.14668-2-maciej.rabeda@intel.com> ,<40FBBD5C52E8B4429773266A45BDCC174C665110@IRSMSX104.ger.corp.intel.com> In-Reply-To: <40FBBD5C52E8B4429773266A45BDCC174C665110@IRSMSX104.ger.corp.intel.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.17.20.51] x-tm-as-product-ver: SMEX-12.5.0.1300-8.5.1010-24964.003 x-tm-as-result: No-19.709400-8.000000-10 x-tmase-matchedrid: jFqw+1pFnMzuo96mfIBuopzEHTUOuMX33dCmvEa6IiGoLZarzrrPmWwk wiJhNkKkNe7yncYhXDw0KPIG0c2cA+WvWGhVgVn0qJSK+HSPY+9wSovBIzQ2lN95RnTaoXbSlaL GcPEXs1AbuyS3yUdFJ5MfUJb9OR2qhrD3pNcSx1Y+NrfDUTEXxMtEPnVvPlFkcBqXYDUNCawhDB 23wWI6FJUJNlnhu9dsYkc1gogFFVRhaj10i6TXQHRSJt6Kzp0Q6qG5M9QNAO1kljwyvGN0By+ry BlDo4Q7yI+Ey9vcbbZycKtPkXGgBxxaKzlXbEWl/ccgt/EtX/3nt7yJaHrgd9EsTITobgNEa7ot zG0Y3BLvsEhC8rfeWg76VdAgkWh6N6IarY2VLsU3X0+M8lqGUhfJTYLG2XFvHDQcqEqNN+moZKy LowV0w5LCcc9+2Btf+zSnDDFnArl1A8pDaZltTlS0U/rncMc42TdMFa1t7LkRHgO3hlQeWWfj+c xYnUGbrdoLblq9S5p1S4DqawMuWnlJc2cM0xFAqjZ865FPtpp8s0cy6t/KSKuPvo9L6iaI8kaKy vurudAoBkamNEKgbuhrAsiaqJtlCQZYBny/onNVXhlmZsTdjKIik2/euMx1s4be7XhW2Z5Qjevv FeK6vVsdGKsEGD800u4+kxCBLdSyQMxm96OyeGlO3nte6oxz8zyVvFxV+QSRH7+lM1L23Xv9jBh UgWuo/qodcjkWARqYHb4xZ59iP58+uv7wB8sT54eqweLWaL5y4VFP6muDhtz0oTaLCDAeeHq7pu iZMhXurXQsL5xNpSSlqWcgaF36yDKxM0ezcw2rm7DrUlmNkF+24nCsUSFN+dkjd91LgACx5amWK 2anSPoLR4+zsDTtEPs/OvZqkzPS3Rxy14J4N3NjCrKaENcCOaE9UqMtHKanfijSNI+kUe8knjbI rfeANkUSDDq742k= x-tm-as-user-approved-sender: Yes x-tm-as-user-blocked-sender: No x-tmase-result: 10--19.709400-8.000000 x-tmase-version: SMEX-12.5.0.1300-8.5.1010-24964.003 MIME-Version: 1.0 X-MDID: 1570613937-5XXRv3umDvhl Content-Language: en-GB Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable I've commented on the bug, SFC adapters will be fine, but I am sure there i= s a vendor out there that will be affected. Bugs due to memory corruption d= uring OS load that depend on the network externalities are notoriously diff= icult to diagnose. We need to make sure this is properly communicated and signposted so other= s can audit their drivers. Cheers, Tom ________________________________________ From: devel@edk2.groups.io on behalf of Rabeda, Mac= iej Sent: 09 October 2019 09:50 To: Fu, Siyuan; Devel EDK2 Cc: Wu, Jiaxin Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBoo= tServices event Hi Siyuan, This change has no effect to Intel Ethernet Server UNDI drivers. They already handle ExitBootServices event and configure the Ethernet adap= ters not to perform any DMA at this point. As per whitepaper (https://firmware.intel.com/sites/default/files/Intel_Wh= itePaper_Using_IOMMU_for_DMA_Protection_in_UEFI.pdf), section Call to Actio= n, UEFI device drivers (including UNDI) should put the controller to halt s= tate and disable bus mastering in ExitBootServices stage - which obliges UN= DI drivers to have ExitBootServices event handlers. To me, this means shutt= ing down DMA on the controllers to which UNDI driver is bound. As for second question, not disabling DMA will make our adapters continue = writing Rx packet data to RAM in DXE Runtime stage. I believe that this behavior is as far from desired as it can. Thanks! Maciej Rabeda -----Original Message----- From: Fu, Siyuan Sent: Wednesday, October 9, 2019 04:08 To: Rabeda, Maciej ; devel@edk2.groups.io Cc: Wu, Jiaxin Subject: RE: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices eve= nt Hi, Maciej Have you tested what will happen if this SNP co-work with those UNDI drive= rs which don't have an ExitBootService event callback to shut down its DMA = activity? And what's the impact to OS if UNDI's DMA is not shut down? Best Regards Siyuan > -----Original Message----- > From: Rabeda, Maciej > Sent: 2019=1B$BG/=1B(B10=1B$B7n=1B(B9=1B$BF|=1B(B 0:16 > To: devel@edk2.groups.io > Cc: Fu, Siyuan ; Wu, Jiaxin > Subject: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices > event > > Patch addresses Bugzilla #1972. > During ExitBootServices stage, drivers should not call any functions > known to use Memory Allocation Services. One of such functions (as per > UEFI spec) is UNDI->Shutdown(). > > Since UNDI drivers during ExitBootServices phase are expected to put > the adapter to such a state that it will not perform any DMA > operations, there is no need to interface UNDI by SNP driver during > that phase. > > Finally, since ExitBootServices event notification function in SNP > only calls UNDI->Shutdown() and Stop() functions, there is no need to > create this event at all. > > Signed-off-by: Maciej Rabeda > Cc: Siyuan Fu > Cc: Jiaxin Wu > --- > NetworkPkg/SnpDxe/Snp.c | 45 -------------------- > NetworkPkg/SnpDxe/Snp.h | 2 - > NetworkPkg/SnpDxe/SnpDxe.inf | 3 -- > 3 files changed, 50 deletions(-) > > diff --git a/NetworkPkg/SnpDxe/Snp.c b/NetworkPkg/SnpDxe/Snp.c index > a23af05078bc..7646a3ce0293 100644 > --- a/NetworkPkg/SnpDxe/Snp.c > +++ b/NetworkPkg/SnpDxe/Snp.c > @@ -8,31 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "Snp.h" > > -/** > - One notified function to stop UNDI device when > gBS->ExitBootServices() called. > - > - @param Event Pointer to this event > - @param Context Event handler private data > - > -**/ > -VOID > -EFIAPI > -SnpNotifyExitBootServices ( > - EFI_EVENT Event, > - VOID *Context > - ) > -{ > - SNP_DRIVER *Snp; > - > - Snp =3D (SNP_DRIVER *)Context; > - > - // > - // Shutdown and stop UNDI driver > - // > - PxeShutdown (Snp); > - PxeStop (Snp); > -} > - > /** > Send command to UNDI. It does nothing currently. > > @@ -647,21 +622,6 @@ SimpleNetworkDriverStart ( > PxeShutdown (Snp); > PxeStop (Snp); > > - // > - // Create EXIT_BOOT_SERIVES Event > - // > - Status =3D gBS->CreateEventEx ( > - EVT_NOTIFY_SIGNAL, > - TPL_NOTIFY, > - SnpNotifyExitBootServices, > - Snp, > - &gEfiEventExitBootServicesGuid, > - &Snp->ExitBootServicesEvent > - ); > - if (EFI_ERROR (Status)) { > - goto Error_DeleteSNP; > - } > - > // > // add SNP to the undi handle > // > @@ -778,11 +738,6 @@ SimpleNetworkDriverStop ( > return Status; > } > > - // > - // Close EXIT_BOOT_SERIVES Event > - // > - gBS->CloseEvent (Snp->ExitBootServicesEvent); > - > Status =3D gBS->CloseProtocol ( > Controller, > &gEfiNetworkInterfaceIdentifierProtocolGuid_31, > diff --git a/NetworkPkg/SnpDxe/Snp.h b/NetworkPkg/SnpDxe/Snp.h index > e6b62930397d..f83a4f075adc 100644 > --- a/NetworkPkg/SnpDxe/Snp.h > +++ b/NetworkPkg/SnpDxe/Snp.h > @@ -120,8 +120,6 @@ typedef struct { > VOID *MapCookie; > } MapList[MAX_MAP_LENGTH]; > > - EFI_EVENT ExitBootServicesEvent; > - > // > // Whether UNDI support reporting media status from GET_STATUS > command, > // i.e. PXE_STATFLAGS_GET_STATUS_NO_MEDIA_SUPPORTED or diff --git > a/NetworkPkg/SnpDxe/SnpDxe.inf b/NetworkPkg/SnpDxe/SnpDxe.inf index > afeb1526cc10..8d045cfcf4ca 100644 > --- a/NetworkPkg/SnpDxe/SnpDxe.inf > +++ b/NetworkPkg/SnpDxe/SnpDxe.inf > @@ -64,9 +64,6 @@ > DebugLib > NetLib > > -[Guids] > - gEfiEventExitBootServicesGuid ## SOMETIMES_CONSUMES #= # > Event > - > [Protocols] > gEfiSimpleNetworkProtocolGuid ## BY_START > gEfiDevicePathProtocolGuid ## TO_START > -- > 2.17.0.windows.1 -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wyd= zial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-3= 16 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresat= a i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej w= iadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakie= kolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the = sole use of the intended recipient(s). If you are not the intended recipien= t, please contact the sender and delete all copies; any review or distribut= ion by others is strictly prohibited.