On 04/27/16 21:45, Carsey, Jaben wrote: > Laszlo, > > Does the library destructor not get called? Shouldn't that destructor > unregister the protocol notify and leave the callback pointer in DXE core > correct?
PiDxeS3BootScriptLib doesn't have a DESTRUCTOR at the moment. I would prefer not attempting to add a destructor from scratch. The constructor does quite a bit of work that would all have to be undone in the destructor. The commit message here only points out the protocol notify, but in fact S3BootScriptLibInitialize() implements a "shared responsibility memory allocation" as well, through PcdS3BootScriptTablePrivateDataPtr. All modules that are linked against this library instance, and are dispatched through the same boot, are prepared to "be the first" and allocate the region. The guys that come second and later just reuse the area. Undoing this is messy; I think it would require reference counting (in another PCD, or in the data structure pointed-to by the current PCD). The constructor also has some sophisticated logic for determining if it is running as part of a DXE_SMM_DRIVER module, or just as part of a DXE_(RUNTIME_)DRIVER module. Even without my patch, the constructor has five (5) RETURN_SUCCESS exit points. Rolling back all those (valid) library states in a destructor would be a nightmare. I think that DESTRUCTOR is a clever facility of edk2, but similarly to how a DXE_DRIVER must be designed from the ground up for unloading (UNLOAD_IMAGE=... in the INF file), a library instance must also be designed from the ground up if it is to have a DESTRUCTOR. I think that none of BootScriptExecutorDxe, S3SaveStateDxe, SmmS3SaveState, and PiDxeS3BootScriptLib qualify. (I do see that the shell command libraries use DESTRUCTORs. That makes perfect sense: the shell binary that they are linked into with NULL library resolution is a UEFI application (not a driver), which is by definition unload-capable. Hence I even believe that for the shell command libs, DESTRUCTORs are actually unavoidable.) Thanks! Laszlo >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, April 27, 2016 12:21 PM >> To: edk2-devel-01 <edk2-de...@ml01.01.org> >> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L >> <jordan.l.jus...@intel.com>; Tian, Feng <feng.t...@intel.com>; Yao, Jiewen >> <jiewen....@intel.com>; Zeng, Star <star.z...@intel.com> >> Subject: [edk2] [PATCH 1/3] MdeModulePkg: PiDxeS3BootScriptLib: honor >> PcdAcpiS3Enable >> >> In the edk2 tree, there are currently four drivers that consume >> PcdAcpiS3Enable: >> >> >> IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.in >> f >> >> MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorD >> xe.inf >> MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf >> MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.inf >> >> From these, AcpiS3SaveDxe is the only one that isn't also a client of the >> S3BootScriptLib class; all the others (BootScriptExecutorDxe, >> S3SaveStateDxe, SmmS3SaveState) are clients of the S3BootScriptLib class. >> >> In turn, the edk2 tree contains only one non-Null instance of the >> S3BootScriptLib class: >> >> MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> >> Therefore we can safely state that BootScriptExecutorDxe, S3SaveStateDxe, >> and SmmS3SaveState are all linked against PiDxeS3BootScriptLib. >> >> Now, if PcdAcpiS3Enable is FALSE when either of BootScriptExecutorDxe, >> SmmS3SaveState, or SmmS3SaveState is dispatched, then the following >> happens: >> >> - The constructor of PiDxeS3BootScriptLib, function >> S3BootScriptLibInitialize(), registers a protocol installation callback >> for gEfiDxeSmmReadyToLockProtocolGuid. Namely, the function >> S3BootScriptEventCallBack(). >> >> - The driver immediately exits with EFI_UNSUPPORTED from its entry point >> function, upon seeing PcdAcpiS3Enable == FALSE. (See commits >> 800c02fbe2da6, 125e093876414, and d2d38610603f6.) >> >> - This leaves a dangling callback pointer in the DXE core. >> >> - When Platform BDS installs gEfiDxeSmmReadyToLockProtocolGuid (which is >> a >> valid thing to do for locking down SMM, even in the absence of S3 >> support!), things blow up. >> >> Fix this issue by returning immediately from S3BootScriptLibInitialize() >> if PcdAcpiS3Enable is FALSE -- it is useless to initialize the library >> instance if the containing driver module exits first thing in its entry >> point. >> >> Cc: Feng Tian <feng.t...@intel.com> >> Cc: Jiewen Yao <jiewen....@intel.com> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Ruiyu Ni <ruiyu...@intel.com> >> Cc: Star Zeng <star.z...@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >> --- >> MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf | 1 + >> MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c | 4 ++++ >> 2 files changed, 5 insertions(+) >> >> diff --git >> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> index 6b7540a5eab9..4e0919ea2c79 100644 >> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> @@ -67,7 +67,8 @@ [Pcd] >> ## SOMETIMES_PRODUCES >> gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptTablePrivateDataPtr >> ## CONSUMES >> ## SOMETIMES_PRODUCES >> >> gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptTablePrivateSmmDataPtr >> >> gEfiMdeModulePkgTokenSpaceGuid.PcdS3BootScriptRuntimeTableReservePa >> geNumber ## CONSUMES >> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable >> ## >> CONSUMES >> >> diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> index e7d2a2492e84..d954503f864d 100644 >> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> @@ -434,12 +434,16 @@ S3BootScriptLibInitialize ( >> EFI_SMM_BASE2_PROTOCOL *SmmBase2; >> BOOLEAN InSmm; >> EFI_SMM_SYSTEM_TABLE2 *Smst; >> EFI_PHYSICAL_ADDRESS Buffer; >> EFI_EVENT Event; >> >> + if (!PcdGetBool (PcdAcpiS3Enable)) { >> + return RETURN_SUCCESS; >> + } >> + >> S3TablePtr = >> (SCRIPT_TABLE_PRIVATE_DATA*)(UINTN)PcdGet64(PcdS3BootScriptTablePriv >> ateDataPtr); >> // >> // The Boot script private data is not be initialized. create it >> // >> if (S3TablePtr == 0) { >> Buffer = SIZE_4GB - 1; >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel