From 36ca87491d06a9e9ea490f9c2f15c07296224825 Mon Sep 17 00:00:00 2001 From: TuxSH <1922548+TuxSH@users.noreply.github.com> Date: Thu, 30 Jan 2020 01:22:37 +0000 Subject: [PATCH] thermosphere: gdb/debug: avoid pause/unpause race condition in vCont + bugfix --- thermosphere/src/debug_manager.c | 24 ++++++++++++++++++------ thermosphere/src/debug_manager.h | 4 +++- thermosphere/src/gdb/debug.c | 6 ++++-- thermosphere/src/irq.c | 11 +++++++++-- thermosphere/src/irq.h | 7 ++++--- thermosphere/src/main.c | 2 +- 6 files changed, 39 insertions(+), 15 deletions(-) diff --git a/thermosphere/src/debug_manager.c b/thermosphere/src/debug_manager.c index 262cd6fd7..ebea69dab 100644 --- a/thermosphere/src/debug_manager.c +++ b/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)); + } +} diff --git a/thermosphere/src/debug_manager.h b/thermosphere/src/debug_manager.h index 7c300c13e..d99640211 100644 --- a/thermosphere/src/debug_manager.h +++ b/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) { diff --git a/thermosphere/src/gdb/debug.c b/thermosphere/src/gdb/debug.c index 1be2229c8..8a066718f 100644 --- a/thermosphere/src/gdb/debug.c +++ b/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; } diff --git a/thermosphere/src/irq.c b/thermosphere/src/irq.c index 872d2ed5f..bc73f6156 100644 --- a/thermosphere/src/irq.c +++ b/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)); } } diff --git a/thermosphere/src/irq.h b/thermosphere/src/irq.h index 21a510796..f6c07eb92 100644 --- a/thermosphere/src/irq.h +++ b/thermosphere/src/irq.h @@ -39,9 +39,10 @@ typedef struct IrqManager { typedef enum ThermosphereSgi { ThermosphereSgi_ExecuteFunction = 0, - ThermosphereSgi_VgicUpdate = 1, - ThermosphereSgi_DebugPause = 2, - ThermosphereSgi_ReportDebuggerBreak = 3, + ThermosphereSgi_VgicUpdate, + ThermosphereSgi_DebugPause, + ThermosphereSgi_ReportDebuggerBreak, + ThermosphereSgi_DebuggerContinue, ThermosphereSgi_Max, } ThermosphereSgi; diff --git a/thermosphere/src/main.c b/thermosphere/src/main.c index dd8173d85..0c6f66255 100644 --- a/thermosphere/src/main.c +++ b/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); }