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

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

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