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

["Laszlo Ersek" <lersek@redhat.com>]

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(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.

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/DxeRuntimeDebugLibSerialPort.inf

do have constructor functions.

> 
> 
>>
>>> 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?)

Not sure :)

Thanks!
Laszlo