[PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Michael Kubacki]

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

Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Maciej Rabeda]

[-- 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 --]

Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Michael Kubacki]

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,
> 

Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Maciej Rabeda]

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,
>>
>
> 
>

Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Laszlo Ersek]

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,
>>>
>>
>> 
>>
> 

Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Maciej Rabeda]

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
>

Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Andrew Fish]

> 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,
>>>> 
>>> 
>>> 
>>> 
>> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Register SnpNotifyExitBootServices at TPL_CALLBACK

[Laszlo Ersek]

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