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

["Laszlo Ersek" <lersek@redhat.com>]

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