thermosphere: gdb/debug: avoid pause/unpause race condition in vCont + bugfix

thermosphere/src/debug_manager.c

@@ -50,7 +50,7 @@ static void debugManagerDoPauseCores(u32 coreList)
     do {
         desiredList |= readList;
         remainingList &= ~readList;
-    } while (atomic_compare_exchange_weak(&g_debugManager.pausedCoreList, &readList, desiredList));
+    } while (!atomic_compare_exchange_weak(&g_debugManager.pausedCoreList, &readList, desiredList));
 
     if (remainingList != BIT(currentCoreCtx->coreId)) {
         // We need to notify other cores...
@@ -116,10 +116,13 @@ void debugManagerPauseCores(u32 coreList)
     restoreInterruptFlags(flags);
 }
 
-void debugManagerUnpauseCores(u32 coreList, u32 singleStepList)
+void debugManagerSetSingleStepCoreList(u32 coreList)
 {
-    singleStepList &= coreList;
+    atomic_store(&g_debugManager.singleStepCoreList, coreList);
+}
 
+void debugManagerUnpauseCores(u32 coreList)
+{
     FOREACH_BIT (tmp, coreId, coreList) {
         if (&g_debugManager.debugEventInfos[coreId].handled) {
             // Discard already handled debug events
@@ -127,8 +130,6 @@ void debugManagerUnpauseCores(u32 coreList, u32 singleStepList)
         }
     }
 
-    // Since we're using a debugger lock, a simple stlr should be fine...
-    atomic_store(&g_debugManager.singleStepCoreList, singleStepList);
     atomic_fetch_and(&g_debugManager.pausedCoreList, ~coreList);
 
     __sev();
@@ -198,7 +199,18 @@ void debugManagerBreakCores(u32 coreList)
     if (coreList & ~BIT(coreId)) {
         generateSgiForList(ThermosphereSgi_ReportDebuggerBreak, coreList & ~BIT(coreId));
     }
-    if (coreList & BIT(coreId)) {
+    if (coreList & BIT(coreId) && !debugManagerIsCorePaused(coreId)) {
         debugManagerReportEvent(DBGEVENT_DEBUGGER_BREAK);
     }
 }
+
+void debugManagerContinueCores(u32 coreList)
+{
+    u32 coreId = currentCoreCtx->coreId;
+    if (coreList & ~BIT(coreId)) {
+        generateSgiForList(ThermosphereSgi_DebuggerContinue, coreList & ~BIT(coreId));
+    }
+    if (coreList & BIT(coreId) && debugManagerIsCorePaused(coreId)) {
+        debugManagerUnpauseCores(BIT(coreId));
+    }
+}

thermosphere/src/debug_manager.h

@@ -55,7 +55,8 @@ bool debugManagerHandlePause(void);
 // "Pause" makes sure all cores reaches the pause function before proceeding.
 // "Unpause" doesn't synchronize, it just makes sure the core resumes & that "pause" can be called again.
 void debugManagerPauseCores(u32 coreList);
-void debugManagerUnpauseCores(u32 coreList, u32 singleStepList);
+void debugManagerUnpauseCores(u32 coreList);
+void debugManagerSetSingleStepCoreList(u32 coreList);
 
 void debugManagerSetSteppingRange(u32 coreId, uintptr_t startAddr, uintptr_t endAddr);
 
@@ -67,6 +68,7 @@ const DebugEventInfo *debugManagerGetCoreDebugEvent(u32 coreId);
 void debugManagerReportEvent(DebugEventType type, ...);
 
 void debugManagerBreakCores(u32 coreList);
+void debugManagerContinueCores(u32 coreList);
 
 static inline bool debugManagerIsCorePaused(u32 coreId)
 {

thermosphere/src/gdb/debug.c

@@ -347,7 +347,8 @@ GDB_DECLARE_HANDLER(ContinueOrStepDeprecated)
         debugManagerSetSteppingRange(coreId, 0, 0);
     }
 
-    debugManagerUnpauseCores(coreList, ssMask);
+    debugManagerSetSingleStepCoreList(ssMask);
+    debugManagerUnpauseCores(coreList);
     return 0;
 }
 
@@ -461,8 +462,9 @@ GDB_DECLARE_VERBOSE_HANDLER(Continue)
         cmd = nextCmd;
     }
 
+    debugManagerSetSingleStepCoreList(stepCoreList);
     debugManagerBreakCores(stopCoreList);
-    debugManagerUnpauseCores(continueCoreList, stepCoreList);
+    debugManagerContinueCores(continueCoreList);
 
     return 0;
 }

thermosphere/src/irq.c

@@ -220,6 +220,7 @@ void handleIrqException(ExceptionStackFrame *frame, bool isLowerEl, bool isA32)
 
     bool isGuestInterrupt = false;
     bool isMaintenanceInterrupt = false;
+    bool isPaused = false;
 
     switch (irqId) {
         case ThermosphereSgi_ExecuteFunction:
@@ -232,7 +233,11 @@ void handleIrqException(ExceptionStackFrame *frame, bool isLowerEl, bool isA32)
             debugManagerPauseSgiHandler();
             break;
         case ThermosphereSgi_ReportDebuggerBreak:
-            // See bottom half
+        case ThermosphereSgi_DebuggerContinue:
+            // See bottom halves
+            // Because exceptions (other debug events) are handling w/ interrupts off, if
+            // we get there, there's no race condition possible with debugManagerReportEvent
+            isPaused = debugManagerIsCorePaused(currentCoreCtx->coreId);
             break;
         case GIC_IRQID_MAINTENANCE:
             isMaintenanceInterrupt = true;
@@ -274,8 +279,10 @@ void handleIrqException(ExceptionStackFrame *frame, bool isLowerEl, bool isA32)
         exceptionEnterInterruptibleHypervisorCode();
         unmaskIrq();
         transportInterfaceIrqHandlerBottomHalf(transportIface);
-    } else if (irqId == ThermosphereSgi_ReportDebuggerBreak) {
+    } else if (irqId == ThermosphereSgi_ReportDebuggerBreak && !isPaused) {
         debugManagerReportEvent(DBGEVENT_DEBUGGER_BREAK);
+    } else if (irqId == ThermosphereSgi_DebuggerContinue && isPaused) {
+        debugManagerUnpauseCores(BIT(currentCoreCtx->coreId));
     }
 
 }

thermosphere/src/irq.h

@@ -39,9 +39,10 @@ typedef struct IrqManager {
 
 typedef enum ThermosphereSgi {
     ThermosphereSgi_ExecuteFunction = 0,
-    ThermosphereSgi_VgicUpdate = 1,
+    ThermosphereSgi_VgicUpdate,
-    ThermosphereSgi_DebugPause = 2,
+    ThermosphereSgi_DebugPause,
-    ThermosphereSgi_ReportDebuggerBreak = 3,
+    ThermosphereSgi_ReportDebuggerBreak,
+    ThermosphereSgi_DebuggerContinue,
 
     ThermosphereSgi_Max,
 } ThermosphereSgi;

thermosphere/src/main.c

@@ -75,7 +75,7 @@ void testProcessDataCallback(TransportInterface *iface, void *p, size_t sz)
 {
     (void)iface;
     (void)sz;
-    debugManagerUnpauseCores(BIT(0), BIT(0));
+    debugManagerUnpauseCores(BIT(0));
     TestCtx *ctx = (TestCtx *)p;
     DEBUG("EL2 [core %u]: you typed: %s\n", currentCoreCtx->coreId, ctx->buf);
 }