* [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK @ 2020-04-09 18:16 Michael Kubacki 2020-04-14 9:59 ` Maciej Rabeda 0 siblings, 1 reply; 9+ messages in thread From: Michael Kubacki @ 2020-04-09 18:16 UTC (permalink / raw) To: devel; +Cc: Siyuan Fu, Maciej Rabeda, Jiaxin Wu From: Michael Kubacki <michael.kubacki@microsoft.com> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 The current SnpDxe implementation registers its ExitBootServices event notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. This function calls PxeShutdown() which issues an UNDI shutdown operation. 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 <= TPL_CALLBACK. In addition, it has been observed in some 3rd party UNDI drivers to cause an issue further down 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 <siyuan.fu@intel.com> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> --- NetworkPkg/SnpDxe/Snp.c | 3 ++- 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 @@ Implementation of driver entry point and driver binding protocol. Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR> +Copyright (c) Microsoft Corporation.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( // Status = gBS->CreateEventEx ( EVT_NOTIFY_SIGNAL, - TPL_NOTIFY, + TPL_CALLBACK, SnpNotifyExitBootServices, Snp, &gEfiEventExitBootServicesGuid, -- 2.16.3.windows.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-09 18:16 [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK Michael Kubacki @ 2020-04-14 9:59 ` Maciej Rabeda 2020-04-14 17:08 ` Michael Kubacki 0 siblings, 1 reply; 9+ messages in thread From: Maciej Rabeda @ 2020-04-14 9:59 UTC (permalink / raw) To: michael.kubacki, devel; +Cc: Siyuan Fu, Jiaxin Wu [-- Attachment #1: Type: text/plain, Size: 3731 bytes --] 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/61bb6eeb4d93c0a34c1995d87914ab41398f9550 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-whitepaper-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 <michael.kubacki@microsoft.com> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 > > The current SnpDxe implementation registers its ExitBootServices event > notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. This > function calls PxeShutdown() which issues an UNDI shutdown operation. > 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 <= TPL_CALLBACK. In addition, it has been > observed in some 3rd party UNDI drivers to cause an issue further down > 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 <siyuan.fu@intel.com> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com> > --- > NetworkPkg/SnpDxe/Snp.c | 3 ++- > 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 @@ > Implementation of driver entry point and driver binding protocol. > > Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR> > +Copyright (c) Microsoft Corporation.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( > // > Status = gBS->CreateEventEx ( > EVT_NOTIFY_SIGNAL, > - TPL_NOTIFY, > + TPL_CALLBACK, > SnpNotifyExitBootServices, > Snp, > &gEfiEventExitBootServicesGuid, [-- Attachment #2: Type: text/html, Size: 5284 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-14 9:59 ` Maciej Rabeda @ 2020-04-14 17:08 ` Michael Kubacki 2020-04-15 9:45 ` [edk2-devel] " Maciej Rabeda 0 siblings, 1 reply; 9+ messages in thread From: Michael Kubacki @ 2020-04-14 17:08 UTC (permalink / raw) To: Rabeda, Maciej, devel; +Cc: Siyuan Fu, Jiaxin Wu 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-stable202005. 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/61bb6eeb4d93c0a34c1995d87914ab41398f9550 > 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-whitepaper-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<michael.kubacki@microsoft.com> >> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 >> >> The current SnpDxe implementation registers its ExitBootServices event >> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. This >> function calls PxeShutdown() which issues an UNDI shutdown operation. >> 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 <= TPL_CALLBACK. In addition, it has been >> observed in some 3rd party UNDI drivers to cause an issue further down >> 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<siyuan.fu@intel.com> >> Cc: Maciej Rabeda<maciej.rabeda@linux.intel.com> >> Cc: Jiaxin Wu<jiaxin.wu@intel.com> >> Signed-off-by: Michael Kubacki<michael.kubacki@microsoft.com> >> --- >> NetworkPkg/SnpDxe/Snp.c | 3 ++- >> 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 @@ >> Implementation of driver entry point and driver binding protocol. >> >> Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR> >> +Copyright (c) Microsoft Corporation.<BR> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >> // >> Status = gBS->CreateEventEx ( >> EVT_NOTIFY_SIGNAL, >> - TPL_NOTIFY, >> + TPL_CALLBACK, >> SnpNotifyExitBootServices, >> Snp, >> &gEfiEventExitBootServicesGuid, > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-14 17:08 ` Michael Kubacki @ 2020-04-15 9:45 ` Maciej Rabeda 2020-04-15 16:25 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Maciej Rabeda @ 2020-04-15 9:45 UTC (permalink / raw) To: devel, michael.kubacki; +Cc: Siyuan Fu, Jiaxin Wu, Laszlo Ersek Siyuan, Jiaxin, Laszlo, It would be great to hear especially your opinion on the matter before decisions take place :) 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-stable202005. > > 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/61bb6eeb4d93c0a34c1995d87914ab41398f9550 >> 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-whitepaper-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<michael.kubacki@microsoft.com> >>> >>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 >>> >>> The current SnpDxe implementation registers its ExitBootServices event >>> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. >>> This >>> function calls PxeShutdown() which issues an UNDI shutdown operation. >>> 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 <= TPL_CALLBACK. In addition, it has >>> been >>> observed in some 3rd party UNDI drivers to cause an issue further down >>> 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<siyuan.fu@intel.com> >>> Cc: Maciej Rabeda<maciej.rabeda@linux.intel.com> >>> Cc: Jiaxin Wu<jiaxin.wu@intel.com> >>> Signed-off-by: Michael Kubacki<michael.kubacki@microsoft.com> >>> --- >>> NetworkPkg/SnpDxe/Snp.c | 3 ++- >>> 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 @@ >>> Implementation of driver entry point and driver binding protocol. >>> Copyright (c) 2004 - 2019, Intel Corporation. All rights >>> reserved.<BR> >>> +Copyright (c) Microsoft Corporation.<BR> >>> SPDX-License-Identifier: BSD-2-Clause-Patent >>> **/ >>> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >>> // >>> Status = gBS->CreateEventEx ( >>> EVT_NOTIFY_SIGNAL, >>> - TPL_NOTIFY, >>> + TPL_CALLBACK, >>> SnpNotifyExitBootServices, >>> Snp, >>> &gEfiEventExitBootServicesGuid, >> > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-15 9:45 ` [edk2-devel] " Maciej Rabeda @ 2020-04-15 16:25 ` Laszlo Ersek 2020-04-15 16:40 ` Laszlo Ersek 2020-04-16 16:12 ` Andrew Fish 0 siblings, 2 replies; 9+ messages in thread From: Laszlo Ersek @ 2020-04-15 16:25 UTC (permalink / raw) To: Rabeda, Maciej, devel, michael.kubacki Cc: Siyuan Fu, Jiaxin Wu, Tomas Pilar (tpilar) On 04/15/20 11:45, Rabeda, Maciej wrote: > Siyuan, Jiaxin, Laszlo, > > 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 (= 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 > > 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-stable202005. >> >> 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/61bb6eeb4d93c0a34c1995d87914ab41398f9550 >>> >>> 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-whitepaper-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<michael.kubacki@microsoft.com> >>>> >>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 >>>> >>>> The current SnpDxe implementation registers its ExitBootServices event >>>> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. >>>> This >>>> function calls PxeShutdown() which issues an UNDI shutdown operation. >>>> 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 <= TPL_CALLBACK. In addition, it has >>>> been >>>> observed in some 3rd party UNDI drivers to cause an issue further down >>>> 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<siyuan.fu@intel.com> >>>> Cc: Maciej Rabeda<maciej.rabeda@linux.intel.com> >>>> Cc: Jiaxin Wu<jiaxin.wu@intel.com> >>>> Signed-off-by: Michael Kubacki<michael.kubacki@microsoft.com> >>>> --- >>>> NetworkPkg/SnpDxe/Snp.c | 3 ++- >>>> 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 @@ >>>> Implementation of driver entry point and driver binding protocol. >>>> Copyright (c) 2004 - 2019, Intel Corporation. All rights >>>> reserved.<BR> >>>> +Copyright (c) Microsoft Corporation.<BR> >>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>>> **/ >>>> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >>>> // >>>> Status = gBS->CreateEventEx ( >>>> EVT_NOTIFY_SIGNAL, >>>> - TPL_NOTIFY, >>>> + TPL_CALLBACK, >>>> SnpNotifyExitBootServices, >>>> Snp, >>>> &gEfiEventExitBootServicesGuid, >>> >> >> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-15 16:25 ` Laszlo Ersek @ 2020-04-15 16:40 ` Laszlo Ersek 2020-04-16 15:25 ` Maciej Rabeda 2020-04-16 16:12 ` Andrew Fish 1 sibling, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2020-04-15 16:40 UTC (permalink / raw) To: Rabeda, Maciej, devel, michael.kubacki Cc: Siyuan Fu, Jiaxin Wu, Tomas Pilar (tpilar) On 04/15/20 18:25, Laszlo Ersek wrote: > 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. For the patch that's being proposed in this thread: Reviewed-by: Laszlo Ersek <lersek@redhat.com> However, there *is* a bug in SnpNotifyExitBootServices(). Namely, it calls PxeShutdown(), and PxeShutdown() does two things: (a) it sends the Shutdown CDB, (b) it calls Snp->PciIo->FreeBuffer(). Step (a) is required in the EBS handler context. Step (b) is *forbidden* in the EBS handler context. Therefore, the cut must be made *between* steps (a) and (b). PxeShutdown() is alright to call from SimpleNetworkDriverStop(), but it is *not* fit for an EBS handler. (SnpNotifyExitBootServices() also calls PxeStop(). I'm not sure if that is really necessary, but at least PxeStop() only sends a CDB to UNDI, and manipulates "Snp->Mode.State". Those actions do not interfere with the UEFI memmap, so they are harmless in this aspect.) "in my opinion", as always :) Thanks, Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-15 16:40 ` Laszlo Ersek @ 2020-04-16 15:25 ` Maciej Rabeda 0 siblings, 0 replies; 9+ messages in thread From: Maciej Rabeda @ 2020-04-16 15:25 UTC (permalink / raw) To: Laszlo Ersek, devel, michael.kubacki Cc: Siyuan Fu, Jiaxin Wu, Tomas Pilar (tpilar) Laszlo, Thanks for the very thorough analysis - this is why I have called you out specifically :) I will file a bug against SnpDxe EBS for bookkeeping. Might end up as "won't fix" or "invalid", but a bit more time has to be spent to address it correctly. Michael, For your patch. Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> Thanks, Maciej On 15-Apr-20 18:40, Laszlo Ersek wrote: > On 04/15/20 18:25, Laszlo Ersek wrote: > >> 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. > For the patch that's being proposed in this thread: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > However, there *is* a bug in SnpNotifyExitBootServices(). Namely, it > calls PxeShutdown(), and PxeShutdown() does two things: > > (a) it sends the Shutdown CDB, > (b) it calls Snp->PciIo->FreeBuffer(). > > Step (a) is required in the EBS handler context. > > Step (b) is *forbidden* in the EBS handler context. > > Therefore, the cut must be made *between* steps (a) and (b). > > PxeShutdown() is alright to call from SimpleNetworkDriverStop(), but it > is *not* fit for an EBS handler. > > (SnpNotifyExitBootServices() also calls PxeStop(). I'm not sure if that > is really necessary, but at least PxeStop() only sends a CDB to UNDI, > and manipulates "Snp->Mode.State". Those actions do not interfere with > the UEFI memmap, so they are harmless in this aspect.) > > "in my opinion", as always :) > > Thanks, > Laszlo > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-15 16:25 ` Laszlo Ersek 2020-04-15 16:40 ` Laszlo Ersek @ 2020-04-16 16:12 ` Andrew Fish 2020-04-20 9:25 ` Laszlo Ersek 1 sibling, 1 reply; 9+ messages in thread From: Andrew Fish @ 2020-04-16 16:12 UTC (permalink / raw) To: devel, lersek Cc: Rabeda, Maciej, michael.kubacki, Siyuan Fu, Jiaxin Wu, Tomas Pilar (tpilar) > On Apr 15, 2020, at 9:25 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 04/15/20 11:45, Rabeda, Maciej wrote: >> Siyuan, Jiaxin, Laszlo, >> >> 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). > Laszlo, This is "way back" but UNDI was defined in the Preboot Execution Environment (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. PXE was a PC BIOS specification for network booting, UNDI (Universal Network Device Interface) was a standardized abstraction for NIC hardware that goes 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. 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. > > 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 (= 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 > >> >> 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-stable202005. >>> >>> 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/61bb6eeb4d93c0a34c1995d87914ab41398f9550 >>>> >>>> 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-whitepaper-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<michael.kubacki@microsoft.com> >>>>> >>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1562 >>>>> >>>>> The current SnpDxe implementation registers its ExitBootServices event >>>>> notification function (SnpNotifyExitBootServices ()) at TPL_NOTIFY. >>>>> This >>>>> function calls PxeShutdown() which issues an UNDI shutdown operation. >>>>> 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 <= TPL_CALLBACK. In addition, it has >>>>> been >>>>> observed in some 3rd party UNDI drivers to cause an issue further down >>>>> 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<siyuan.fu@intel.com> >>>>> Cc: Maciej Rabeda<maciej.rabeda@linux.intel.com> >>>>> Cc: Jiaxin Wu<jiaxin.wu@intel.com> >>>>> Signed-off-by: Michael Kubacki<michael.kubacki@microsoft.com> >>>>> --- >>>>> NetworkPkg/SnpDxe/Snp.c | 3 ++- >>>>> 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 @@ >>>>> Implementation of driver entry point and driver binding protocol. >>>>> Copyright (c) 2004 - 2019, Intel Corporation. All rights >>>>> reserved.<BR> >>>>> +Copyright (c) Microsoft Corporation.<BR> >>>>> SPDX-License-Identifier: BSD-2-Clause-Patent >>>>> **/ >>>>> @@ -653,7 +654,7 @@ SimpleNetworkDriverStart ( >>>>> // >>>>> Status = gBS->CreateEventEx ( >>>>> EVT_NOTIFY_SIGNAL, >>>>> - TPL_NOTIFY, >>>>> + TPL_CALLBACK, >>>>> SnpNotifyExitBootServices, >>>>> Snp, >>>>> &gEfiEventExitBootServicesGuid, >>>> >>> >>> >>> >> > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK 2020-04-16 16:12 ` Andrew Fish @ 2020-04-20 9:25 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2020-04-20 9:25 UTC (permalink / raw) To: Andrew Fish, devel Cc: Rabeda, Maciej, michael.kubacki, Siyuan Fu, Jiaxin Wu, Tomas Pilar (tpilar) On 04/16/20 18:12, Andrew Fish wrote: > Laszlo, > > This is "way back" but UNDI was defined in the Preboot Execution Environment (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. > > PXE was a PC BIOS specification for network booting, UNDI (Universal Network Device Interface) was a standardized abstraction for NIC hardware that goes back to PC BIOS Thanks for the explanation! > 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. Awesome! I've always missed a page like this! Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-20 9:25 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-09 18:16 [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK Michael Kubacki 2020-04-14 9:59 ` Maciej Rabeda 2020-04-14 17:08 ` Michael Kubacki 2020-04-15 9:45 ` [edk2-devel] " Maciej Rabeda 2020-04-15 16:25 ` Laszlo Ersek 2020-04-15 16:40 ` Laszlo Ersek 2020-04-16 15:25 ` Maciej Rabeda 2020-04-16 16:12 ` Andrew Fish 2020-04-20 9:25 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox