Thanks Laszlo for your time on detail reviewing and very good feedbacks! Please see my reply inline below.
> -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, September 26, 2019 2:58 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 v2] MdeModulePkg: Enable/Disable > S3BootScript dynamically. > > On 09/25/19 11:21, Chiu, Chasel wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2212 > > > > In binary model the same binary may have to support both > > S3 enabled and disabled scenarios, however not all DXE drivers linking > > PiDxeS3BootScriptLib can return error to invoke library DESTRUCTOR for > > releasing resource. > > Thanks, this sounds better. More comments: > > > To support this usage model below PCD is used 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> > > Cc: Laszlo Ersek <ler...@redhat.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 > > (1) I think that, for future maintenance, it would help if we added a similar > check (on mAcpiS3Enable) to S3BootScriptLibDeinitialize() as well. > > I understand that, right now, if the constructor is short-circuited, then the > destructor will end up doing nothing. But I think it would make maintenance > easier if the destructor were short-circuited explicitly as well. > Agree. I will add check to destructor. > > > @@ -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. > > (2) I would like to see the debug message updated: > > (2a) please log, as part of the message, with "%a", the "gEfiCallerBaseName" > variable. A library instance can be linked into multiple modules, and > knowing the module name is useful. > Yes. will add module name into debug message. > (2b) I think we should add the debug message to the constructor function > instead. Please see the message that we already have in the destructor. > > Mainly, a DEBUG_INFO message is too loud for a utility function that may be > called several times. So, if we keep the message at DEBUG_INFO, it should be > moved into the constructor. Conversely, if you want to keep the message in > S3BootScriptGetEntryAddAddress(), then it should be downgraded to > DEBUG_VERBOSE. > I have question for adding debug message in constructor. DebugLib might also have constructor and as far as I know currently we do not have mechanism to ensure that DebugLib constructor will be executed earlier than S3BootScriptLib constructor. Or have we add the mechanism already? If so I have no concern to move debug message to constructor. DEBUG_VERBOSE sounds like good idea but most of the platforms by default disabled it and will not see this message. How about adding a "mDebugMessageAlreadyPrinted" to control the debug message only show once per module, what do you think? > > > 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 > > + > > (3) Please do not add the superfluous empty line. > Sorry for this, I will correct it and be more careful in the following code review. (might PatchCheck.py be enhanced to capture this kind of coding style issue?) > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48066): https://edk2.groups.io/g/devel/message/48066 Mute This Topic: https://groups.io/mt/34285869/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
