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

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

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, September 26, 2019 9:58 PM
> To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
> 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/26/19 03:52, Chiu, Chasel wrote:
> >
> > 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(PcdS3BootScriptTablePriva
> >> t
> >> 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.
> 
> To my understanding, if:
> - we have a library instance Instance1 (for Class1), and
> - we have Instance2 (for Class2), and
> - Instance1 depends on Class2, and
> - both Instance1 and Instance2 have CONSTRUCTOR functions,
> 
> then the build tools make sure to generate such code that the constructor of
> Instance2 be called before calling the constructor of Instance1.
> 
> We have encountered issues in the past *only* when at least one instance
> lacked a constructor. (Note that it can get quite tricky: you can have a long
> chain of dependencies, and if one "link" (one library instance in the
> dependency chain) lacks a constructor, then things can break.)
> 
> To give you one example, the following constructor functions:
> - QemuFwCfgInitialize() [OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c]
> - QemuFwCfgInitialize() [OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c]
> 
> call DEBUG(). They work fine.
> 
> For example, "OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf" is a module
> that depends on QemuFwCfgLib. The QemuFwCfgLib instance is the DXE one,
> from above (OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf).
> 
> Furthermore, QemuFwCfgDxeLib.inf depends on DebugLib. And the DebugLib
> instance in use is
> "OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf",
> which also has a constructor function: PlatformDebugLibIoPortConstructor().
> 
> Now, let's see the code generated for AcpiPlatformDxe ("AutoGen.c"):
> 
> VOID
> EFIAPI
> ProcessLibraryConstructorList (
>   IN EFI_HANDLE        ImageHandle,
>   IN EFI_SYSTEM_TABLE  *SystemTable
>   )
> {
>   EFI_STATUS  Status;
> 
>   Status = PlatformDebugLibIoPortConstructor ();
>   ASSERT_RETURN_ERROR (Status);
> 
>   Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = UefiRuntimeServicesTableLibConstructor (ImageHandle,
> SystemTable);
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = DevicePathLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = UefiLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = QemuFwCfgInitialize ();
>   ASSERT_RETURN_ERROR (Status);
> 
>   Status = HobLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
> 
>   Status = DxeServicesTableLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
> 
> }
> 
> As you can see, this is a topological sort (put differently, a dependency sort).
> The PlatformDebugLibIoPort instance is constructed first, because that's the
> one all the other library constructors in AcpiPlatformDxe depend on.
> QemuFwCfgInitialize() is called later, and so the DEBUG() calls in
> QemuFwCfgInitialize() work fine.
> 
> > DEBUG_VERBOSE sounds like good idea but most of the platforms by
> default disabled it and will not see this message.
> 
> That's OK. It is up to those platforms to enable DEBUG_VERBOSE in their
> debug log masks.
> 
> Note that the relevant PCD can be set per module too (in the DSC file), not
> only globally for all modules, so it should be quite flexible.
> 
> > How about adding a "mDebugMessageAlreadyPrinted" to control the
> debug message only show once per module, what do you think?
> 
> I think that's an inferior solution. If we want to print the message only once,
> then it should go into the constructor.
> 
> I think that, in practice, *all* DebugLib instances should have constructor
> functions -- even if they do nothing, they should have constructors precisely
> in order to enable the topological sort in BaseTools that I describe above.
> 
> The two most commonly used serial port debug lib instances in edk2,
> namely
> 
> - MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> -
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialP
> ort.inf
> 
> do have constructor functions.
> 


I did a test and it works so I will send V3 patch to move debug message to constructor.
Thanks Laszlo!


> >
> >
> >>
> >>> 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.i
> >>> +++ nf
> >>> @@ -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?)
> 
> Not sure :)
> 
> Thanks!
> Laszlo