[v1] MdeModulePkg: Enable/Disable S3BootScript dynamically.

[edk2-devel] [PATCH] MdeModulePkg: Enable/Disable S3BootScript dynamically.

Posted by Chiu, Chasel 4 years, 7 months ago

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

Current S3BootScriptLib can only support build time opt-out
but in binary model the same binary should support both
enabling and disabling scenarios.

To support this below PCD is expected to be used as
DynamicPCD (or DynamicEx) to skip S3BootScript functions
when PCD set to FALSE:
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable

Test: Verified on internal platform and S3BootScript
      functions can be skipped by PCD during boot time.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---
 MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c       | 13 ++++++++++++-
 MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf |  4 +++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
index c116727531..c5353119f7 100644
--- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
+++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
@@ -1,7 +1,7 @@
 /** @file
   Save the S3 data to S3 boot script.
 
-  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -124,6 +124,7 @@ VOID                             *mRegistrationSmmReadyToLock = NULL;
 BOOLEAN                          mS3BootScriptTableAllocated = FALSE;
 BOOLEAN                          mS3BootScriptTableSmmAllocated = FALSE;
 EFI_SMM_SYSTEM_TABLE2            *mBootScriptSmst = NULL;
+BOOLEAN                          mAcpiS3Enable = TRUE;
 
 /**
   This is an internal function to add a terminate node the entry, recalculate the table
@@ -436,6 +437,11 @@ S3BootScriptLibInitialize (
   BOOLEAN                        InSmm;
   EFI_PHYSICAL_ADDRESS           Buffer;
 
+  if (!PcdGetBool (PcdAcpiS3Enable)) {
+    mAcpiS3Enable = FALSE;
+    return RETURN_SUCCESS;
+  }
+
   S3TablePtr = (SCRIPT_TABLE_PRIVATE_DATA*)(UINTN)PcdGet64(PcdS3BootScriptTablePrivateDataPtr);
   //
   // The Boot script private data is not be initialized. create it
@@ -810,6 +816,11 @@ S3BootScriptGetEntryAddAddress (
 {
   UINT8*                         NewEntryPtr;
 
+  if (!mAcpiS3Enable) {
+    DEBUG ((DEBUG_INFO, "Skip S3BootScript because ACPI S3 disabled.\n"));
+    return NULL;
+  }
+
   if (mS3BootScriptTablePtr->SmmLocked) {
     //
     // We need check InSmm, because after SmmReadyToLock, only SMM driver is allowed to write boot script.
diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
index 517ea69568..fa139b03ff 100644
--- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
+++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
@@ -1,7 +1,7 @@
 ## @file
 # DXE S3 boot script Library.
 #
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -65,4 +65,6 @@
   ## SOMETIMES_PRODUCES
   gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptTablePrivateSmmDataPtr
   gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptRuntimeTableReservePageNumber   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                                ## CONSUMES
+
 
-- 
2.13.3.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47908): https://edk2.groups.io/g/devel/message/47908
Mute This Topic: https://groups.io/mt/34273353/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg: Enable/Disable S3BootScript dynamically.

Posted by Laszlo Ersek 4 years, 7 months ago

On 09/24/19 07:54, Chiu, Chasel wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2212
> 
> Current S3BootScriptLib can only support build time opt-out

I think this problem statement is wrong.

In the OVMF platform anyway, PiDxeS3BootScriptLib is linked into the following two modules:

  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
  MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf

Both modules honor "PcdAcpiS3Enable" already.

If the PCD is set to FALSE (which can be done dynamically, by platform PEI code, for example), then these modules exit their entry point functions with EFI_UNSUPPORTED.

As a consequence, the library DESTRUCTOR function will undo whatever the CONSTRUCTOR function did. Thus no resources should be leaked.

If you have a DXE driver that depends on the PiDxeS3BootScriptLib instance, such that it *cannot* exit from its entry point function with EFI_UNSUPPORTED, when PcdAcpiS3Enable is set to FALSE, then this patch could be justified. But then the use case should be described much more clearly. The current statement ("can only support build time opt-out") is wrong; the destructor function supports unloading, and drivers can ask for being unloaded dependent on a dynamic PCD.

Thanks
Laszlo

> but in binary model the same binary should support both
> enabling and disabling scenarios.
> 
> To support this below PCD is expected to be used as
> DynamicPCD (or DynamicEx) to skip S3BootScript functions
> when PCD set to FALSE:
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
> 
> Test: Verified on internal platform and S3BootScript
>       functions can be skipped by PCD during boot time.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> ---
>  MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c       | 13 ++++++++++++-
>  MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf |  4 +++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> index c116727531..c5353119f7 100644
> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Save the S3 data to S3 boot script.
>  
> -  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -124,6 +124,7 @@ VOID                             *mRegistrationSmmReadyToLock = NULL;
>  BOOLEAN                          mS3BootScriptTableAllocated = FALSE;
>  BOOLEAN                          mS3BootScriptTableSmmAllocated = FALSE;
>  EFI_SMM_SYSTEM_TABLE2            *mBootScriptSmst = NULL;
> +BOOLEAN                          mAcpiS3Enable = TRUE;
>  
>  /**
>    This is an internal function to add a terminate node the entry, recalculate the table
> @@ -436,6 +437,11 @@ S3BootScriptLibInitialize (
>    BOOLEAN                        InSmm;
>    EFI_PHYSICAL_ADDRESS           Buffer;
>  
> +  if (!PcdGetBool (PcdAcpiS3Enable)) {
> +    mAcpiS3Enable = FALSE;
> +    return RETURN_SUCCESS;
> +  }
> +
>    S3TablePtr = (SCRIPT_TABLE_PRIVATE_DATA*)(UINTN)PcdGet64(PcdS3BootScriptTablePrivateDataPtr);
>    //
>    // The Boot script private data is not be initialized. create it
> @@ -810,6 +816,11 @@ S3BootScriptGetEntryAddAddress (
>  {
>    UINT8*                         NewEntryPtr;
>  
> +  if (!mAcpiS3Enable) {
> +    DEBUG ((DEBUG_INFO, "Skip S3BootScript because ACPI S3 disabled.\n"));
> +    return NULL;
> +  }
> +
>    if (mS3BootScriptTablePtr->SmmLocked) {
>      //
>      // We need check InSmm, because after SmmReadyToLock, only SMM driver is allowed to write boot script.
> diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> index 517ea69568..fa139b03ff 100644
> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  # DXE S3 boot script Library.
>  #
> -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -65,4 +65,6 @@
>    ## SOMETIMES_PRODUCES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptTablePrivateSmmDataPtr
>    gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptRuntimeTableReservePageNumber   ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                                ## CONSUMES
> +
>  
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47996): https://edk2.groups.io/g/devel/message/47996
Mute This Topic: https://groups.io/mt/34273353/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg: Enable/Disable S3BootScript dynamically.

Posted by Chiu, Chasel 4 years, 7 months ago

Agree and I will correct the commit message to describe usage model better.
Thanks for good catch!

Regards,
Chasel


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Wednesday, September 25, 2019 6:23 AM
> To: devel@edk2.groups.io; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg: Enable/Disable
> S3BootScript dynamically.
> 
> On 09/24/19 07:54, Chiu, Chasel wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2212
> >
> > Current S3BootScriptLib can only support build time opt-out
> 
> I think this problem statement is wrong.
> 
> In the OVMF platform anyway, PiDxeS3BootScriptLib is linked into the
> following two modules:
> 
>   MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
> 
> MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorD
> xe.inf
> 
> Both modules honor "PcdAcpiS3Enable" already.
> 
> If the PCD is set to FALSE (which can be done dynamically, by platform PEI
> code, for example), then these modules exit their entry point functions with
> EFI_UNSUPPORTED.
> 
> As a consequence, the library DESTRUCTOR function will undo whatever the
> CONSTRUCTOR function did. Thus no resources should be leaked.
> 
> If you have a DXE driver that depends on the PiDxeS3BootScriptLib instance,
> such that it *cannot* exit from its entry point function with
> EFI_UNSUPPORTED, when PcdAcpiS3Enable is set to FALSE, then this patch
> could be justified. But then the use case should be described much more
> clearly. The current statement ("can only support build time opt-out") is
> wrong; the destructor function supports unloading, and drivers can ask for
> being unloaded dependent on a dynamic PCD.
> 
> Thanks
> Laszlo
> 
> > but in binary model the same binary should support both enabling and
> > disabling scenarios.
> >
> > To support this below PCD is expected to be used as DynamicPCD (or
> > DynamicEx) to skip S3BootScript functions when PCD set to FALSE:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
> >
> > Test: Verified on internal platform and S3BootScript
> >       functions can be skipped by PCD during boot time.
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> > ---
> >  MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c       |
> 13 ++++++++++++-
> >  MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf |
> 4
> > +++-
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> > b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> > index c116727531..c5353119f7 100644
> > --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> > +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Save the S3 data to S3 boot script.
> >
> > -  Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > +  Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > + reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -124,6 +124,7 @@ VOID
> *mRegistrationSmmReadyToLock = NULL;
> >  BOOLEAN                          mS3BootScriptTableAllocated =
> FALSE;
> >  BOOLEAN
> mS3BootScriptTableSmmAllocated = FALSE;
> >  EFI_SMM_SYSTEM_TABLE2            *mBootScriptSmst = NULL;
> > +BOOLEAN                          mAcpiS3Enable = TRUE;
> >
> >  /**
> >    This is an internal function to add a terminate node the entry,
> > recalculate the table @@ -436,6 +437,11 @@ S3BootScriptLibInitialize (
> >    BOOLEAN                        InSmm;
> >    EFI_PHYSICAL_ADDRESS           Buffer;
> >
> > +  if (!PcdGetBool (PcdAcpiS3Enable)) {
> > +    mAcpiS3Enable = FALSE;
> > +    return RETURN_SUCCESS;
> > +  }
> > +
> >    S3TablePtr =
> (SCRIPT_TABLE_PRIVATE_DATA*)(UINTN)PcdGet64(PcdS3BootScriptTablePrivat
> eDataPtr);
> >    //
> >    // The Boot script private data is not be initialized. create it @@
> > -810,6 +816,11 @@ S3BootScriptGetEntryAddAddress (  {
> >    UINT8*                         NewEntryPtr;
> >
> > +  if (!mAcpiS3Enable) {
> > +    DEBUG ((DEBUG_INFO, "Skip S3BootScript because ACPI S3
> disabled.\n"));
> > +    return NULL;
> > +  }
> > +
> >    if (mS3BootScriptTablePtr->SmmLocked) {
> >      //
> >      // We need check InSmm, because after SmmReadyToLock, only
> SMM driver is allowed to write boot script.
> > diff --git
> > a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> > b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> > index 517ea69568..fa139b03ff 100644
> > --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> > +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  # DXE S3 boot script Library.
> >  #
> > -# Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +# Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -65,4 +65,6 @@
> >    ## SOMETIMES_PRODUCES
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptTablePrivateSmmDataPtr
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptRuntimeTableReservePa
> geNumber   ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
> ## CONSUMES
> > +
> >
> >
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48014): https://edk2.groups.io/g/devel/message/48014
Mute This Topic: https://groups.io/mt/34273353/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-