From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 26 Sep 2019 06:57:50 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BCD0330BBE87; Thu, 26 Sep 2019 13:57:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id CB35B5D6B2; Thu, 26 Sep 2019 13:57:47 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg: Enable/Disable S3BootScript dynamically. To: "Chiu, Chasel" , "devel@edk2.groups.io" Cc: "Wu, Hao A" , "Dong, Eric" , "Desimone, Nathaniel L" , "Gao, Liming" References: <20190925092135.16664-1-chasel.chiu@intel.com> <3C3EFB470A303B4AB093197B6777CCEC504E2AEC@PGSMSX112.gar.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 26 Sep 2019 15:57:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3C3EFB470A303B4AB093197B6777CCEC504E2AEC@PGSMSX112.gar.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Thu, 26 Sep 2019 13:57:49 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Sent: Thursday, September 26, 2019 2:58 AM >> To: devel@edk2.groups.io; Chiu, Chasel >> Cc: Wu, Hao A ; Dong, Eric ; >> Desimone, Nathaniel L ; Gao, Liming >> >> 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 >>> Cc: Eric Dong >>> Cc: Nate DeSimone >>> Cc: Liming Gao >>> Cc: Laszlo Ersek >>> Signed-off-by: Chasel Chiu >>> --- >>> 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.
>>> + Copyright (c) 2006 - 2019, Intel Corporation. All rights >>> + reserved.
>>> >>> 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.
>>> +# Copyright (c) 2006 - 2019, Intel Corporation. All rights >>> +reserved.
>>> # >>> # 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