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

["Michael Kubacki" <michael.kubacki@outlook.com>]

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