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

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

Thanks Laszlo for your time on detail reviewing and very good feedbacks!
Please see my reply inline below.

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, September 26, 2019 2:58 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 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.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>
> > Cc: Laszlo Ersek <lersek@redhat.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
> 
> (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