On 04/28/16 01:19, Carsey, Jaben wrote: > thanks for the explanation. > > I agree that libraries must be designed for this feature from the > beginning. I also know that people use libraries for purposes other > than their original intent. I think it would be best to plan for > this by having properly coded destructors in libraries whenever > possible. > > I mean it's a lot harder to use a DXE/PEI DRIVER in an application or > unloadable driver than it is to link to a library. I just think that > libraries should inherently be self-contained.
I agree. I'll keep that in mind for any further library instances we might write. > That does not mean that you should add one in this case though... > > Reviewed-by: Jaben Carsey <jaben.car...@intel.com> Thank you! Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Wednesday, April 27, 2016 1:12 PM >> To: Carsey, Jaben <jaben.car...@intel.com>; 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: Re: [edk2] [PATCH 1/3] MdeModulePkg: PiDxeS3BootScriptLib: >> honor PcdAcpiS3Enable >> Importance: High >> >> 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
