From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by mx.groups.io with SMTP id smtpd.web12.10057.1587053540433050744 for ; Thu, 16 Apr 2020 09:12:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=dSt+VWNH; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id 03GGBx6G012734; Thu, 16 Apr 2020 09:12:19 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=content-type : mime-version : subject : from : in-reply-to : date : cc : content-transfer-encoding : message-id : references : to; s=20180706; bh=MzB894XMUYtxrQN1Hgd52DPdOmzvalDMRt3WMkZEO08=; b=dSt+VWNHYZz2OPPCHB3d/cIYMY6P5ya60jD8/yBgMoO7b6IBwIVqETJyX8zjBnkM0ayX 97aDJVRIn9LtCINaOcVF1DpLn1/h/9YD2qMyzN8a6MJk0kjNAmVXuuqVz09UH5neLk23 0hx5BteTOT3XFubH9JyhhhHdQSNUlFF/nendB9nDR8ltD4VlxVH3YqN69pUNCRc0UhMC HMZBQt4Sw5O+sCVAAL8omFJ/jtB+4M6ny9eeRyBvnqm6IYoLWl3naQOv5Xl3QItqBHYc ds0QK3R2xpVJDttB7LRf9mn3mzUSUcOuV9nX+llfVJ70b4cCfiwOohzeE3MYz9hRy/Cw Hw== Received: from rn-mailsvcp-mta-lapp02.rno.apple.com (rn-mailsvcp-mta-lapp02.rno.apple.com [10.225.203.150]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 30dn6ep6ep-11 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 16 Apr 2020 09:12:18 -0700 Received: from rn-mailsvcp-mmp-lapp02.rno.apple.com (rn-mailsvcp-mmp-lapp02.rno.apple.com [17.179.253.15]) by rn-mailsvcp-mta-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPS id <0Q8W00C7Y2CIBWG0@rn-mailsvcp-mta-lapp02.rno.apple.com>; Thu, 16 Apr 2020 09:12:18 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp02.rno.apple.com by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) id <0Q8W0010026AHZ00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Thu, 16 Apr 2020 09:12:18 -0700 (PDT) X-Va-A: X-Va-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-Va-E-CD: 15e637de245e7f26fbd66b2248ff2a9c X-Va-R-CD: b75e73d30b9f2d6730a7b8478f20e2d1 X-Va-CD: 0 X-Va-ID: 19e6d3a1-b811-47c9-8e9c-c2c77b9e7f0d X-V-A: X-V-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-V-E-CD: 15e637de245e7f26fbd66b2248ff2a9c X-V-R-CD: b75e73d30b9f2d6730a7b8478f20e2d1 X-V-CD: 0 X-V-ID: 0803e01b-59bb-4197-ab44-dd4792098cf7 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.676 definitions=2020-04-16_06:2020-04-14,2020-04-16 signatures=0 Received: from [17.235.17.34] (unknown [17.235.17.34]) by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPSA id <0Q8W00CPQ2CG5T00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Thu, 16 Apr 2020 09:12:18 -0700 (PDT) MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK From: "Andrew Fish" In-reply-to: Date: Thu, 16 Apr 2020 09:12:16 -0700 Cc: "Rabeda, Maciej" , michael.kubacki@outlook.com, Siyuan Fu , Jiaxin Wu , "Tomas Pilar (tpilar)" Message-id: <0CFA9104-ADB5-40F9-B564-A15D76FB58FC@apple.com> References: To: devel@edk2.groups.io, lersek@redhat.com X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.676 definitions=2020-04-16_06:2020-04-14,2020-04-16 signatures=0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: quoted-printable > On Apr 15, 2020, at 9:25 AM, Laszlo Ersek wrote: >=20 > On 04/15/20 11:45, Rabeda, Maciej wrote: >> Siyuan, Jiaxin, Laszlo, >>=20 >> It would be great to hear especially your opinion on the matter before >> decisions take place :) >=20 > My understanding is that UNDI is yet another (historica?l) NIC interface > specification. I've failed to find a download location for this > specification, therefore I don't know what functionality it offers. But > Appendix E of the UEFI spec seems like a good substitute (or maybe the > latter is the authoritative specification for UNDI at this time -- I'm > not sure). >=20 Laszlo, This is "way back" but UNDI was defined in the Preboot Execution Environme= nt (PXE) Specification. I think version 2.1 was the last one in and it was = released in 1999. The how to do it in EFI ended up in the EFI spec.=20 PXE was a PC BIOS specification for network booting, UNDI (Universal Netwo= rk Device Interface) was a standardized abstraction for NIC hardware that g= oes back to PC BIOS The cross link for specs referenced in the UEFI Spec is here: https://uefi= .org/uefi. The idea is to keep that up do date.=20 Thanks, Andrew Fish > Further, my understanding is that SnpDxe wraps the UNDI interfaces (via > consuming the NetworkInterfaceIdentifier protocol), and exposes a > SimpleNetwork protocol interface on top. >=20 > UNDI seems like a more primitive interface than UEFI. I believe UNDI is > available on some traditional BIOS computers too. Therefore expecting > the agent (whatever agent!) that *provides* (implements) the UNDI > interfaces, to create their own "EBS handler", is wrong, in my opinion. > For an agent that offers a UNDI interface, the "UEFI EBS handler" > concept is simply not defined -- because such an agent (i.e. one that > offers UNDI) is not tied to UEFI in any way. >=20 > I agree that, on a UEFI system, the UNDI-offering "agent" must be *made* > abort pending DMA transactions, at ExitBootServices(). But, because UNDI > per se is independent of -- unaware of! -- UEFI, UNDI cannot have any > idea of the "UEFI memory map". Therefore, I don't see how *just* writing > the shutdown command (per appendix E.4.9, also per bug 1974 comment 0) > via the !PXE structure (regardless of software UNDI vs. hardware UNDI) > could *ever* modify the UEFI memory map. >=20 > Please refer to appendix "E.4.7 Initialize". It says: >=20 > Once the memory requirements of the UNDI are obtained by using the Get > Init Info command, a block of kernel (nonswappable) memory may need to > be allocated by the protocol driver. The address of this kernel memory > must be passed to UNDI using the Initialize command CPB. This memory > is used for transmit and receive buffers and internal processing. >=20 > Which means (supporting my above statements about Shutdown) that UNDI > *itself* cannot allocate memory -- it doesn't know *how*. Only the > module consuming (or wrapping) UNDI can allocate memory *for* UNDI, and > pass the address to UNDI via the Initialize command. >=20 > Where "Appendix E.4.9 Shutdown" says, "the memory buffer assigned in the > Initialize command can be released or reassigned", that is a concession > made towards the *sender* of the Shutdown CDB. It means that, now that > the UNDI-providing agent has forgotten the memory originally assigned to > it with the Initialize command, the *caller* of UNDI is at liberty to > repurpose that memory. >=20 > Accordingly, considering a complete DisconnectController / > DriverBindingStop procedure on a UEFI system, >=20 > (a) UNDI would have to be sent a Shutdown command, which would abort > pending DMA and make UNDI forget about the Initialize-originated memory > buffer, >=20 > (b) the memory no longer referenced by UNDI would have to be released > with the appropriate boot service. >=20 > And so, in an EBS handler, step (a) must be done, and (b) must not. >=20 > Given that SnpDxe is the driver that wraps UNDI for UEFI (sends CDBs), > in my opinion it is the SnpDxe driver that needs to install the EBS > handler. This handler needs to send the Shutdown CDB, and this handler > must not release memory (=3D it must not change the UEFI memmap). >=20 > (Again: the UNDI-providing agent that *receives* the Shutdown CDB at EBS > is *unable* to change the UEFI memory map. The reason is (again) that > the UNDI-providing agent simply doesn't *know* that it is running on a > UEFI system or not, therefore it cannot call UEFI boot services, or even > define the "UEFI memmap" concept.) >=20 > In other words, I personally believe that bug#1974 should have been > closed as INVALID (without patching the edk2 source). The EBS handler in > SnpDxe is necessary (as long as it does not alter the UEFI memory map > itself), because sending the Shutdown CDB at EBS *is* needed, and the > UNDI-providing agent that processes the Shutdown CDB *cannot* release > UEFI-owned memory. >=20 > ... Now, I do realize there *could* be UNDI implementations -- > *software* UNDI ones -- that are actually UEFI modules themselves, > providing the software UNDI interface, and also installing the > NetworkInterfaceIdentifier protocol. These drivers *could* in theory > alter the UEFI memory map, upon receiving the Shutdown CDB. >=20 > In my opinion, that would definitely be a bug in *those* drivers. (It is > a layering violation. Whether a UNDI interface is software vs. hardware > is inconsequential regarding memory life-cycle management!) In that > case, setting PcdSnpCreateExitBootServicesEvent to FALSE could be > necessary for *working around* the bug in said software UNDI-providing > agent. >=20 > I suggest we preserve the EBS handler that is in SnpDxe, and that we > keep the PCD too (with its current default TRUE value). >=20 >=20 > Regarding the TPL, I go with TPL_CALLBACK by default, *by principle*. I > only condone TPL_NOTIFY if TPL_CALLBACK can be shown to be wrong or > insufficient. >=20 > I have no idea why the EBS handler in question was enqueued at > TPL_NOTIFY in the first place, in historical -- 11 years old -- commit > 0428a6cb12ec ("fixed DMA not be stopped issue when gBS->ExitBootServices > called.", 2009-04-10). >=20 > Thanks, > Laszlo >=20 >>=20 >> On 14-Apr-20 19:08, Michael Kubacki wrote: >>> Hi Maciej, >>>=20 >>> Thank you for summarizing the background. >>>=20 >>> I would like to get others feedback as well. If the EBS notification >>> is kept, I'd like to request this patch be included in edk2-stable2020= 05. >>>=20 >>> Thanks, >>> Michael >>>=20 >>> On 4/14/2020 2:59 AM, Rabeda, Maciej wrote: >>>> Hi Michael, >>>>=20 >>>> Some time ago we have introduced a patch in ExitBootServices (EBS) >>>> area for SnpDxe to allow for EBS event creation control. >>>> Commit: >>>> https://github.com/tianocore/edk2/commit/61bb6eeb4d93c0a34c1995d87914= ab41398f9550 >>>>=20 >>>> Patch: https://edk2.groups.io/g/devel/message/48899 >>>>=20 >>>> Ideally, at EBS stage, SNP should not interface UNDI at all. >>>> UEFI spec for EVT_SIGNAL_EXIT_BOOT_SERVICES events (should apply to >>>> event tied to EBS GUID): >>>> "The notification function for this event is not allowed to >>>> use the Memory Allocation Services, or call any functions that use >>>> the Memory Allocation Services and must only call functions that are >>>> known not to use Memory Allocation Services, because these >>>> services modify the current memory map." >>>>=20 >>>> UEFI spec Appendix E states for UNDI->Stop(): >>>> "The memory buffer assigned in the Initialize command can be released >>>> or reassigned." >>>>=20 >>>> Furthermore, all UEFI drivers controlling PCI devices are obliged to >>>> shut down all DMA activity. >>>> "Call to Action" section in: >>>> https://software.intel.com/sites/default/files/managed/8d/88/intel-wh= itepaper-using-iommu-for-dma-protection-in-uefi.pdf >>>>=20 >>>> Quote: "The UEFI device driver should disable bus master and put >>>> controller to halt state in ExitBootServices." >>>> That would require UNDI drivers to create their own EBS event and >>>> shutdown their adapters. >>>>=20 >>>> Based on the information above, I was willing to remove the EBS event >>>> creation altogether from SnpDxe due to misalignment with the UEFI spe= c. >>>> The only argument for hesitance was potential backward compatibility >>>> with older UNDI drivers, hence the event creation control via PCD. >>>>=20 >>>> If we are observing issues on SNP<->UNDI line in EBS stage, I think >>>> the subject is worth revisiting. >>>> I would love to get any and all community input on that matter. >>>>=20 >>>> Thanks, >>>> Maciej >>>>=20 >>>> On 09-Apr-20 20:16, michael.kubacki@outlook.com wrote: >>>>> From: Michael Kubacki >>>>>=20 >>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1562 >>>>>=20 >>>>> The current SnpDxe implementation registers its ExitBootServices eve= nt >>>>> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. >>>>> This >>>>> function calls PxeShutdown() which issues an UNDI shutdown operatio= n. >>>>> Ultimately, this may invoke Shutdown() in EFI_SIMPLE_NETWORK_PROTOCO= L. >>>>>=20 >>>>> The UEFI specification 2.8A Table 27 "TPL Restrictions" restricts >>>>> the TPL >>>>> for Simple Network Protocol to <=3D TPL_CALLBACK. In addition, it ha= s >>>>> been >>>>> observed in some 3rd party UNDI drivers to cause an issue further do= wn >>>>> the call stack if the TPL is higher than TPL_CALLBACK on invocation. >>>>>=20 >>>>> Therefore, this commit changes the TPL of >>>>> SnpNotifyExitBootServices() to >>>>> TPL_CALLBACK. >>>>>=20 >>>>> Cc: Siyuan Fu >>>>> Cc: Maciej Rabeda >>>>> Cc: Jiaxin Wu >>>>> Signed-off-by: Michael Kubacki >>>>> --- >>>>> NetworkPkg/SnpDxe/Snp.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>=20 >>>>> diff --git a/NetworkPkg/SnpDxe/Snp.c b/NetworkPkg/SnpDxe/Snp.c >>>>> index 078b27cf5edd..fe022e16eacc 100644 >>>>> --- a/NetworkPkg/SnpDxe/Snp.c >>>>> +++ b/NetworkPkg/SnpDxe/Snp.c >>>>> @@ -2,6 +2,7 @@ >>>>> Implementation of driver entry point and driver binding protocol= . >>>>> Copyright (c) 2004 - 2019, Intel Corporation. All rights >>>>> reserved.
>>>>> +Copyright (c) Microsoft Corporation.
>>>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>>>> **/ >>>>> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >>>>> // >>>>> Status =3D gBS->CreateEventEx ( >>>>> EVT_NOTIFY_SIGNAL, >>>>> - TPL_NOTIFY, >>>>> + TPL_CALLBACK, >>>>> SnpNotifyExitBootServices, >>>>> Snp, >>>>> &gEfiEventExitBootServicesGuid, >>>>=20 >>>=20 >>>=20 >>>=20 >>=20 >=20 >=20 >=20 >=20