From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.17901.1586967915490300339 for ; Wed, 15 Apr 2020 09:25:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=CDjBLYTA; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586967914; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FWTAy8zUCAF9Q+Hyc6TVf/RzpsUspJnlwNYcclgXOLI=; b=CDjBLYTAWN58txIyRZ1mBCme4SswR+k14SHAElthR88O81oEoI1joxRWLX5u4L5+s77nQm 6Qzsefy68NcOiNBXr5g9IbCQqCx1A2qo50ANXazjM1yS2HebTeIsdhzkZ3X+TcMmRrlW/z WWlnSZdmqTbQ4aaXFU4TXxQ/K7hYiH4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-508-K7-6TaX8P9m4xgtTeEhOoQ-1; Wed, 15 Apr 2020 12:25:04 -0400 X-MC-Unique: K7-6TaX8P9m4xgtTeEhOoQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BC1EB8018A4; Wed, 15 Apr 2020 16:25:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-155.ams2.redhat.com [10.36.112.155]) by smtp.corp.redhat.com (Postfix) with ESMTP id EF146118DE8; Wed, 15 Apr 2020 16:25:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK To: "Rabeda, Maciej" , devel@edk2.groups.io, michael.kubacki@outlook.com Cc: Siyuan Fu , Jiaxin Wu , "Tomas Pilar (tpilar)" References: From: "Laszlo Ersek" Message-ID: Date: Wed, 15 Apr 2020 18:25:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 :) 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). Further, my understanding is that SnpDxe wraps the UNDI interfaces (via consuming the NetworkInterfaceIdentifier protocol), and exposes a SimpleNetwork protocol interface on top. 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. 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. Please refer to appendix "E.4.7 Initialize". It says: 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. 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. 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. Accordingly, considering a complete DisconnectController / DriverBindingStop procedure on a UEFI system, (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, (b) the memory no longer referenced by UNDI would have to be released with the appropriate boot service. And so, in an EBS handler, step (a) must be done, and (b) must not. 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). (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.) 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. ... 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. 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. I suggest we preserve the EBS handler that is in SnpDxe, and that we keep the PCD too (with its current default TRUE value). 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. 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). Thanks, Laszlo >=20 > On 14-Apr-20 19:08, Michael Kubacki wrote: >> Hi Maciej, >> >> Thank you for summarizing the background. >> >> 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-stable20200= 5. >> >> Thanks, >> Michael >> >> On 4/14/2020 2:59 AM, Rabeda, Maciej wrote: >>> Hi Michael, >>> >>> 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/61bb6eeb4d93c0a34c1995d87914a= b41398f9550 >>> >>> Patch: https://edk2.groups.io/g/devel/message/48899 >>> >>> 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." >>> >>> UEFI spec Appendix E states for UNDI->Stop(): >>> "The memory buffer assigned in the Initialize command can be released >>> or reassigned." >>> >>> 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-whi= tepaper-using-iommu-for-dma-protection-in-uefi.pdf >>> >>> 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. >>> >>> Based on the information above, I was willing to remove the EBS event >>> creation altogether from SnpDxe due to misalignment with the UEFI spec= . >>> The only argument for hesitance was potential backward compatibility >>> with older UNDI drivers, hence the event creation control via PCD. >>> >>> 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. >>> >>> Thanks, >>> Maciej >>> >>> On 09-Apr-20 20:16, michael.kubacki@outlook.com wrote: >>>> From: Michael Kubacki >>>> >>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1562 >>>> >>>> The current SnpDxe implementation registers its ExitBootServices even= t >>>> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. >>>> This >>>> function calls PxeShutdown() which issues an UNDI=C2=A0 shutdown oper= ation. >>>> Ultimately, this may invoke Shutdown() in EFI_SIMPLE_NETWORK_PROTOCOL= . >>>> >>>> The UEFI specification 2.8A Table 27 "TPL Restrictions" restricts >>>> the TPL >>>> for Simple Network Protocol to <=3D TPL_CALLBACK. In addition, it has >>>> been >>>> observed in some 3rd party UNDI drivers to cause an issue further dow= n >>>> the call stack if the TPL is higher than TPL_CALLBACK on invocation. >>>> >>>> Therefore, this commit changes the TPL of >>>> SnpNotifyExitBootServices() to >>>> TPL_CALLBACK. >>>> >>>> Cc: Siyuan Fu >>>> Cc: Maciej Rabeda >>>> Cc: Jiaxin Wu >>>> Signed-off-by: Michael Kubacki >>>> --- >>>> =C2=A0 NetworkPkg/SnpDxe/Snp.c | 3 ++- >>>> =C2=A0 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> 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 @@ >>>> =C2=A0=C2=A0=C2=A0 Implementation of driver entry point and driver bi= nding protocol. >>>> =C2=A0 =C2=A0 Copyright (c) 2004 - 2019, Intel Corporation. All right= s >>>> reserved.
>>>> +Copyright (c) Microsoft Corporation.
>>>> =C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent >>>> =C2=A0 =C2=A0 **/ >>>> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status =3D gBS->CreateEventEx ( >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= = =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EVT_NOTIFY_SIGNA= L, >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TPL_NOTIFY, >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TPL_CALLBACK, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= = =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SnpNotifyExitBoo= tServices, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= = =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Snp, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= = =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &gEfiEventExitBo= otServicesGuid, >>> >> >>=20 >> >=20