Improvements to Windbgx integration and UEFI extension (#22)

## Description

Improvements to the core for:
1. Add windbg workaround PCD to allow disabling for native GDB systems.
2. Add break reason for debugger initiated breakpoints
3. Add workaround for #10 
4. Fix `.reboot` not working

Improvements to UEFI Extension:
1. Cleaned up help function
2. Added crude implementation for adv logger parser 


- [x] Impacts functionality?
- [ ] Impacts security?
- [ ] Breaking change?
- [ ] Includes tests?
- [x] Includes documentation?

## How This Was Tested

Local tests on QEMU

## Integration Instructions

N/A

DebuggerFeaturePkg/DebuggerFeaturePkg.dec

@@ -54,3 +54,7 @@
   ## Forcibly enables the debugger with a default configuration depending on the
   #  phase implementation.
   DebuggerFeaturePkgTokenSpaceGuid.PcdForceEnableDebugger|FALSE|BOOLEAN|0x00000003
+
+  ## Enabled work-arounds in the debugger for bugs in windbg's GDB implementation.
+  #  This should not break GDB debuggers, but may cause slightly unexpected behavior.
+  DebuggerFeaturePkgTokenSpaceGuid.PcdEnableWindbgWorkarounds|TRUE|BOOLEAN|0x00000004

DebuggerFeaturePkg/Library/DebugAgent/AARCH64/DebugAarch64.c

@@ -158,6 +158,18 @@ DebuggerExceptionHandler (
 
   ExceptionInfo.ArchExceptionCode = ExceptionType;
 
+  if (PcdGetBool (PcdEnableWindbgWorkarounds) &&
+      (ExceptionType == 0x3c) &&
+      (DebuggerBreakpointReason != BreakpointReasonNone) &&
+      (CompareMem ((UINT8 *)Context->ELR, &ArchBreakpointInstruction[0], ArchBreakpointInstructionSize) == 0))
+  {
+    //
+    // Windbg will act oddly when broken in on a actual debug breakpoint instruction,
+    // so preemptively step past this.
+    //
+    Context->ELR += ArchBreakpointInstructionSize;
+  }
+
   ReportEntryToDebugger (&ExceptionInfo, SystemContext);
 
   //

DebuggerFeaturePkg/Library/DebugAgent/Breakpoint.c

@@ -25,6 +25,8 @@ typedef struct _BREAKPOINT_INFO {
 
 BREAKPOINT_INFO  mBreakpoints[MAX_BREAKPOINTS];
 
+BREAKPOINT_REASON  DebuggerBreakpointReason = BreakpointReasonNone;
+
 /**
   Adds a software breakpoint as the specific address.
 
@@ -96,3 +98,19 @@ RemoveSoftwareBreakpoint (
   // Not found.
   return FALSE;
 }
+
+/**
+  Immediately breaks into the debugger.
+
+  @param[in]  Reason    The reason for the debug break.
+
+**/
+VOID
+DebuggerBreak (
+  BREAKPOINT_REASON  Reason
+  )
+{
+  DebuggerBreakpointReason = Reason;
+  CpuBreakpoint ();
+  DebuggerBreakpointReason = BreakpointReasonNone;
+}

DebuggerFeaturePkg/Library/DebugAgent/DebugAgent.h

@@ -71,10 +71,17 @@ extern UINTN   ArchBreakpointInstructionSize;
 extern UINT32  ArchExceptionTypes[];
 
 //
-// Global used for early use of performance counter
+// Global used to track debugger invoked breakpoint.
 //
 
-extern UINT64  gPerformanceCounterFreq;
+typedef enum _BREAKPOINT_REASON {
+  BreakpointReasonNone = 0,
+  BreakpointReasonInitial,
+  BreakpointReasonModuleLoad,
+  BreakpointReasonDebuggerBreak,
+} BREAKPOINT_REASON;
+
+extern BREAKPOINT_REASON  DebuggerBreakpointReason;
 
 //
 // Global used for debugger information.
@@ -172,6 +179,11 @@ RemoveSoftwareBreakpoint (
   UINTN  Address
   );
 
+VOID
+DebuggerBreak (
+  BREAKPOINT_REASON  Reason
+  );
+
 //
 // Architecture specific
 //

DebuggerFeaturePkg/Library/DebugAgent/DebugAgentDxe.c

@@ -22,6 +22,7 @@
 #include <Library/CpuExceptionHandlerLib.h>
 #include <Library/DebugTransportLib.h>
 #include <Library/HobLib.h>
+#include <Library/ResetSystemLib.h>
 
 #include "DebugAgent.h"
 
@@ -328,7 +329,7 @@ OnLoadedImageNotification (
 
     if (AsciiStrLen (Name) == AsciiStrLen (mDbgBreakOnModuleLoadString)) {
       if (AsciiStriCmp (mDbgBreakOnModuleLoadString, Name) == 0) {
-        CpuBreakpoint ();
+        DebuggerBreak (BreakpointReasonModuleLoad);
         mDbgBreakOnModuleLoadString[0] = 0;
         break;
       }
@@ -410,11 +411,7 @@ DebugReboot (
   VOID
   )
 {
-  if ((gDxeCoreRT == NULL) || (gDxeCoreRT->ResetSystem == NULL)) {
-    return;
-  }
-
-  gDxeCoreRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
+  ResetCold ();
 }
 
 /**

DebuggerFeaturePkg/Library/DebugAgent/DebugAgentDxe.inf

@@ -62,6 +62,7 @@
   DeviceStateLib
   DebugTransportLib
   TransportLogControlLib
+  HwResetSystemLib
 
 [LibraryClasses.AARCH64]
   TimerLib # Only safe to use early in AARCH64
@@ -79,6 +80,7 @@
 
 [Pcd.common]
   DebuggerFeaturePkgTokenSpaceGuid.PcdForceEnableDebugger           ## CONSUMES
+  DebuggerFeaturePkgTokenSpaceGuid.PcdEnableWindbgWorkarounds       ## CONSUMES
 
 [BuildOptions]
   *_*_*_CC_FLAGS  = -D BUILDING_IN_UEFI

DebuggerFeaturePkg/Library/DebugAgent/DebugAgentMm.inf

@@ -74,6 +74,7 @@
 
 [Pcd.common]
   DebuggerFeaturePkgTokenSpaceGuid.PcdForceEnableDebugger           ## CONSUMES
+  DebuggerFeaturePkgTokenSpaceGuid.PcdEnableWindbgWorkarounds       ## CONSUMES
 
 [BuildOptions]
   *_*_*_CC_FLAGS  = -D BUILDING_IN_UEFI

DebuggerFeaturePkg/Library/DebugAgent/GdbStub/GdbStub.c

@@ -409,7 +409,10 @@ ProcessMemoryCommand (
       // return 0 so that the logic fails fast.
       //
 
-      if (((Address == 0) || ((Address & ~EFI_PAGE_MASK) == 0xfffff78000000000llu)) && (RangeLength < EFI_PAGE_SIZE)) {
+      if (PcdGetBool (PcdEnableWindbgWorkarounds) &&
+          ((Address == 0) || ((Address & ~EFI_PAGE_MASK) == 0xfffff78000000000llu)) &&
+          (RangeLength < EFI_PAGE_SIZE))
+      {
         ZeroMem (&mScratch[0], RangeLength);
       } else if (!DbgReadMemory (Address, &mScratch[0], RangeLength)) {
         SendGdbError (GDB_ERROR_BAD_MEM_ADDRESS);
@@ -1120,7 +1123,7 @@ DebuggerPollInput (
 
     // Check for the break character CTRL-C.
     if (Character == 0x3) {
-      CpuBreakpoint ();
+      DebuggerBreak (BreakpointReasonDebuggerBreak);
     }
   }
 }
@@ -1138,7 +1141,7 @@ DebuggerInitialBreakpoint (
   )
 {
   mNextBreakpointTimeout = Timeout;
-  CpuBreakpoint ();
+  DebuggerBreak (BreakpointReasonInitial);
 }
 
 /**

DebuggerFeaturePkg/Library/DebugAgent/X64/DebugX64.c

@@ -112,6 +112,19 @@ DebuggerExceptionHandler (
 
   ExceptionInfo.ArchExceptionCode = InterruptType;
 
+  if (PcdGetBool (PcdEnableWindbgWorkarounds) &&
+      (InterruptType == EXCEPT_X64_BREAKPOINT) &&
+      (DebuggerBreakpointReason != BreakpointReasonNone) &&
+      (*((UINT8 *)Context->Rip) == 0xCC))
+  {
+    //
+    // Windbg will act oddly when broken in on a actual INT 3 instruction, so
+    // preemptively step past this.
+    //
+
+    Context->Rip++;
+  }
+
   // Call into the core debugger module.
   ReportEntryToDebugger (&ExceptionInfo, SystemContext);
 

DebuggerFeaturePkg/Readme.md

@@ -90,7 +90,8 @@ for more information.
 You can attach to the UEFI debugger from the windbgx UI. Press Ctrl-K to get to
 the kernel debugger options, and click on the EXDI tab. From there you can can select
 the UEFI option, the appropriate architecture, Windows (not used), "None" for
-Image scanning heuristics size, and select the correct IP and port.
+Image scanning heuristics size, and select the correct IP and port. This requires
+Windbgx version 1.2404.22002.0 or newer.
 
 ![Windbgx EXDI UEFI](./Docs/Images/windbgx_uefi.png)
 

UefiDbgExt/memory.cpp

@@ -212,3 +212,148 @@ hobs (
   EXIT_API ();
   return S_OK;
 }
+
+#pragma pack (push, 1)
+typedef struct {
+  UINT32    Signature;          // Signature
+  UINT8     MajorVersion;       // Major version of advanced logger message structure
+  UINT8     MinorVersion;       // Minor version of advanced logger message structure
+  UINT32    DebugLevel;         // Debug Level
+  UINT64    TimeStamp;          // Time stamp
+  UINT16    Phase;              // Boot phase that produced this message entry
+  UINT16    MessageLen;         // Number of bytes in Message
+  UINT16    MessageOffset;      // Offset of Message from start of structure,
+                                // used to calculate the address of the Message
+  // CHAR      MessageText[];      // Message Text
+} ADVANCED_LOGGER_MESSAGE_ENTRY_V2;
+#pragma pack (pop)
+
+HRESULT CALLBACK
+advlog (
+  PDEBUG_CLIENT4  Client,
+  PCSTR           args
+  )
+{
+  ULONG64                           InfoAddress;
+  ULONG64                           EntryAddress;
+  ULONG64                           EndAddress;
+  ULONG                             LogBufferSize;
+  UINT16                            Version;
+  ULONG                             BytesRead;
+  HRESULT                           Result;
+  CHAR                              *LogBuffer = NULL;
+  ULONG                             Offset;
+  ULONG                             End;
+  ADVANCED_LOGGER_MESSAGE_ENTRY_V2  *Entry;
+
+  // NOTE: This implementation is a crude first past, The following should be done
+  // in the future.
+  //
+  // 1. Handle circular buffer.
+  // 2. Handle interleaved multipart messages.
+  // 3. More robust error checking.
+  // 4. Print metadata and allow filtering.
+  //
+
+  INIT_API ();
+
+  // If no valid input address was give, find the symbol.
+  if (GetExpressionEx (args, &InfoAddress, &args) == FALSE) {
+    InfoAddress = GetExpression ("mLoggerInfo");
+    if (InfoAddress == NULL) {
+      dprintf ("Failed to find mLoggerInfo!\n");
+      Result = ERROR_NOT_FOUND;
+      goto Exit;
+    }
+
+    if (!ReadPointer (InfoAddress, &InfoAddress)) {
+      dprintf ("Failed to read mLoggerInfo!\n");
+      Result = ERROR_NOT_FOUND;
+      goto Exit;
+    }
+  }
+
+  if (InfoAddress == NULL) {
+    dprintf ("Logger info is NULL!\n");
+    Result = ERROR_NOT_FOUND;
+    goto Exit;
+  }
+
+  GetFieldValue (InfoAddress, "ADVANCED_LOGGER_INFO", "Version", Version);
+  GetFieldValue (InfoAddress, "ADVANCED_LOGGER_INFO", "LogBufferSize", LogBufferSize);
+
+  g_ExtControl->ControlledOutput (
+                  DEBUG_OUTCTL_AMBIENT_DML,
+                  DEBUG_OUTPUT_NORMAL,
+                  "Header:   <exec cmd=\"dt ADVANCED_LOGGER_INFO %016I64x\">%x</exec>\n",
+                  InfoAddress,
+                  InfoAddress
+                  );
+
+  dprintf ("Version:  %d\n", Version);
+  dprintf ("Size:     0x%x bytes\n", LogBufferSize);
+
+  if (LogBufferSize == 0) {
+    dprintf ("Bad log buffer size!\n");
+    Result = ERROR_NOT_SUPPORTED;
+    goto Exit;
+  }
+
+  if (Version == 4) {
+    GetFieldValue (InfoAddress, "ADVANCED_LOGGER_INFO", "LogBuffer", EntryAddress);
+    GetFieldValue (InfoAddress, "ADVANCED_LOGGER_INFO", "LogCurrent", EndAddress);
+    if (EndAddress < EntryAddress) {
+      dprintf ("Looped logs not yet implemented in extension!\n");
+      Result = ERROR_NOT_SUPPORTED;
+      goto Exit;
+    } else {
+      // non-loop optimization, only download through the last message.
+      LogBufferSize = min (LogBufferSize, (ULONG)(EndAddress - InfoAddress));
+    }
+
+    LogBuffer = (CHAR *)malloc (LogBufferSize);
+
+    // Output() forces IO flush to inform user.
+    g_ExtControl->Output (DEBUG_OUTPUT_NORMAL, "Reading log (0x%x bytes) ... \n", LogBufferSize);
+    ReadMemory (InfoAddress, LogBuffer, LogBufferSize, &BytesRead);
+    if (BytesRead != LogBufferSize) {
+      dprintf ("Failed to read log memory!\n");
+      Result = ERROR_BAD_LENGTH;
+      goto Exit;
+    }
+
+    Offset = (ULONG)(EntryAddress - InfoAddress);
+    End    = (ULONG)(EndAddress - InfoAddress);
+
+    dprintf ("\n------------------------------------------------------------------------------\n");
+    while (Offset < End) {
+      Entry = (ADVANCED_LOGGER_MESSAGE_ENTRY_V2 *)(LogBuffer + Offset);
+      if (Entry->Signature != 0x324d4c41) {
+        dprintf ("\nBad message signature!!\n");
+        break;
+      }
+
+      ULONG  StringEnd = Offset + Entry->MessageOffset + Entry->MessageLen;
+      CHAR   Temp      = LogBuffer[StringEnd];
+      LogBuffer[StringEnd] = 0;
+      dprintf ("%s", LogBuffer + Offset + Entry->MessageOffset);
+      LogBuffer[StringEnd] = Temp;
+
+      Offset = ALIGN_UP (Offset + Entry->MessageOffset + Entry->MessageLen, 8);
+    }
+
+    dprintf ("\n------------------------------------------------------------------------------\n");
+  } else {
+    dprintf ("\nVersion not implemented in debug extension!\n");
+  }
+
+  Result = S_OK;
+
+Exit:
+  if (LogBuffer != NULL) {
+    free (LogBuffer);
+  }
+
+  EXIT_API ();
+  return Result;
+}

UefiDbgExt/swdebug.cpp

@@ -16,6 +16,24 @@ Module Name:
 
 #include "uefiext.h"
 
+HRESULT CALLBACK
+info (
+  PDEBUG_CLIENT4  Client,
+  PCSTR           args
+  )
+{
+  INIT_API ();
+
+  g_ExtControl->Execute (
+                  DEBUG_OUTCTL_ALL_CLIENTS,
+                  ".exdicmd target:0:?",
+                  DEBUG_EXECUTE_DEFAULT
+                  );
+
+  EXIT_API ();
+  return S_OK;
+}
+
 HRESULT CALLBACK
 modulebreak (
   PDEBUG_CLIENT4  Client,
@@ -51,7 +69,7 @@ readmsr (
     dprintf ("Must provide MSR index in HEX!");
   }
 
-  sprintf_s (Command, sizeof (Command), ".exdicmd target:0:\"m%s\"", args);
+  sprintf_s (Command, sizeof (Command), ".exdicmd target:0:m%s", args);
   g_ExtControl->Execute (
                   DEBUG_OUTCTL_ALL_CLIENTS,
                   Command,
@@ -76,7 +94,7 @@ readvar (
     dprintf ("Must provide variable name!");
   }
 
-  sprintf_s (Command, sizeof (Command), ".exdicmd target:*:\"v%s\"", args);
+  sprintf_s (Command, sizeof (Command), ".exdicmd target:*:v%s", args);
   g_ExtControl->Execute (
                   DEBUG_OUTCTL_ALL_CLIENTS,
                   Command,