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

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

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