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

["Andrew Fish" <afish@apple.com>]

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