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

["Chiu, Chasel" <chasel.chiu@intel.com>]

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