From 4c5d1d1c3c78d4329a550ba6f5d049834d456dcc Mon Sep 17 00:00:00 2001 From: Oliver Smith-Denny Date: Tue, 10 Oct 2023 16:18:32 -0700 Subject: [PATCH] Only call HdwPortWrite if DebugLevel Met (#311) ## Description The DebugLevel is checked twice in the hot path on serial path writes and the HdwPortWrite call is made even if the upper layer knows that the message being logged does not meet the DebugLevel criteria. Closes #309. In order to maintain backwards compatibility, if the LoggerInfo block is found to have a version less than the hardware logging level version, the PCD is checked to decide whether to call HdwPortWrite or not. In SEC, because we may not have the LoggerInfo structure, we check the PCD to see if the message should be logged at this DebugLevel. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Build tested. ## Integration Instructions If a bespoke AdvLoggerHdwPortLib is used, AdvLoggerHdwPortWrite should not check DebugLevel, but simply write the message to the hardware port. Co-authored-by: kuqin12 <42554914+kuqin12@users.noreply.github.com> --- .../AdvancedLoggerHdwPortLib.c | 9 +-------- .../AdvancedLoggerHdwPortLib.inf | 7 +------ .../AdvancedLoggerLib/AdvancedLoggerCommon.c | 17 ++++++++++++++--- .../AdvancedLoggerLib/Dxe/AdvancedLoggerLib.inf | 1 + .../MmCore/AdvancedLoggerLib.inf | 5 ++++- .../AdvancedLoggerLib/Pei/AdvancedLoggerLib.inf | 1 + .../Pei64/AdvancedLoggerLib.inf | 1 + .../Runtime/AdvancedLoggerLib.inf | 1 + .../AdvancedLoggerLib/Sec/AdvancedLoggerLib.inf | 1 + .../SmmCore/AdvancedLoggerLib.inf | 1 + 10 files changed, 26 insertions(+), 18 deletions(-) diff --git a/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.c b/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.c index 7a480e0593..1b18fe1548 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.c @@ -58,12 +58,5 @@ AdvancedLoggerHdwPortWrite ( IN UINTN NumberOfBytes ) { - UINTN NumberReturned; - - NumberReturned = NumberOfBytes; - if (DebugLevel & PcdGet32 (PcdAdvancedLoggerHdwPortDebugPrintErrorLevel)) { - NumberReturned = SerialPortWrite (Buffer, NumberOfBytes); - } - - return NumberReturned; + return SerialPortWrite (Buffer, NumberOfBytes); } diff --git a/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.inf index 25a1b6e26d..1c275a0dac 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerHdwPortLib/AdvancedLoggerHdwPortLib.inf @@ -1,5 +1,5 @@ ## @file -# Advanced Logger Access library. +# Advanced Logger Hardware Port Library. # # Copyright (c) Microsoft Corporation. # @@ -30,10 +30,5 @@ [LibraryClasses] SerialPortLib -[Protocols] - -[Pcd] - gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## CONSUMES - [Depex] TRUE diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c b/AdvLoggerPkg/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c index 2638db2b33..40a829a9fa 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/AdvancedLoggerCommon.c @@ -142,13 +142,24 @@ AdvancedLoggerWrite ( // If LoggerInfo == NULL, assume there is a HdwPort and it has not been disabled. This // does occur in SEC if ((LoggerInfo == NULL) || (!LoggerInfo->HdwPortDisabled)) { + if (DebugLevel & PcdGet32 (PcdAdvancedLoggerHdwPortDebugPrintErrorLevel)) { + AdvancedLoggerHdwPortWrite (DebugLevel, (UINT8 *)Buffer, NumberOfBytes); + } + } + #else if ((LoggerInfo != NULL) && (!LoggerInfo->HdwPortDisabled)) { + // if we are at a high enough version to support HW_LVL logging, only call the HdwPortWrite if this DebugLevel + // is asked to be logged + // if we are at an older version, check the PCD to see if we should log this message if (LoggerInfo->Version >= ADVANCED_LOGGER_HW_LVL_VER) { - DebugLevel = (DebugLevel & LoggerInfo->HwPrintLevel); + if (DebugLevel & LoggerInfo->HwPrintLevel) { + AdvancedLoggerHdwPortWrite (DebugLevel, (UINT8 *)Buffer, NumberOfBytes); + } + } else if (DebugLevel & PcdGet32 (PcdAdvancedLoggerHdwPortDebugPrintErrorLevel)) { + AdvancedLoggerHdwPortWrite (DebugLevel, (UINT8 *)Buffer, NumberOfBytes); } + } #endif - AdvancedLoggerHdwPortWrite (DebugLevel, (UINT8 *)Buffer, NumberOfBytes); - } } diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/Dxe/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/Dxe/AdvancedLoggerLib.inf index 88e663c53c..c8a1133e38 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/Dxe/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/Dxe/AdvancedLoggerLib.inf @@ -37,3 +37,4 @@ gEfiDebugPortProtocolGuid ## CONSUMES [Pcd] + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## SOMETIMES_CONSUMES diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCore/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCore/AdvancedLoggerLib.inf index 5273e39b2e..eac348b636 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCore/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCore/AdvancedLoggerLib.inf @@ -40,5 +40,8 @@ [Guids] gAdvancedLoggerHobGuid +[Pcd] + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## SOMETIMES_CONSUMES + [Depex] - TRUE \ No newline at end of file + TRUE diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei/AdvancedLoggerLib.inf index 4d14cc22b9..347dc0b7b4 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei/AdvancedLoggerLib.inf @@ -36,3 +36,4 @@ gAdvancedLoggerPpiGuid ## CONSUMES [Pcd] + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## SOMETIMES_CONSUMES diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei64/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei64/AdvancedLoggerLib.inf index 9f2cacaef5..5293106094 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei64/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/Pei64/AdvancedLoggerLib.inf @@ -39,3 +39,4 @@ [Pcd] gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerBase ## CONSUMES + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## SOMETIMES_CONSUMES diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/Runtime/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/Runtime/AdvancedLoggerLib.inf index 9e34e04672..eb9f355af1 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/Runtime/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/Runtime/AdvancedLoggerLib.inf @@ -43,3 +43,4 @@ gAdvancedLoggerProtocolGuid ## CONSUMES [Pcd] + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## SOMETIMES_CONSUMES diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/Sec/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/Sec/AdvancedLoggerLib.inf index a5f332126a..976f03d134 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/Sec/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/Sec/AdvancedLoggerLib.inf @@ -45,6 +45,7 @@ gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerBase ## CONSUMES gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerPreMemPages ## CONSUMES gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerPages ## CONSUMES + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## CONSUMES [BuildOptions] *_*_*_CC_FLAGS = -D ADVANCED_LOGGER_SEC=1 diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/SmmCore/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/SmmCore/AdvancedLoggerLib.inf index 2b03dc9870..480a989d6e 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/SmmCore/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/SmmCore/AdvancedLoggerLib.inf @@ -45,3 +45,4 @@ gAdvancedLoggerProtocolGuid ## CONSUMES [Pcd] + gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerHdwPortDebugPrintErrorLevel ## SOMETIMES_CONSUMES