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 <[email protected]>
microsoft · Jan 25, 2024 · 3c47b5f · 3c47b5f
1 parent 6d5e0c5
commit 3c47b5f
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 64 deletions.
diff --git 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
@@ -44,6 +44,7 @@
   UefiApplicationEntryPoint
   UefiLib
   UnitTestLib
+  UefiBootServicesTableLib
 
 [FeaturePcd]
   gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerAutoWrapEnable