From 3c47b5f8e2f2bce5533fe446be011938445103db Mon Sep 17 00:00:00 2001 From: kuqin12 <42554914+kuqin12@users.noreply.github.com> Date: Thu, 25 Jan 2024 14:53:01 -0800 Subject: [PATCH] Fixing Advanced logger wrapping unit test (#417) # Preface Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior to submitting the pull request. In particular, [pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). ## Description The existing shell test will not take the disabled processors into account, thus causing the system to have false positive results. This change updated the logic to tick out disabled cores, removed some UT logs to avoid spamming the output and replaced all the returned status with UT_ASSERT for easier troubleshooting. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [ ] 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, ... - [x] 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 This change was tested on proprietary hardware with multiple cores. ## Integration Instructions N/A --------- Co-authored-by: Oliver Smith-Denny --- .../AdvancedLoggerWrapperTestApp.c | 116 ++++++++---------- .../AdvancedLoggerWrapperTestApp.inf | 1 + 2 files changed, 53 insertions(+), 64 deletions(-) diff --git a/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.c b/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.c index c6e331116d..2509686009 100644 --- a/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.c +++ b/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.c @@ -165,7 +165,7 @@ InitializeInMemoryLog ( // Make sure the wrapper feature is enabled. if (!FeaturePcdGet (PcdAdvancedLoggerAutoWrapEnable)) { - return UNIT_TEST_ERROR_TEST_FAILED; + return UNIT_TEST_ERROR_PREREQUISITE_NOT_MET; } // @@ -185,6 +185,7 @@ InitializeInMemoryLog ( if (!ValidateInfoBlock ()) { mLoggerInfo = NULL; + UT_ASSERT_NOT_NULL ((VOID *)mLoggerInfo); } // This is to bypass the restriction on runtime check. @@ -220,6 +221,8 @@ TestCursorWrapping ( Btc = (BASIC_TEST_CONTEXT *)Context; + UT_ASSERT_TRUE (mLoggerInfo != NULL); + // First fill in the buffer while (mLoggerInfo->LogCurrent + MESSAGE_ENTRY_SIZE_V2 (sizeof (ADVANCED_LOGGER_MESSAGE_ENTRY_V2), sizeof (ADV_TIME_TEST_STR)) < mMaxAddress) { AdvancedLoggerWrite (DEBUG_ERROR, ADV_TIME_TEST_STR, sizeof (ADV_TIME_TEST_STR)); @@ -295,28 +298,27 @@ TestCursorWrappingMP ( IN UNIT_TEST_CONTEXT Context ) { - BASIC_TEST_CONTEXT *Btc; - EFI_STATUS Status; - UINTN Index; - UINTN StrIndex; - UINTN NumberOfProcessors; - UINTN EnabledProcessors; - UINT8 *TempCache = NULL; - UNIT_TEST_STATUS UtStatus; - UINTN PrefixSize; - CHAR8 *EndPointer; + BASIC_TEST_CONTEXT *Btc; + EFI_STATUS Status; + UINTN Index; + UINTN StrIndex; + UINTN NumberOfProcessors; + UINTN EnabledProcessors; + UINT8 *TempCache = NULL; + UINTN PrefixSize; + CHAR8 EndChar; + EFI_PROCESSOR_INFORMATION CpuInfo; Btc = (BASIC_TEST_CONTEXT *)Context; + UT_ASSERT_NOT_NULL ((VOID *)mLoggerInfo); + // First fill in the buffer while (mLoggerInfo->LogCurrent + MESSAGE_ENTRY_SIZE_V2 (sizeof (ADVANCED_LOGGER_MESSAGE_ENTRY_V2), sizeof (ADV_TIME_TEST_STR)) < mMaxAddress) { AdvancedLoggerWrite (DEBUG_ERROR, ADV_TIME_TEST_STR, sizeof (ADV_TIME_TEST_STR)); } - if (mMpServicesProtocol == NULL) { - UtStatus = UNIT_TEST_ERROR_PREREQUISITE_NOT_MET; - goto Done; - } + UT_ASSERT_NOT_NULL (mMpServicesProtocol); Status = mMpServicesProtocol->StartupAllAPs ( mMpServicesProtocol, @@ -327,35 +329,32 @@ TestCursorWrappingMP ( NULL, NULL ); - if (EFI_ERROR (Status)) { - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; - } + UT_ASSERT_NOT_EFI_ERROR (Status); // BSP needs to run the procedure as well... ApProcedure (NULL); Status = mMpServicesProtocol->GetNumberOfProcessors (mMpServicesProtocol, &NumberOfProcessors, &EnabledProcessors); - if (EFI_ERROR (Status)) { - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; - } + UT_ASSERT_NOT_EFI_ERROR (Status); TempCache = AllocatePool (NumberOfProcessors * sizeof (UINT8)); - if (TempCache == NULL) { - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; - } + UT_ASSERT_NOT_NULL (TempCache); // Initialize the cache to 0xFF - SetMem (TempCache, NumberOfProcessors * sizeof (UINT8), 0xFF); - for (Index = 0; Index < NumberOfProcessors; Index++) { - Status = AdvancedLoggerAccessLibGetNextFormattedLine (&mMessageEntry); - if (EFI_ERROR (Status)) { - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; + Status = mMpServicesProtocol->GetProcessorInfo (mMpServicesProtocol, CPU_V2_EXTENDED_TOPOLOGY | Index, &CpuInfo); + UT_ASSERT_NOT_EFI_ERROR (Status); + + if (CpuInfo.StatusFlag & PROCESSOR_ENABLED_BIT) { + TempCache[Index] = 0xFF; + } else { + TempCache[Index] = 0; } + } + + for (Index = 0; Index < EnabledProcessors; Index++) { + Status = AdvancedLoggerAccessLibGetNextFormattedLine (&mMessageEntry); + UT_ASSERT_TRUE ((Status == EFI_SUCCESS) || (Status == EFI_END_OF_FILE)); // HACKHACK: Bypass the potential print out from MpLib if ((Index == 0) && (AsciiStrStr (mMessageEntry.Message, "5-Level Paging") != NULL)) { @@ -365,9 +364,7 @@ TestCursorWrappingMP ( UT_ASSERT_NOT_NULL (mMessageEntry.Message); UT_LOG_INFO ("\nReturn Length=%d\n", mMessageEntry.MessageLen); - UT_LOG_INFO ("\n = %a =\n", mMessageEntry.Message); UT_LOG_INFO ("\nExpected Length=%d\n", AsciiStrLen (Btc->ExpectedLine)); - UT_LOG_INFO ("\n = %a =\n", Btc->ExpectedLine); if (mMessageEntry.MessageLen != AsciiStrLen (Btc->ExpectedLine)) { DUMP_HEX (DEBUG_ERROR, 0, mMessageEntry.Message, mMessageEntry.MessageLen, "Actual - "); @@ -392,47 +389,38 @@ TestCursorWrappingMP ( ); // Now check the index - EndPointer = &mMessageEntry.Message[ADV_TIME_STAMP_PREFIX_LEN + sizeof (ADV_WRAP_TEST_STR) + 8 - 1]; - Status = AsciiStrHexToUintnS ( - &mMessageEntry.Message[ADV_TIME_STAMP_PREFIX_LEN + sizeof (ADV_WRAP_TEST_STR) - 1], - &EndPointer, - &StrIndex - ); - if (EFI_ERROR (Status)) { - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; - } + // First cache the current value at the end of target sequence + EndChar = mMessageEntry.Message[ADV_TIME_STAMP_PREFIX_LEN + PrefixSize + 8]; - if (StrIndex >= NumberOfProcessors) { - // Index is out of range - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; - } + // Then set that char to NULL + mMessageEntry.Message[ADV_TIME_STAMP_PREFIX_LEN + PrefixSize + 8] = '\0'; - if (TempCache[StrIndex] == 0) { - // Printed this index already - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; - } else { - TempCache[StrIndex] = 0; - } - } + // Then convert the string to a number + Status = AsciiStrHexToUintnS ( + &mMessageEntry.Message[ADV_TIME_STAMP_PREFIX_LEN + PrefixSize], + NULL, + &StrIndex + ); + UT_ASSERT_NOT_EFI_ERROR (Status); - if (!IsZeroBuffer (TempCache, NumberOfProcessors * sizeof (UINT8))) { - UtStatus = UNIT_TEST_ERROR_TEST_FAILED; - goto Done; + // Now we can set the char back to what it was + mMessageEntry.Message[ADV_TIME_STAMP_PREFIX_LEN + PrefixSize + 8] = EndChar; + + UT_ASSERT_TRUE (StrIndex < NumberOfProcessors); + + UT_ASSERT_TRUE (TempCache[StrIndex] == 0xFF); + TempCache[StrIndex] = 0; } - UtStatus = UNIT_TEST_PASSED; + UT_ASSERT_TRUE (IsZeroBuffer (TempCache, NumberOfProcessors * sizeof (UINT8))); -Done: if (TempCache != NULL) { FreePool (TempCache); } Btc->MemoryToFree = mMessageEntry.Message; - return UtStatus; + return UNIT_TEST_PASSED; } /// ================================================================================================ diff --git a/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.inf b/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.inf index 9467fc3e15..9c15db7d9b 100644 --- a/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.inf +++ b/AdvLoggerPkg/UnitTests/AdvancedLoggerWrapper/AdvancedLoggerWrapperTestApp.inf @@ -44,6 +44,7 @@ UefiApplicationEntryPoint UefiLib UnitTestLib + UefiBootServicesTableLib [FeaturePcd] gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerAutoWrapEnable