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; Mon, 29 Jul 2019 09:09:51 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 12837285AE; Mon, 29 Jul 2019 16:09:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-240.ams2.redhat.com [10.36.117.240]) by smtp.corp.redhat.com (Postfix) with ESMTP id 58D256012A; Mon, 29 Jul 2019 16:09:47 +0000 (UTC) Subject: Re: [edk2-devel][Patch 1/3] MdePkg/UefiDebugLibConOut: Add destructor to CloseEvent To: devel@edk2.groups.io, philmd@redhat.com, wei6.xu@intel.com Cc: Michael D Kinney , Liming Gao References: <20190726031055.10020-1-wei6.xu@intel.com> <20190726031055.10020-2-wei6.xu@intel.com> From: "Laszlo Ersek" Message-ID: <5ea2f46c-d7ab-fc45-8a0b-f8e99eee9cc8@redhat.com> Date: Mon, 29 Jul 2019 18:09: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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 29 Jul 2019 16:09:51 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/29/19 16:38, Philippe Mathieu-Daud=C3=A9 wrote: > Hi, >=20 > On 7/26/19 5:10 AM, Xu, Wei6 wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2012 >> >> When driver is unloaded, the ExitBootSerivesEvent must be closed at >> the same time. Otherwise exception will occur when ExitBootServices. >> >> Cc: Michael D Kinney >> Cc: Liming Gao >> Signed-off-by: Wei6 Xu >> --- >> .../UefiDebugLibConOut/DebugLibConstructor.c | 23 ++++++++++++++= ++++++++ >> .../UefiDebugLibConOut/UefiDebugLibConOut.inf | 1 + >> 2 files changed, 24 insertions(+) >> >> diff --git a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c b/= MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c >> index 8005370372..ed73f92818 100644 >> --- a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c >> +++ b/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c >> @@ -73,5 +73,28 @@ DxeDebugLibConstructor( >> &mExitBootServicesEvent >> ); >> >> return EFI_SUCCESS; >> } >> + >> +/** >> + The destructor closes Exit Boot Services Event. >> + >> + @param ImageHandle The firmware allocated handle for the EFI imag= e. >> + @param SystemTable A pointer to the EFI System Table. >> + >> + @retval EFI_SUCCESS The destructor always returns EFI_SUCCESS. >> + >> +**/ >> +EFI_STATUS >> +EFIAPI >> +DxeDebugLibDestructor( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + if (mExitBootServicesEvent !=3D NULL) { >> + SystemTable->BootServices->CloseEvent (mExitBootServicesEvent); >> + } >=20 > Is it OK to let mDebugST (pointer to SystemTable) initialized? Yes, ignoring "mDebugST" in this function should be. "mDebugST" is a global variable (=3D an object with file scope and static storage duration) defined in "MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c". The library instance (for which the destructor function is being added) is linked into driver and application modules. The destructor function is invoked whenever the driver or application is about to be unloaded from memory. As part of the unloading, the ExitBootServices() notification function -- which was automatically registered via the constructor function when the driver or application started up -- must be de-registered, otherwise the platform firmware will be left with a dangling callback pointer. That's what the CloseEvent() above takes care of. But it's OK to ignore "mDebugST" altogether, as the memory that "mDebugST" lives inside is about to be reclaimed by the platform firmware anyway. The library destructor is invoked: - when a UEFI application exits (with success or failure), by returning from its entry point function, or by calling the Exit() boot service - when a UEFI driver exits with failure (hence it will be unloaded automatically) - when a UEFI driver exited with success (hence staying resident), and it supports unloading, and now another agent is unloading it with the UnloadImage() boot service (such as the UEFI shell with the "unload" command, IIRC). Thanks Laszlo >=20 >> + >> + return EFI_SUCCESS; >> +} >> diff --git a/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf b= /MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf >> index 4c279a5bf2..b577d52ac6 100644 >> --- a/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf >> +++ b/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf >> @@ -20,10 +20,11 @@ >> MODULE_TYPE =3D UEFI_DRIVER >> VERSION_STRING =3D 1.0 >> LIBRARY_CLASS =3D DebugLib|DXE_CORE DXE_DRIVER DXE_= RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER >> >> CONSTRUCTOR =3D DxeDebugLibConstructor >> + DESTRUCTOR =3D DxeDebugLibDestructor >> >> # >> # VALID_ARCHITECTURES =3D IA32 X64 EBC >> # >> >> >=20 >=20 >=20