Two cosmetic comments:
On 06/06/16 12:38, Star Zeng wrote:
> PiDxeS3BootScriptLib has a constructor S3BootScriptLibInitialize() that
> registers ready-to-lock callback S3BootScriptSmmEventCallBack() and several
> more. The library is linked to SMM modules. If the module entry-point
> function returns error (because of lack of resources, unsupported,
> whatever), the module will be unloaded and the notify callback pointers
> will point to undefined memory. On ready-to-lock exception occurs when
> calling S3BootScriptSmmEventCallBack(), and probably all the other
> callbacks registered by the constructor would also cause exception.
>
> This patch is to implement library Destructor to free the resources
> allocated by S3BootScriptLibInitialize() and unregister callbacks.
>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.z...@intel.com>
> Reviewed-by: Jiewen Yao <jiewen....@intel.com>
> ---
> .../Library/PiDxeS3BootScriptLib/BootScriptSave.c | 155
> ++++++++++++++++-----
> .../PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf | 3 +-
> 2 files changed, 124 insertions(+), 34 deletions(-)
>
> diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
> index d954503f864d..e84195ded547 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 - 2015, Intel Corporation. All rights reserved.<BR>
> + Save the S3 data to S3 boot script.
> +
> + Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions
> @@ -124,6 +124,14 @@ EFI_GUID
> mBootScriptSmmPrivateDataGuid = {
> 0x627ee2da, 0x3bf9, 0x439b, { 0x92, 0x9f, 0x2e, 0xe, 0x6e, 0x9d, 0xba,
> 0x62 }
> };
>
> +EFI_EVENT mEventDxeSmmReadyToLock = NULL;
> +VOID *mRegistrationSmmExitBootServices = NULL;
> +VOID *mRegistrationSmmLegacyBoot = NULL;
> +VOID *mRegistrationSmmReadyToLock = NULL;
> +BOOLEAN mS3BootScriptTableAllocated = FALSE;
> +BOOLEAN mS3BootScriptTableSmmAllocated = FALSE;
> +EFI_SMM_SYSTEM_TABLE2 *mSmst = NULL;
> +
> /**
> This is an internal function to add a terminate node the entry,
> recalculate the table
> length and fill into the table.
> @@ -417,8 +425,8 @@ S3BootScriptSmmAtRuntimeCallBack (
> @param ImageHandle The firmware allocated handle for the EFI image.
> @param SystemTable A pointer to the EFI System Table.
>
> - @retval RETURN_SUCCESS Allocate the global memory space to
> store S3 boot script table private data
> - @retval RETURN_OUT_OF_RESOURCES No enough memory to allocated.
> + @retval RETURN_SUCCESS The constructor always returns EFI_SUCCESS.
> +
> **/
> RETURN_STATUS
> EFIAPI
> @@ -433,9 +441,7 @@ S3BootScriptLibInitialize (
> VOID *Registration;
> 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;
> @@ -453,25 +459,24 @@ S3BootScriptLibInitialize (
> EFI_SIZE_TO_PAGES(sizeof(SCRIPT_TABLE_PRIVATE_DATA)),
> &Buffer
> );
> - if (EFI_ERROR (Status)) {
> - return RETURN_OUT_OF_RESOURCES;
> - }
> + ASSERT_EFI_ERROR (Status);
> + mS3BootScriptTableAllocated = TRUE;
> S3TablePtr = (VOID *) (UINTN) Buffer;
>
> Status = PcdSet64S (PcdS3BootScriptTablePrivateDataPtr, (UINT64)
> (UINTN)S3TablePtr);
> ASSERT_EFI_ERROR (Status);
> - ZeroMem (S3TablePtr, sizeof(SCRIPT_TABLE_PRIVATE_DATA));
> + ZeroMem (S3TablePtr, sizeof(SCRIPT_TABLE_PRIVATE_DATA));
> //
> // Create event to notify the library system enter the SmmLocked phase.
> //
> - Event = EfiCreateProtocolNotifyEvent (
> - &gEfiDxeSmmReadyToLockProtocolGuid,
> - TPL_CALLBACK,
> - S3BootScriptEventCallBack,
> - NULL,
> - &Registration
> - );
> - ASSERT (Event != NULL);
> + mEventDxeSmmReadyToLock = EfiCreateProtocolNotifyEvent (
> + &gEfiDxeSmmReadyToLockProtocolGuid,
> + TPL_CALLBACK,
> + S3BootScriptEventCallBack,
> + NULL,
> + &Registration
> + );
> + ASSERT (mEventDxeSmmReadyToLock != NULL);
> }
> mS3BootScriptTablePtr = S3TablePtr;
>
> @@ -492,7 +497,7 @@ S3BootScriptLibInitialize (
> //
> // Good, we are in SMM
> //
> - Status = SmmBase2->GetSmstLocation (SmmBase2, &Smst);
> + Status = SmmBase2->GetSmstLocation (SmmBase2, &mSmst);
> if (EFI_ERROR (Status)) {
> return RETURN_SUCCESS;
> }
> @@ -502,14 +507,13 @@ S3BootScriptLibInitialize (
> // The Boot script private data in SMM is not be initialized. create it
> //
> if (S3TableSmmPtr == 0) {
> - Status = Smst->SmmAllocatePool (
> + Status = mSmst->SmmAllocatePool (
> EfiRuntimeServicesData,
> sizeof(SCRIPT_TABLE_PRIVATE_DATA),
> (VOID **) &S3TableSmmPtr
> );
> - if (EFI_ERROR (Status)) {
> - return RETURN_OUT_OF_RESOURCES;
> - }
> + ASSERT_EFI_ERROR (Status);
> + mS3BootScriptTableSmmAllocated = TRUE;
>
> Status = PcdSet64S (PcdS3BootScriptTablePrivateSmmDataPtr, (UINT64)
> (UINTN)S3TableSmmPtr);
> ASSERT_EFI_ERROR (Status);
> @@ -518,19 +522,17 @@ S3BootScriptLibInitialize (
> //
> // Register SmmExitBootServices and SmmLegacyBoot notification.
> //
> - Registration = NULL;
> - Status = Smst->SmmRegisterProtocolNotify (
> + Status = mSmst->SmmRegisterProtocolNotify (
> &gEdkiiSmmExitBootServicesProtocolGuid,
> S3BootScriptSmmAtRuntimeCallBack,
> - &Registration
> + &mRegistrationSmmExitBootServices
> );
> ASSERT_EFI_ERROR (Status);
>
> - Registration = NULL;
> - Status = Smst->SmmRegisterProtocolNotify (
> + Status = mSmst->SmmRegisterProtocolNotify (
> &gEdkiiSmmLegacyBootProtocolGuid,
> S3BootScriptSmmAtRuntimeCallBack,
> - &Registration
> + &mRegistrationSmmLegacyBoot
> );
> ASSERT_EFI_ERROR (Status);
> }
> @@ -539,16 +541,103 @@ S3BootScriptLibInitialize (
> //
> // Register SmmReadyToLock notification.
> //
> - Registration = NULL;
> - Status = Smst->SmmRegisterProtocolNotify (
> + Status = mSmst->SmmRegisterProtocolNotify (
> &gEfiSmmReadyToLockProtocolGuid,
> S3BootScriptSmmEventCallBack,
> - &Registration
> + &mRegistrationSmmReadyToLock
> );
> ASSERT_EFI_ERROR (Status);
>
> return RETURN_SUCCESS;
> }
> +
> +/**
> + Library Destructor to free the resources allocated by
> + S3BootScriptLibInitialize() and unregister callbacks.
> +
> + NOTICE: The desturctor doesn't support unloading as a separate action, and
> it
(1) "desturctor" above is a typo, please fix it up before committing the
patch.
> + only supports unloading if the containing driver's entry point function
> fails.
> +
> + @param ImageHandle The firmware allocated handle for the EFI image.
> + @param SystemTable A pointer to the EFI System Table.
> +
> + @retval RETURN_SUCCESS The decstructor always returns EFI_SUCCESS.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +S3BootScriptLibDeinitialize (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> +
> + DEBUG ((EFI_D_INFO, "%a() in %a module\n", __FUNCTION__,
> gEfiCallerBaseName));
> +
> + if (mEventDxeSmmReadyToLock != NULL) {
> + //
> + // Close the DxeSmmReadyToLock event.
> + //
> + Status = gBS->CloseEvent (mEventDxeSmmReadyToLock);
> + ASSERT_EFI_ERROR (Status);
> + }
> +
> + if (mSmst != NULL) {
> + if (mRegistrationSmmExitBootServices != NULL) {
> + //
> + // Unregister SmmExitBootServices notification.
> + //
> + Status = mSmst->SmmRegisterProtocolNotify (
> + &gEdkiiSmmExitBootServicesProtocolGuid,
> + NULL,
> + &mRegistrationSmmExitBootServices
> + );
> + ASSERT_EFI_ERROR (Status);
> + }
> + if (mRegistrationSmmLegacyBoot != NULL) {
> + //
> + // Unregister SmmLegacyBoot notification.
> + //
> + Status = mSmst->SmmRegisterProtocolNotify (
> + &gEdkiiSmmLegacyBootProtocolGuid,
> + NULL,
> + &mRegistrationSmmLegacyBoot
> + );
> + ASSERT_EFI_ERROR (Status);
> + }
> + if (mRegistrationSmmReadyToLock != NULL) {
> + //
> + // Unregister SmmReadyToLock notification.
> + //
> + Status = mSmst->SmmRegisterProtocolNotify (
> + &gEfiSmmReadyToLockProtocolGuid,
> + NULL,
> + &mRegistrationSmmReadyToLock
> + );
> + ASSERT_EFI_ERROR (Status);
> + }
> + }
> +
> + //
> + // Free the resources allocated and set PCDs to 0.
> + //
> + if (mS3BootScriptTableAllocated) {
> + Status = gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN)
> mS3BootScriptTablePtr, EFI_SIZE_TO_PAGES(sizeof(SCRIPT_TABLE_PRIVATE_DATA)));
> + ASSERT_EFI_ERROR (Status);
> + Status = PcdSet64S (PcdS3BootScriptTablePrivateDataPtr, 0);
> + ASSERT_EFI_ERROR (Status);
> + }
> + if (mS3BootScriptTableSmmAllocated) {
> + Status = mSmst->SmmFreePool ((VOID *) mS3BootScriptTableSmmPtr);
(2) I think the cast to (VOID *) is not necessary.
"mS3BootScriptTableSmmPtr" already has a pointer type:
(SCRIPT_TABLE_PRIVATE_DATA*), hence the argument should automatically be
converted to (VOID*).
With those updated:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks!
Laszlo
> + ASSERT_EFI_ERROR (Status);
> + Status = PcdSet64S (PcdS3BootScriptTablePrivateSmmDataPtr, 0);
> + ASSERT_EFI_ERROR (Status);
> + }
> +
> + return RETURN_SUCCESS;
> +}
> +
> /**
> To get the start address from which a new boot time s3 boot script entry
> will write into.
> If the table is not exist, the functio will first allocate a buffer for
> the table
> diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
> index 4e0919ea2c79..0f4151180f6a 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 - 2015, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> #
> # This program and the accompanying materials are
> # licensed and made available under the terms and conditions of the BSD
> License
> @@ -24,6 +24,7 @@ [Defines]
>
>
> CONSTRUCTOR = S3BootScriptLibInitialize
> + DESTRUCTOR = S3BootScriptLibDeinitialize
>
> #
> # The following information is for reference only and not required by the
> build tools.
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
