Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

["Tomas Pilar (tpilar)" <tpilar@solarflare.com>]

I've commented on the bug, SFC adapters will be fine, but I am sure there is a vendor out there that will be affected. Bugs due to memory corruption during OS load that depend on the network externalities are notoriously difficult to diagnose.

We need to make sure this is properly communicated and signposted so others can audit their drivers.

Cheers,
Tom
________________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Rabeda, Maciej <maciej.rabeda@intel.com>
Sent: 09 October 2019 09:50
To: Fu, Siyuan; Devel EDK2
Cc: Wu, Jiaxin
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

Hi Siyuan,

This change has no effect to Intel Ethernet Server UNDI drivers.
They already handle ExitBootServices event and configure the Ethernet adapters not to perform any DMA at this point.

As per whitepaper (https://firmware.intel.com/sites/default/files/Intel_WhitePaper_Using_IOMMU_for_DMA_Protection_in_UEFI.pdf), section Call to Action, UEFI device drivers (including UNDI) should put the controller to halt state and disable bus mastering in ExitBootServices stage - which obliges UNDI drivers to have ExitBootServices event handlers. To me, this means shutting down DMA on the controllers to which UNDI driver is bound.

As for second question, not disabling DMA will make our adapters continue writing Rx packet data to RAM in DXE Runtime stage.
I believe that this behavior is as far from desired as it can.

Thanks!
Maciej Rabeda

-----Original Message-----
From: Fu, Siyuan
Sent: Wednesday, October 9, 2019 04:08
To: Rabeda, Maciej <maciej.rabeda@intel.com>; devel@edk2.groups.io
Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

Hi, Maciej

Have you tested what will happen if this SNP co-work with those UNDI drivers which don't have an ExitBootService event callback to shut down its DMA activity? And what's the impact to OS if UNDI's DMA is not shut down?

Best Regards
Siyuan

> -----Original Message-----
> From: Rabeda, Maciej <maciej.rabeda@intel.com>
> Sent: 2019年10月9日 0:16
> To: devel@edk2.groups.io
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices
> event
>
> Patch addresses Bugzilla #1972.
> During ExitBootServices stage, drivers should not call any functions
> known to use Memory Allocation Services. One of such functions (as per
> UEFI spec) is UNDI->Shutdown().
>
> Since UNDI drivers during ExitBootServices phase are expected to put
> the adapter to such a state that it will not perform any DMA
> operations, there is no need to interface UNDI by SNP driver during
> that phase.
>
> Finally, since ExitBootServices event notification function in SNP
> only calls UNDI->Shutdown() and Stop() functions, there is no need to
> create this event at all.
>
> Signed-off-by: Maciej Rabeda <maciej.rabeda@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  NetworkPkg/SnpDxe/Snp.c      | 45 --------------------
>  NetworkPkg/SnpDxe/Snp.h      |  2 -
>  NetworkPkg/SnpDxe/SnpDxe.inf |  3 --
>  3 files changed, 50 deletions(-)
>
> diff --git a/NetworkPkg/SnpDxe/Snp.c b/NetworkPkg/SnpDxe/Snp.c index
> a23af05078bc..7646a3ce0293 100644
> --- a/NetworkPkg/SnpDxe/Snp.c
> +++ b/NetworkPkg/SnpDxe/Snp.c
> @@ -8,31 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  #include "Snp.h"
>
> -/**
> -  One notified function to stop UNDI device when
> gBS->ExitBootServices() called.
> -
> -  @param  Event                   Pointer to this event
> -  @param  Context                 Event handler private data
> -
> -**/
> -VOID
> -EFIAPI
> -SnpNotifyExitBootServices (
> -  EFI_EVENT Event,
> -  VOID      *Context
> -  )
> -{
> -  SNP_DRIVER             *Snp;
> -
> -  Snp  = (SNP_DRIVER *)Context;
> -
> -  //
> -  // Shutdown and stop UNDI driver
> -  //
> -  PxeShutdown (Snp);
> -  PxeStop (Snp);
> -}
> -
>  /**
>    Send command to UNDI. It does nothing currently.
>
> @@ -647,21 +622,6 @@ SimpleNetworkDriverStart (
>    PxeShutdown (Snp);
>    PxeStop (Snp);
>
> -  //
> -  // Create EXIT_BOOT_SERIVES Event
> -  //
> -  Status = gBS->CreateEventEx (
> -                  EVT_NOTIFY_SIGNAL,
> -                  TPL_NOTIFY,
> -                  SnpNotifyExitBootServices,
> -                  Snp,
> -                  &gEfiEventExitBootServicesGuid,
> -                  &Snp->ExitBootServicesEvent
> -                  );
> -  if (EFI_ERROR (Status)) {
> -    goto Error_DeleteSNP;
> -  }
> -
>    //
>    //  add SNP to the undi handle
>    //
> @@ -778,11 +738,6 @@ SimpleNetworkDriverStop (
>      return Status;
>    }
>
> -  //
> -  // Close EXIT_BOOT_SERIVES Event
> -  //
> -  gBS->CloseEvent (Snp->ExitBootServicesEvent);
> -
>    Status = gBS->CloseProtocol (
>                    Controller,
>                    &gEfiNetworkInterfaceIdentifierProtocolGuid_31,
> diff --git a/NetworkPkg/SnpDxe/Snp.h b/NetworkPkg/SnpDxe/Snp.h index
> e6b62930397d..f83a4f075adc 100644
> --- a/NetworkPkg/SnpDxe/Snp.h
> +++ b/NetworkPkg/SnpDxe/Snp.h
> @@ -120,8 +120,6 @@ typedef struct {
>      VOID                  *MapCookie;
>    } MapList[MAX_MAP_LENGTH];
>
> -  EFI_EVENT              ExitBootServicesEvent;
> -
>    //
>    // Whether UNDI support reporting media status from GET_STATUS
> command,
>    // i.e. PXE_STATFLAGS_GET_STATUS_NO_MEDIA_SUPPORTED or diff --git
> a/NetworkPkg/SnpDxe/SnpDxe.inf b/NetworkPkg/SnpDxe/SnpDxe.inf index
> afeb1526cc10..8d045cfcf4ca 100644
> --- a/NetworkPkg/SnpDxe/SnpDxe.inf
> +++ b/NetworkPkg/SnpDxe/SnpDxe.inf
> @@ -64,9 +64,6 @@
>    DebugLib
>    NetLib
>
> -[Guids]
> -  gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> Event
> -
>  [Protocols]
>    gEfiSimpleNetworkProtocolGuid                 ## BY_START
>    gEfiDevicePathProtocolGuid                    ## TO_START
> --
> 2.17.0.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.