public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v1 0/1] Remove UNDI calls from SNP during ExitBootServices
@ 2019-10-08 16:16 Rabeda, Maciej
  2019-10-08 16:16 ` [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Rabeda, Maciej
  0 siblings, 1 reply; 13+ messages in thread
From: Rabeda, Maciej @ 2019-10-08 16:16 UTC (permalink / raw)
  To: devel; +Cc: Siyuan Fu, Jiaxin Wu

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1974

Removing calls to UNDI->Shutdown() and Stop() functions in ExitBootServices
event notification function. Since SNP event reacting to ExitBootServices
is only calling those two functions, I am removing that as well.

Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Maciej Rabeda (1):
  NetworkPkg/SnpDxe: Remove ExitBootServices event

 NetworkPkg/SnpDxe/Snp.c      | 45 --------------------
 NetworkPkg/SnpDxe/Snp.h      |  2 -
 NetworkPkg/SnpDxe/SnpDxe.inf |  3 --
 3 files changed, 50 deletions(-)

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-08 16:16 [PATCH v1 0/1] Remove UNDI calls from SNP during ExitBootServices Rabeda, Maciej
@ 2019-10-08 16:16 ` Rabeda, Maciej
  2019-10-09  2:07   ` Siyuan, Fu
  2019-10-09 22:09   ` Laszlo Ersek
  0 siblings, 2 replies; 13+ messages in thread
From: Rabeda, Maciej @ 2019-10-08 16:16 UTC (permalink / raw)
  To: devel; +Cc: Siyuan Fu, Jiaxin Wu

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.


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-08 16:16 ` [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Rabeda, Maciej
@ 2019-10-09  2:07   ` Siyuan, Fu
  2019-10-09  8:50     ` Rabeda, Maciej
  2019-10-09 22:09   ` Laszlo Ersek
  1 sibling, 1 reply; 13+ messages in thread
From: Siyuan, Fu @ 2019-10-09  2:07 UTC (permalink / raw)
  To: Rabeda, Maciej, devel@edk2.groups.io; +Cc: Wu, Jiaxin

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-09  2:07   ` Siyuan, Fu
@ 2019-10-09  8:50     ` Rabeda, Maciej
  2019-10-09  9:38       ` [edk2-devel] " Tomas Pilar (tpilar)
  0 siblings, 1 reply; 13+ messages in thread
From: Rabeda, Maciej @ 2019-10-09  8:50 UTC (permalink / raw)
  To: Fu, Siyuan, devel@edk2.groups.io; +Cc: Wu, Jiaxin

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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-09  8:50     ` Rabeda, Maciej
@ 2019-10-09  9:38       ` Tomas Pilar (tpilar)
  0 siblings, 0 replies; 13+ messages in thread
From: Tomas Pilar (tpilar) @ 2019-10-09  9:38 UTC (permalink / raw)
  To: Fu, Siyuan, Devel EDK2, maciej.rabeda@intel.com; +Cc: Wu, Jiaxin

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.





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-08 16:16 ` [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Rabeda, Maciej
  2019-10-09  2:07   ` Siyuan, Fu
@ 2019-10-09 22:09   ` Laszlo Ersek
  2019-10-10  3:32     ` Siyuan, Fu
  1 sibling, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2019-10-09 22:09 UTC (permalink / raw)
  To: devel, maciej.rabeda; +Cc: Siyuan Fu, Jiaxin Wu

On 10/08/19 18:16, Rabeda, Maciej wrote:
> Patch addresses Bugzilla #1972.

I think the BZ reference should be
<https://bugzilla.tianocore.org/show_bug.cgi?id=1974>. (The cover letter
has it right.)

Thanks
Laszlo

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-09 22:09   ` Laszlo Ersek
@ 2019-10-10  3:32     ` Siyuan, Fu
  2019-10-10  8:06       ` Laszlo Ersek
  0 siblings, 1 reply; 13+ messages in thread
From: Siyuan, Fu @ 2019-10-10  3:32 UTC (permalink / raw)
  To: devel@edk2.groups.io, lersek@redhat.com, Rabeda, Maciej; +Cc: Wu, Jiaxin

Hi, Maciej

Considering that this patch has to co-work with corresponding UNDI device driver
bug fix, in order to avoid potential compatibility problem, please add a PCD to 
NetworkPkg for this fix, and set the default value to disable state (no behavior
 change). The platform which need this fix could set the PCD to enable in its 
platform DSC.

Please add clear description for the problem, new PCD, the SNP fix, and also the
 expected UNDI driver fix in Bugzilla 1974. So 3rd party UNDI device vendor could
 know how to fix the problem if they meet same issue.

And please correct the Bugzilla number in patch description as Laszlo pointed out.

Thanks.

Best Regards
Siyuan 

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: 2019年10月10日 6:10
> To: devel@edk2.groups.io; Rabeda, Maciej <maciej.rabeda@intel.com>
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> ExitBootServices event
> 
> On 10/08/19 18:16, Rabeda, Maciej wrote:
> > Patch addresses Bugzilla #1972.
> 
> I think the BZ reference should be
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1974>. (The cover letter
> has it right.)
> 
> Thanks
> Laszlo
> 
> > 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
> >
> 
> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-10  3:32     ` Siyuan, Fu
@ 2019-10-10  8:06       ` Laszlo Ersek
  2019-10-10  9:29         ` Siyuan, Fu
  0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2019-10-10  8:06 UTC (permalink / raw)
  To: Fu, Siyuan, devel@edk2.groups.io, Rabeda, Maciej; +Cc: Wu, Jiaxin

On 10/10/19 05:32, Fu, Siyuan wrote:
> Hi, Maciej
> 
> Considering that this patch has to co-work with corresponding UNDI device driver
> bug fix, in order to avoid potential compatibility problem, please add a PCD to 
> NetworkPkg for this fix, and set the default value to disable state (no behavior
>  change). The platform which need this fix could set the PCD to enable in its 
> platform DSC.

Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
gated with a new build flag defined in "NetworkPkg/NetworkDefines.dsc.inc"?

Thanks
Laszlo

> 
> Please add clear description for the problem, new PCD, the SNP fix, and also the
>  expected UNDI driver fix in Bugzilla 1974. So 3rd party UNDI device vendor could
>  know how to fix the problem if they meet same issue.
> 
> And please correct the Bugzilla number in patch description as Laszlo pointed out.
> 
> Thanks.
> 
> Best Regards
> Siyuan 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>> Sent: 2019年10月10日 6:10
>> To: devel@edk2.groups.io; Rabeda, Maciej <maciej.rabeda@intel.com>
>> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
>> ExitBootServices event
>>
>> On 10/08/19 18:16, Rabeda, Maciej wrote:
>>> Patch addresses Bugzilla #1972.
>>
>> I think the BZ reference should be
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=1974>. (The cover letter
>> has it right.)
>>
>> Thanks
>> Laszlo
>>
>>> 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
>>>
>>
>>
>> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-10  8:06       ` Laszlo Ersek
@ 2019-10-10  9:29         ` Siyuan, Fu
  2019-10-10 16:06           ` Laszlo Ersek
  0 siblings, 1 reply; 13+ messages in thread
From: Siyuan, Fu @ 2019-10-10  9:29 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, Rabeda, Maciej; +Cc: Wu, Jiaxin

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2019年10月10日 16:06
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
> Maciej <maciej.rabeda@intel.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> ExitBootServices event
> 
> On 10/10/19 05:32, Fu, Siyuan wrote:
> > Hi, Maciej
> >
> > Considering that this patch has to co-work with corresponding UNDI device
> driver
> > bug fix, in order to avoid potential compatibility problem, please add a PCD
> to
> > NetworkPkg for this fix, and set the default value to disable state (no
> behavior
> >  change). The platform which need this fix could set the PCD to enable in
> its
> > platform DSC.
> 
> Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
> gated with a new build flag defined in "NetworkPkg/NetworkDefines.dsc.inc"?

Currently not all the network package PCDs are listed in the NetworkPcds.dsc.inc,
But I do not oppose it if you think this PCD should be controlled by a build flag.

Thanks,
Siyuan

> 
> Thanks
> Laszlo
> 
> >
> > Please add clear description for the problem, new PCD, the SNP fix, and
> also the
> >  expected UNDI driver fix in Bugzilla 1974. So 3rd party UNDI device vendor
> could
> >  know how to fix the problem if they meet same issue.
> >
> > And please correct the Bugzilla number in patch description as Laszlo
> pointed out.
> >
> > Thanks.
> >
> > Best Regards
> > Siyuan
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> >> Ersek
> >> Sent: 2019年10月10日 6:10
> >> To: devel@edk2.groups.io; Rabeda, Maciej <maciej.rabeda@intel.com>
> >> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> >> ExitBootServices event
> >>
> >> On 10/08/19 18:16, Rabeda, Maciej wrote:
> >>> Patch addresses Bugzilla #1972.
> >>
> >> I think the BZ reference should be
> >> <https://bugzilla.tianocore.org/show_bug.cgi?id=1974>. (The cover letter
> >> has it right.)
> >>
> >> Thanks
> >> Laszlo
> >>
> >>> 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
> >>>
> >>
> >>
> >> 
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-10  9:29         ` Siyuan, Fu
@ 2019-10-10 16:06           ` Laszlo Ersek
  2019-10-11  0:14             ` Siyuan, Fu
  0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2019-10-10 16:06 UTC (permalink / raw)
  To: Fu, Siyuan, devel@edk2.groups.io, Rabeda, Maciej; +Cc: Wu, Jiaxin

On 10/10/19 11:29, Fu, Siyuan wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: 2019年10月10日 16:06
>> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
>> Maciej <maciej.rabeda@intel.com>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
>> ExitBootServices event
>>
>> On 10/10/19 05:32, Fu, Siyuan wrote:
>>> Hi, Maciej
>>>
>>> Considering that this patch has to co-work with corresponding UNDI device
>> driver
>>> bug fix, in order to avoid potential compatibility problem, please add a PCD
>> to
>>> NetworkPkg for this fix, and set the default value to disable state (no
>> behavior
>>>  change). The platform which need this fix could set the PCD to enable in
>> its
>>> platform DSC.
>>
>> Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
>> gated with a new build flag defined in "NetworkPkg/NetworkDefines.dsc.inc"?
> 
> Currently not all the network package PCDs are listed in the NetworkPcds.dsc.inc,
> But I do not oppose it if you think this PCD should be controlled by a build flag.

I mainly asked from a consistency point of view.

The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE, at
least in platforms that utilize the NetworkPkg *.inc files. And the PCD
in question would control the behavior of SnpDxe at ExitBootServices(),
if I understand correctly.

This is similar to NETWORK_HTTP_BOOT_ENABLE and
NETWORK_ALLOW_HTTP_CONNECTIONS:
- The former includes HttpDxe and HttpBootDxe (and some other drivers).
- The latter controls PcdAllowHttpConnections, which is consumed by
HttpDxe and HttpBootDxe.

I don't feel too strongly about it, I just thought it worth raising.

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-10 16:06           ` Laszlo Ersek
@ 2019-10-11  0:14             ` Siyuan, Fu
  2019-10-11  9:41               ` Laszlo Ersek
  0 siblings, 1 reply; 13+ messages in thread
From: Siyuan, Fu @ 2019-10-11  0:14 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, Rabeda, Maciej; +Cc: Wu, Jiaxin

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2019年10月11日 0:06
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
> Maciej <maciej.rabeda@intel.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> ExitBootServices event
> 
> On 10/10/19 11:29, Fu, Siyuan wrote:
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: 2019年10月10日 16:06
> >> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
> >> Maciej <maciej.rabeda@intel.com>
> >> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> >> ExitBootServices event
> >>
> >> On 10/10/19 05:32, Fu, Siyuan wrote:
> >>> Hi, Maciej
> >>>
> >>> Considering that this patch has to co-work with corresponding UNDI
> device
> >> driver
> >>> bug fix, in order to avoid potential compatibility problem, please add a
> PCD
> >> to
> >>> NetworkPkg for this fix, and set the default value to disable state (no
> >> behavior
> >>>  change). The platform which need this fix could set the PCD to enable in
> >> its
> >>> platform DSC.
> >>
> >> Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
> >> gated with a new build flag defined in
> "NetworkPkg/NetworkDefines.dsc.inc"?
> >
> > Currently not all the network package PCDs are listed in the
> NetworkPcds.dsc.inc,
> > But I do not oppose it if you think this PCD should be controlled by a build
> flag.
> 
> I mainly asked from a consistency point of view.
> 
> The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE, at
> least in platforms that utilize the NetworkPkg *.inc files. And the PCD
> in question would control the behavior of SnpDxe at ExitBootServices(),
> if I understand correctly.
> 
> This is similar to NETWORK_HTTP_BOOT_ENABLE and
> NETWORK_ALLOW_HTTP_CONNECTIONS:
> - The former includes HttpDxe and HttpBootDxe (and some other drivers).
> - The latter controls PcdAllowHttpConnections, which is consumed by
> HttpDxe and HttpBootDxe.
> 
> I don't feel too strongly about it, I just thought it worth raising.

It's not an existing "consistency" in current network package. The current macros
are mostly used for control more high level feature enable/disable, not such kind
of bug fix,
For example, NETWORK_ISCSI_ENABLE includes IScsiDxe driver, while 
PcdIScsiAIPNetworkBootPolicy is consumed by IScsiDriver, but not controlled by
a macro. And also other PXE, DHCP and PXE related PCDs.

Thanks,
Siyuan

> 
> Thanks
> Laszlo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-11  0:14             ` Siyuan, Fu
@ 2019-10-11  9:41               ` Laszlo Ersek
  2019-10-11 10:08                 ` Rabeda, Maciej
  0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2019-10-11  9:41 UTC (permalink / raw)
  To: Fu, Siyuan, devel@edk2.groups.io, Rabeda, Maciej; +Cc: Wu, Jiaxin

On 10/11/19 02:14, Fu, Siyuan wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: 2019年10月11日 0:06
>> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
>> Maciej <maciej.rabeda@intel.com>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
>> ExitBootServices event
>>
>> On 10/10/19 11:29, Fu, Siyuan wrote:
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: 2019年10月10日 16:06
>>>> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
>>>> Maciej <maciej.rabeda@intel.com>
>>>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
>>>> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
>>>> ExitBootServices event
>>>>
>>>> On 10/10/19 05:32, Fu, Siyuan wrote:
>>>>> Hi, Maciej
>>>>>
>>>>> Considering that this patch has to co-work with corresponding UNDI
>> device
>>>> driver
>>>>> bug fix, in order to avoid potential compatibility problem, please add a
>> PCD
>>>> to
>>>>> NetworkPkg for this fix, and set the default value to disable state (no
>>>> behavior
>>>>>  change). The platform which need this fix could set the PCD to enable in
>>>> its
>>>>> platform DSC.
>>>>
>>>> Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
>>>> gated with a new build flag defined in
>> "NetworkPkg/NetworkDefines.dsc.inc"?
>>>
>>> Currently not all the network package PCDs are listed in the
>> NetworkPcds.dsc.inc,
>>> But I do not oppose it if you think this PCD should be controlled by a build
>> flag.
>>
>> I mainly asked from a consistency point of view.
>>
>> The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE, at
>> least in platforms that utilize the NetworkPkg *.inc files. And the PCD
>> in question would control the behavior of SnpDxe at ExitBootServices(),
>> if I understand correctly.
>>
>> This is similar to NETWORK_HTTP_BOOT_ENABLE and
>> NETWORK_ALLOW_HTTP_CONNECTIONS:
>> - The former includes HttpDxe and HttpBootDxe (and some other drivers).
>> - The latter controls PcdAllowHttpConnections, which is consumed by
>> HttpDxe and HttpBootDxe.
>>
>> I don't feel too strongly about it, I just thought it worth raising.
> 
> It's not an existing "consistency" in current network package. The current macros
> are mostly used for control more high level feature enable/disable, not such kind
> of bug fix,
> For example, NETWORK_ISCSI_ENABLE includes IScsiDxe driver, while 
> PcdIScsiAIPNetworkBootPolicy is consumed by IScsiDriver, but not controlled by
> a macro. And also other PXE, DHCP and PXE related PCDs.

Good point!

So, I'm fine if the new feature PCD is not exposed with a build flag.

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
  2019-10-11  9:41               ` Laszlo Ersek
@ 2019-10-11 10:08                 ` Rabeda, Maciej
  0 siblings, 0 replies; 13+ messages in thread
From: Rabeda, Maciej @ 2019-10-11 10:08 UTC (permalink / raw)
  To: Laszlo Ersek, Fu, Siyuan, devel@edk2.groups.io; +Cc: Wu, Jiaxin

Laszlo, Siyuan,

Very good input, thanks.
I'll introduce the V2 patch with that fix + control with fixed PCD - it works for me :)

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, October 11, 2019 11:42
To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda, Maciej <maciej.rabeda@intel.com>
Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

On 10/11/19 02:14, Fu, Siyuan wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: 2019年10月11日 0:06
>> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda, 
>> Maciej <maciej.rabeda@intel.com>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove 
>> ExitBootServices event
>>
>> On 10/10/19 11:29, Fu, Siyuan wrote:
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: 2019年10月10日 16:06
>>>> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda, 
>>>> Maciej <maciej.rabeda@intel.com>
>>>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
>>>> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove 
>>>> ExitBootServices event
>>>>
>>>> On 10/10/19 05:32, Fu, Siyuan wrote:
>>>>> Hi, Maciej
>>>>>
>>>>> Considering that this patch has to co-work with corresponding UNDI
>> device
>>>> driver
>>>>> bug fix, in order to avoid potential compatibility problem, please 
>>>>> add a
>> PCD
>>>> to
>>>>> NetworkPkg for this fix, and set the default value to disable 
>>>>> state (no
>>>> behavior
>>>>>  change). The platform which need this fix could set the PCD to 
>>>>> enable in
>>>> its
>>>>> platform DSC.
>>>>
>>>> Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be 
>>>> gated with a new build flag defined in
>> "NetworkPkg/NetworkDefines.dsc.inc"?
>>>
>>> Currently not all the network package PCDs are listed in the
>> NetworkPcds.dsc.inc,
>>> But I do not oppose it if you think this PCD should be controlled by 
>>> a build
>> flag.
>>
>> I mainly asked from a consistency point of view.
>>
>> The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE, 
>> at least in platforms that utilize the NetworkPkg *.inc files. And 
>> the PCD in question would control the behavior of SnpDxe at 
>> ExitBootServices(), if I understand correctly.
>>
>> This is similar to NETWORK_HTTP_BOOT_ENABLE and
>> NETWORK_ALLOW_HTTP_CONNECTIONS:
>> - The former includes HttpDxe and HttpBootDxe (and some other drivers).
>> - The latter controls PcdAllowHttpConnections, which is consumed by 
>> HttpDxe and HttpBootDxe.
>>
>> I don't feel too strongly about it, I just thought it worth raising.
> 
> It's not an existing "consistency" in current network package. The 
> current macros are mostly used for control more high level feature 
> enable/disable, not such kind of bug fix, For example, 
> NETWORK_ISCSI_ENABLE includes IScsiDxe driver, while 
> PcdIScsiAIPNetworkBootPolicy is consumed by IScsiDriver, but not 
> controlled by a macro. And also other PXE, DHCP and PXE related PCDs.

Good point!

So, I'm fine if the new feature PCD is not exposed with a build flag.

Thanks
Laszlo
--------------------------------------------------------------------

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-10-11 10:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-08 16:16 [PATCH v1 0/1] Remove UNDI calls from SNP during ExitBootServices Rabeda, Maciej
2019-10-08 16:16 ` [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Rabeda, Maciej
2019-10-09  2:07   ` Siyuan, Fu
2019-10-09  8:50     ` Rabeda, Maciej
2019-10-09  9:38       ` [edk2-devel] " Tomas Pilar (tpilar)
2019-10-09 22:09   ` Laszlo Ersek
2019-10-10  3:32     ` Siyuan, Fu
2019-10-10  8:06       ` Laszlo Ersek
2019-10-10  9:29         ` Siyuan, Fu
2019-10-10 16:06           ` Laszlo Ersek
2019-10-11  0:14             ` Siyuan, Fu
2019-10-11  9:41               ` Laszlo Ersek
2019-10-11 10:08                 ` Rabeda, Maciej

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox