From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 29E31223C1792 for ; Tue, 20 Feb 2018 08:29:15 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DBC3740FB646; Tue, 20 Feb 2018 16:35:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-24.rdu2.redhat.com [10.10.120.24]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2F207AE7A8; Tue, 20 Feb 2018 16:35:11 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: ruiyu.ni@intel.com, michael.d.kinney@intel.com, liming.gao@intel.com, leif.lindholm@linaro.org, star.zeng@intel.com, afish@apple.com References: <20180220110524.9050-1-ard.biesheuvel@linaro.org> <20180220110524.9050-2-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <24fadcfa-31f4-47aa-0557-3129902b6d8e@redhat.com> Date: Tue, 20 Feb 2018 17:35:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180220110524.9050-2-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 20 Feb 2018 16:35:12 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 20 Feb 2018 16:35:12 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Feb 2018 16:29:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/20/18 12:05, Ard Biesheuvel wrote: > Introduce a variant of BaseDebugLibSerialPort that behaves correctly wrt > to use of the serial port after ExitBootServices(). Also, it uses fixed > PCDs for all the parameterized values so that no calls into PcdLib are > made at runtime. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c | 342 ++++++++++++++++++++ > MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf | 46 +++ > MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni | 21 ++ > 3 files changed, 409 insertions(+) > > diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c > new file mode 100644 > index 000000000000..d18267d91322 > --- /dev/null > +++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c > @@ -0,0 +1,342 @@ > +/** @file > + DXE runtime Debug library instance based on Serial Port library. > + It uses PrintLib to send debug messages to serial port device. > + > + NOTE: If the Serial Port library enables hardware flow control, then a call > + to DebugPrint() or DebugAssert() may hang if writes to the serial port are > + being blocked. This may occur if a key(s) are pressed in a terminal emulator > + used to monitor the DEBUG() and ASSERT() messages. > + > + Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.
> + Copyright (c) 2018, Linaro, Ltd. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +STATIC EFI_EVENT mEfiExitBootServicesEvent; > +STATIC BOOLEAN mEfiAtRuntime; > + > +// > +// Define the maximum debug and assert message length that this library supports > +// > +#define MAX_DEBUG_MESSAGE_LENGTH 0x100 > + > +/** > + Set AtRuntime flag as TRUE after ExitBootServices. > + > + @param[in] Event The Event that is being processed. > + @param[in] Context The Event Context. > + > +**/ > +STATIC > +VOID > +EFIAPI > +RuntimeLibExitBootServicesEvent ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + mEfiAtRuntime = TRUE; > +} > + > +/** > + The constructor function initialize the Serial Port Library > + > + @retval EFI_SUCCESS The constructor always returns RETURN_SUCCESS. > + > +**/ > +EFI_STATUS > +EFIAPI > +DxeRuntimeDebugLibSerialPortConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status = SerialPortInitialize (); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return SystemTable->BootServices->CreateEventEx ( > + EVT_NOTIFY_SIGNAL, > + TPL_NOTIFY, > + RuntimeLibExitBootServicesEvent, > + NULL, > + &gEfiEventExitBootServicesGuid, > + &mEfiExitBootServicesEvent); > +} > + > +/** > + Prints a debug message to the debug output device if the specified error level > + is enabled. > + > + If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function > + GetDebugPrintErrorLevel (), then print the message specified by Format and the > + associated variable argument list to the debug output device. > + > + If Format is NULL, then ASSERT(). > + > + @param ErrorLevel The error level of the debug message. > + @param Format Format string for the debug message to print. > + @param ... Variable argument list whose contents are accessed > + based on the format string specified by Format. > + > +**/ > +VOID > +EFIAPI > +DebugPrint ( > + IN UINTN ErrorLevel, > + IN CONST CHAR8 *Format, > + ... > + ) > +{ > + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; > + VA_LIST Marker; > + > + if (mEfiAtRuntime) { > + return; > + } > + > + // > + // Check driver debug mask value and global mask > + // > + if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) { > + return; > + } > + > + // > + // Convert the DEBUG() message to an ASCII String > + // > + VA_START (Marker, Format); > + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker); > + VA_END (Marker); > + > + // > + // Send the print string to a Serial Port > + // > + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); > +} > + > + > +/** > + Prints an assert message containing a filename, line number, and description. > + This may be followed by a breakpoint or a dead loop. > + > + Print a message of the form "ASSERT (): \n" > + to the debug output device. If DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED bit > + of PcdDebugProperyMask is set then CpuBreakpoint() is called. Otherwise, if > + DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is set then > + CpuDeadLoop() is called. If neither of these bits are set, then this function > + returns immediately after the message is printed to the debug output device. > + DebugAssert() must actively prevent recursion. If DebugAssert() is called > + while processing another DebugAssert(), then DebugAssert() must return > + immediately. > + > + If FileName is NULL, then a string of "(NULL) Filename" is printed. > + If Description is NULL, then a string of "(NULL) Description" is > + printed. > + > + @param FileName The pointer to the name of the source file that generated > + the assert condition. > + @param LineNumber The line number in the source file that generated the > + assert condition > + @param Description The pointer to the description of the assert condition. > + > +**/ > +VOID > +EFIAPI > +DebugAssert ( > + IN CONST CHAR8 *FileName, > + IN UINTN LineNumber, > + IN CONST CHAR8 *Description > + ) > +{ > + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; > + > + if (!mEfiAtRuntime) { > + // > + // Generate the ASSERT() message in Ascii format > + // > + AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT [%a] %a(%d): %a\n", > + gEfiCallerBaseName, FileName, LineNumber, Description); > + > + // > + // Send the print string to the Console Output device > + // > + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); > + } > + > + // > + // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings > + // > + if ((FixedPcdGet8 (PcdDebugPropertyMask) & > + DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) { > + CpuBreakpoint (); > + } else if ((FixedPcdGet8 (PcdDebugPropertyMask) & > + DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) { > + CpuDeadLoop (); > + } > +} > + > + > +/** > + Fills a target buffer with PcdDebugClearMemoryValue, and returns the target > + buffer. > + > + This function fills Length bytes of Buffer with the value specified by > + PcdDebugClearMemoryValue, and returns Buffer. > + > + If Buffer is NULL, then ASSERT(). > + If Length is greater than (MAX_ADDRESS - Buffer + 1), then ASSERT(). > + > + @param Buffer The pointer to the target buffer to be filled with > + PcdDebugClearMemoryValue. > + @param Length The number of bytes in Buffer to fill with > + PcdDebugClearMemoryValue. > + > + @return Buffer The pointer to the target buffer filled with > + PcdDebugClearMemoryValue. > + > +**/ > +VOID * > +EFIAPI > +DebugClearMemory ( > + OUT VOID *Buffer, > + IN UINTN Length > + ) > +{ > + // > + // SetMem() checks for the the ASSERT() condition on Length and returns Buffer > + // > + return SetMem (Buffer, Length, FixedPcdGet8 (PcdDebugClearMemoryValue)); > +} > + > + > +/** > + Returns TRUE if ASSERT() macros are enabled. > + > + This function returns TRUE if the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of > + PcdDebugProperyMask is set. Otherwise FALSE is returned. > + > + @retval TRUE The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of > + PcdDebugProperyMask is set. > + @retval FALSE The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of > + PcdDebugProperyMask is clear. > + > +**/ > +BOOLEAN > +EFIAPI > +DebugAssertEnabled ( > + VOID > + ) > +{ > + return (FixedPcdGet8 (PcdDebugPropertyMask) & > + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0; > +} > + > + > +/** > + Returns TRUE if DEBUG() macros are enabled. > + > + This function returns TRUE if the DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of > + PcdDebugProperyMask is set. Otherwise FALSE is returned. > + > + @retval TRUE The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of > + PcdDebugProperyMask is set. > + @retval FALSE The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of > + PcdDebugProperyMask is clear. > + > +**/ > +BOOLEAN > +EFIAPI > +DebugPrintEnabled ( > + VOID > + ) > +{ > + return (FixedPcdGet8 (PcdDebugPropertyMask) & > + DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0; > +} > + > + > +/** > + Returns TRUE if DEBUG_CODE() macros are enabled. > + > + This function returns TRUE if the DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of > + PcdDebugProperyMask is set. Otherwise FALSE is returned. > + > + @retval TRUE The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of > + PcdDebugProperyMask is set. > + @retval FALSE The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of > + PcdDebugProperyMask is clear. > + > +**/ > +BOOLEAN > +EFIAPI > +DebugCodeEnabled ( > + VOID > + ) > +{ > + return (FixedPcdGet8 (PcdDebugPropertyMask) & > + DEBUG_PROPERTY_DEBUG_CODE_ENABLED) != 0; > +} > + > + > +/** > + Returns TRUE if DEBUG_CLEAR_MEMORY() macro is enabled. > + > + This function returns TRUE if the DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of > + PcdDebugProperyMask is set. Otherwise FALSE is returned. > + > + @retval TRUE The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of > + PcdDebugProperyMask is set. > + @retval FALSE The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of > + PcdDebugProperyMask is clear. > + > +**/ > +BOOLEAN > +EFIAPI > +DebugClearMemoryEnabled ( > + VOID > + ) > +{ > + return (FixedPcdGet8 (PcdDebugPropertyMask) & > + DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED) != 0; > +} > + > +/** > + Returns TRUE if any one of the bit is set both in ErrorLevel and > + PcdFixedDebugPrintErrorLevel. > + > + This function compares the bit mask of ErrorLevel and > + PcdFixedDebugPrintErrorLevel. > + > + @retval TRUE Current ErrorLevel is supported. > + @retval FALSE Current ErrorLevel is not supported. > + > +**/ > +BOOLEAN > +EFIAPI > +DebugPrintLevelEnabled ( > + IN CONST UINTN ErrorLevel > + ) > +{ > + return (ErrorLevel & FixedPcdGet32 (PcdFixedDebugPrintErrorLevel)) != 0; > +} > diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf > new file mode 100644 > index 000000000000..9f300f4f1b12 > --- /dev/null > +++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf > @@ -0,0 +1,46 @@ > +## @file > +# > +# Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
> +# Copyright (c) 2018, Linaro, Ltd. All rights reserved.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php. > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = DxeRuntimeDebugLibSerialPort > + MODULE_UNI_FILE = DxeRuntimeDebugLibSerialPort.uni > + FILE_GUID = 9D914E2F-7CCB-41DB-8E74-9AFF8F3BBFBF > + MODULE_TYPE = DXE_RUNTIME_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = DebugLib > + CONSTRUCTOR = DxeRuntimeDebugLibSerialPortConstructor > + > +[Sources] > + DebugLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + BaseLib > + BaseMemoryLib > + DebugPrintErrorLevelLib > + PcdLib > + PrintLib > + SerialPortLib > + > +[Guids] > + gEfiEventExitBootServicesGuid > + > +[FixedPcd] > + gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue ## SOMETIMES_CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES > diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni > new file mode 100644 > index 000000000000..cd65515c4177 > --- /dev/null > +++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni > @@ -0,0 +1,21 @@ > +// /** @file > +// Instance of Debug Library based on Serial Port Library. > +// > +// It uses Print Library to produce formatted output strings to seiral port device. > +// > +// Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> +// > +// This program and the accompanying materials > +// are licensed and made available under the terms and conditions of the BSD License > +// which accompanies this distribution. The full text of the license may be found at > +// http://opensource.org/licenses/bsd-license.php. > +// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +// WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +// > +// **/ > + > + > +#string STR_MODULE_ABSTRACT #language en-US "Instance of Debug Library based on Serial Port Library" > + > +#string STR_MODULE_DESCRIPTION #language en-US "It uses Print Library to produce formatted output strings to a serial port device." > + > (1) The "raison d'etre" of this library instance is spelled out well in the commit message, but it's not reflected in the files: - The C file has a @file comment, but it's not (fully) up-to-date. - The INF file has an empty @file comment. - The UNI file has a @file comment, but both that comment and the STR_* entries are out of date (IMO). (2) I think it would be nice to keep VALID_ARCHITECTURES in the INF file. (If you disagree, that's fine as well.) (3) The LIBRARY_CLASS definition in the INF file should be restricted as in: "DebugLib|DXE_RUNTIME_DRIVER". The MODULE_TYPE definition does not effect this restriction, it only determines the constructor and destructor signatures for the library instance. (4) I think gEfiEventExitBootServicesGuid should be marked as "## CONSUMES" in the INF file. (5) Beyond having a constructor, this library instance needs a destructor as well -- if a client runtime DXE driver exits its entry point function with an error, then it is unloaded, so we have to close the event in the lib instance's destructor function. See the oddly named RuntimeDriverLibDeconstruct() function, for example, in "MdePkg/Library/UefiRuntimeLib/RuntimeLib.c". (6) the name "RuntimeLibExitBootServicesEvent" comes from UefiRuntimeLib, I think we should find a better ("more local") name for our function here. What about just "ExitBootServicesNotify"? (7) The trailing (closing) paren after CreateEventEx() is not idiomatic. (8) In DebugPrint(), I think it would make sense to preserve "ASSERT (Format != NULL)"; at least if we continue to claim in the leading comment, "If Format is NULL, then ASSERT()". (BTW this claim comes from the lib class header, so we might have no choice here.) (9) The same as (8) applies to DebugClearMemory(). (10) I suggest to keep the (BOOLEAN) cast in DebugPrintLevelEnabled(), from the original. While it is terrible style, some VS compilers yell that an "INT" (the result of the != operator) is converted to a UINT8 (i.e. BOOLEAN), causing possible loss of precision, or some such. :/ (11) I see there's a bunch of cleanup going on as well, such as stripping trailing whitespace and rewrapping. I think that's great, but: it should be first done to BaseDebugLibSerialPort, and the DxeRuntimeDebugLibSerialPort instance should only contain the functional changes on top. In my opinion anyway. (12) In the commit message, "wrt to" should be replaced with "wrt the". Before doing any of the above work, I suggest we await feedback from the MdePkg maintainers. Also, please feel free to skip those comments from my side that you disagree with. Thank you for the patch! Laszlo