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.c...@intel.com> > Cc: Wu, Hao A <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; > Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Gao, Liming > <liming....@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...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Signed-off-by: Chasel Chiu <chasel.c...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
