| Message ID | 1461784849-30809-2-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, April 27, 2016 12:21 PM >> To: edk2-devel-01 <edk2-devel@ml01.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; Tian, Feng <feng.tian@intel.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; Zeng, Star <star.zeng@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.tian@intel.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >> Cc: Star Zeng <star.zeng@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@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
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.carsey@intel.com> Thank you! Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Wednesday, April 27, 2016 1:12 PM >> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2- >> devel@ml01.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L >> <jordan.l.justen@intel.com>; Tian, Feng <feng.tian@intel.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; Zeng, Star <star.zeng@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-bounces@lists.01.org] On Behalf Of >>>> Laszlo Ersek >>>> Sent: Wednesday, April 27, 2016 12:21 PM >>>> To: edk2-devel-01 <edk2-devel@ml01.01.org> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L >>>> <jordan.l.justen@intel.com>; Tian, Feng <feng.tian@intel.com>; Yao, >> Jiewen >>>> <jiewen.yao@intel.com>; Zeng, Star <star.zeng@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.tian@intel.com> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>>> Cc: Star Zeng <star.zeng@intel.com> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Laszlo Ersek <lersek@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
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.PcdS3BootScriptRuntimeTableReservePageNumber ## 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(PcdS3BootScriptTablePrivateDataPtr); // // The Boot script private data is not be initialized. create it // if (S3TablePtr == 0) { Buffer = SIZE_4GB - 1;
In the edk2 tree, there are currently four drivers that consume PcdAcpiS3Enable: IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.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.tian@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf | 1 + MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c | 4 ++++ 2 files changed, 5 insertions(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel