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.

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(PcdS3BootScriptTablePrivateDataPtr);
   //
   // 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.PcdS3BootScriptRuntimeTableReservePageNumber   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                                ## CONSUMES
+
 
-- 
2.13.3.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48015): https://edk2.groups.io/g/devel/message/48015
Mute This Topic: https://groups.io/mt/34285869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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(PcdS3BootScriptTablePrivateDataPtr);
>    //
>    // 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.


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

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


> 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.PcdS3BootScriptRuntimeTableReservePageNumber   ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                                ## CONSUMES
> +

(3) Please do not add the superfluous empty line.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48047): https://edk2.groups.io/g/devel/message/48047
Mute This Topic: https://groups.io/mt/34285869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48110): https://edk2.groups.io/g/devel/message/48110
Mute This Topic: https://groups.io/mt/34285869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48191): https://edk2.groups.io/g/devel/message/48191
Mute This Topic: https://groups.io/mt/34285869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-