> -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, September 26, 2019 9:58 PM > To: Chiu, Chasel <chasel.c...@intel.com>; devel@edk2.groups.io > 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/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 <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(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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
