From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.128.50, mailfrom: philmd@redhat.com) Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by groups.io with SMTP; Mon, 29 Jul 2019 10:48:53 -0700 Received: by mail-wm1-f50.google.com with SMTP id s3so54631737wms.2 for ; Mon, 29 Jul 2019 10:48:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=lBXl1r80idfhLvrqbCCNFvjPrUhRhtqO+fNMUcfLfwg=; b=FzgsFDlfrSBqTO/3+PRS9HK3S75vgF9idv1hN902C7yBvbLVOUKqFXBpdVxuqKZVVi erJW+2wVJijMZO5tXJskkkS1ma8FocD6Cd6x2m33Sb+Eaw5qkewtQ5naqb3XY3CbLkdc KNMuEPPTosVxXjV9LLuyF+9D48VLrbafBe7pZphsvCLJS3AkpTCoNN0Mw1cu8hLYPmnA chn882e9TRQn5HTGx7tfE9y23cdsRPT83O2YWsUQNKyzDQLgBYp9jmh9GcyCmlDyc/a5 FtNPbebP3mq5wOF1MUilH/JYTiMLXkVCp5v9Lx+a5Cveoj9wL+X1yLZBTisO+10LjxVb XrHg== X-Gm-Message-State: APjAAAUuUOnvROrwb6v3qZA2eYFg/T4x/rd57sj+We8JKV4y6/pX7sRg bvpVU6ZN6HPrQkLNK4kQnaQcJw== X-Google-Smtp-Source: APXvYqzvwEO13Pm01hRGD4WuYrMDZ6fLVcT7aFaaA2SRQeRONM+qZBGiu/bbvMB2owG9W3979o8vHA== X-Received: by 2002:a1c:e341:: with SMTP id a62mr42924595wmh.165.1564422531359; Mon, 29 Jul 2019 10:48:51 -0700 (PDT) Return-Path: Received: from [192.168.1.38] (190.red-81-40-121.staticip.rima-tde.net. [81.40.121.190]) by smtp.gmail.com with ESMTPSA id f12sm67900003wrg.5.2019.07.29.10.48.50 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 29 Jul 2019 10:48:50 -0700 (PDT) Subject: Re: [edk2-devel][Patch 1/3] MdePkg/UefiDebugLibConOut: Add destructor to CloseEvent To: Laszlo Ersek , devel@edk2.groups.io, 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> <5ea2f46c-d7ab-fc45-8a0b-f8e99eee9cc8@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: <32f0f2d5-61ee-c0a7-c9ae-68020bc50420@redhat.com> Date: Mon, 29 Jul 2019 19:48:49 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <5ea2f46c-d7ab-fc45-8a0b-f8e99eee9cc8@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 7/29/19 6:09 PM, Laszlo Ersek wrote: > On 07/29/19 16:38, Philippe Mathieu-Daudé wrote: >> Hi, >> >> On 7/26/19 5:10 AM, Xu, Wei6 wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2012 >>> >>> 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 image. >>> + @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 != NULL) { >>> + SystemTable->BootServices->CloseEvent (mExitBootServicesEvent); >>> + } >> >> Is it OK to let mDebugST (pointer to SystemTable) initialized? > > Yes, ignoring "mDebugST" in this function should be. > > "mDebugST" is a global variable (= 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 for the explanations! I noticed (late) this series is already pushed (commits e92bdcb3ecbf..28bc6992400) so it doesn't require review anymore. BTW Liming the author name seems incorrect: Wei6 Xu and Xu, Wei6. >>> + >>> + 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 = UEFI_DRIVER >>> VERSION_STRING = 1.0 >>> LIBRARY_CLASS = DebugLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER >>> >>> CONSTRUCTOR = DxeDebugLibConstructor >>> + DESTRUCTOR = DxeDebugLibDestructor >>> >>> # >>> # VALID_ARCHITECTURES = IA32 X64 EBC >>> # >>> >>> >> >> >> >