From 58041c95b42dd4c4dbe69a8ec39c2f2d7b238cba Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Wed, 1 Mar 2023 22:43:42 +0100 Subject: [PATCH 1/3] Clean up unused dpc event member variable --- src/core/kernel/exports/EmuKrnlKe.cpp | 18 ++++-------------- src/core/kernel/init/CxbxKrnl.cpp | 4 ++-- src/core/kernel/init/CxbxKrnl.h | 2 +- 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlKe.cpp b/src/core/kernel/exports/EmuKrnlKe.cpp index e78614bb8..45a3a521a 100644 --- a/src/core/kernel/exports/EmuKrnlKe.cpp +++ b/src/core/kernel/exports/EmuKrnlKe.cpp @@ -96,11 +96,10 @@ namespace NtDll // TODO : Move towards thread-simulation based Dpc emulation typedef struct _DpcData { CRITICAL_SECTION Lock; - HANDLE DpcEvent; xbox::LIST_ENTRY DpcQueue; // TODO : Use KeGetCurrentPrcb()->DpcListHead instead } DpcData; -DpcData g_DpcData = { 0 }; // Note : g_DpcData is initialized in InitDpcThread() +DpcData g_DpcData = { 0 }; // Note : g_DpcData is initialized in InitDpcData() xbox::ulonglong_xt LARGE_INTEGER2ULONGLONG(xbox::LARGE_INTEGER value) { @@ -479,14 +478,12 @@ void ExecuteDpcQueue() LeaveCriticalSection(&(g_DpcData.Lock)); } -void InitDpcThread() +void InitDpcData() { - DWORD dwThreadId = 0; - + // Let's initialize the Dpc handling thread too, + // here for now (should be called by our caller) InitializeCriticalSection(&(g_DpcData.Lock)); InitializeListHead(&(g_DpcData.DpcQueue)); - EmuLogEx(CXBXR_MODULE::INIT, LOG_LEVEL::DEBUG, "Creating DPC event\n"); - g_DpcData.DpcEvent = CreateEvent(/*lpEventAttributes=*/nullptr, /*bManualReset=*/FALSE, /*bInitialState=*/FALSE, /*lpName=*/nullptr); } static constexpr uint32_t XBOX_TSC_FREQUENCY = 733333333; // Xbox Time Stamp Counter Frequency = 733333333 (CPU Clock) @@ -498,13 +495,6 @@ ULONGLONG CxbxGetPerformanceCounter(bool acpi) return Timer_GetScaledPerformanceCounter(period); } -void CxbxInitPerformanceCounters() -{ - // Let's initialize the Dpc handling thread too, - // here for now (should be called by our caller) - InitDpcThread(); -} - // ****************************************************************** // * 0x005C - KeAlertResumeThread() // ****************************************************************** diff --git a/src/core/kernel/init/CxbxKrnl.cpp b/src/core/kernel/init/CxbxKrnl.cpp index 94f7db80b..3643381af 100644 --- a/src/core/kernel/init/CxbxKrnl.cpp +++ b/src/core/kernel/init/CxbxKrnl.cpp @@ -1205,8 +1205,8 @@ static void CxbxrKrnlInitHacks() Timer_Init(); // for unicode conversions setlocale(LC_ALL, "English"); - // Initialize time-related variables for the kernel and the timers - CxbxInitPerformanceCounters(); + // Initialize DPC global + InitDpcData(); #ifdef _DEBUG // PopupCustom(LOG_LEVEL::INFO, "Attach a Debugger"); // Debug child processes using https://marketplace.visualstudio.com/items?itemName=GreggMiskelly.MicrosoftChildProcessDebuggingPowerTool diff --git a/src/core/kernel/init/CxbxKrnl.h b/src/core/kernel/init/CxbxKrnl.h index 0a1a46fc4..5f89fb5b0 100644 --- a/src/core/kernel/init/CxbxKrnl.h +++ b/src/core/kernel/init/CxbxKrnl.h @@ -155,7 +155,7 @@ void CxbxKrnlPanic(); /*! empty function */ void CxbxKrnlNoFunc(); -void CxbxInitPerformanceCounters(); // Implemented in EmuKrnlKe.cpp +void InitDpcData(); // Implemented in EmuKrnlKe.cpp /*! kernel thunk table */ extern uint32_t CxbxKrnl_KernelThunkTable[379]; From 99ab34ac82f2577d450aeb7e97117d39461f6507 Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Thu, 2 Mar 2023 14:59:10 +0100 Subject: [PATCH 2/3] Fixed dpc recursion bug --- src/core/kernel/exports/EmuKrnl.cpp | 8 ++++++-- src/core/kernel/exports/EmuKrnlKe.cpp | 20 ++++++++++++++++++-- src/core/kernel/init/CxbxKrnl.h | 1 + 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnl.cpp b/src/core/kernel/exports/EmuKrnl.cpp index a53220c83..6934cd1b5 100644 --- a/src/core/kernel/exports/EmuKrnl.cpp +++ b/src/core/kernel/exports/EmuKrnl.cpp @@ -158,7 +158,10 @@ void CallSoftwareInterrupt(const xbox::KIRQL SoftwareIrql) xbox::KiExecuteKernelApc(); break; case DISPATCH_LEVEL: // = 2 - ExecuteDpcQueue(); + // This can be recursively called by KiUnlockDispatcherDatabase and KfLowerIrql, so avoid calling DPCs again if the current one has queued yet another one + if (!IsDpcActive()) { // Avoid KeIsExecutingDpc(), as that logs + ExecuteDpcQueue(); + } break; case APC_LEVEL | DISPATCH_LEVEL: // = 3 KiUnexpectedInterrupt(); @@ -450,7 +453,8 @@ XBSYSAPI EXPORTNUM(163) xbox::void_xt FASTCALL xbox::KiUnlockDispatcherDatabase LOG_FUNC_ONE_ARG_TYPE(KIRQL_TYPE, OldIrql); // Wrong, this should only happen when OldIrql >= DISPATCH_LEVEL - if (!(KeGetCurrentPrcb()->DpcRoutineActive)) { // Avoid KeIsExecutingDpc(), as that logs + // Checking DpcRoutineActive doesn't work because our Prcb is per-thread instead of being per-processor + if (!IsDpcActive()) { // Avoid KeIsExecutingDpc(), as that logs HalRequestSoftwareInterrupt(DISPATCH_LEVEL); } diff --git a/src/core/kernel/exports/EmuKrnlKe.cpp b/src/core/kernel/exports/EmuKrnlKe.cpp index 45a3a521a..3300b8587 100644 --- a/src/core/kernel/exports/EmuKrnlKe.cpp +++ b/src/core/kernel/exports/EmuKrnlKe.cpp @@ -96,6 +96,7 @@ namespace NtDll // TODO : Move towards thread-simulation based Dpc emulation typedef struct _DpcData { CRITICAL_SECTION Lock; + std::atomic_flag IsDpcActive; xbox::LIST_ENTRY DpcQueue; // TODO : Use KeGetCurrentPrcb()->DpcListHead instead } DpcData; @@ -459,8 +460,10 @@ void ExecuteDpcQueue() // Mark it as no longer linked into the DpcQueue pkdpc->Inserted = FALSE; // Set DpcRoutineActive to support KeIsExecutingDpc: + g_DpcData.IsDpcActive.test_and_set(); KeGetCurrentPrcb()->DpcRoutineActive = TRUE; // Experimental - EmuLog(LOG_LEVEL::DEBUG, "Global DpcQueue, calling DPC at 0x%.8X", pkdpc->DeferredRoutine); + LeaveCriticalSection(&(g_DpcData.Lock)); + EmuLog(LOG_LEVEL::DEBUG, "Global DpcQueue, calling DPC object 0x%.8X at 0x%.8X", pkdpc, pkdpc->DeferredRoutine); // Call the Deferred Procedure : pkdpc->DeferredRoutine( @@ -469,7 +472,9 @@ void ExecuteDpcQueue() pkdpc->SystemArgument1, pkdpc->SystemArgument2); + EnterCriticalSection(&(g_DpcData.Lock)); KeGetCurrentPrcb()->DpcRoutineActive = FALSE; // Experimental + g_DpcData.IsDpcActive.clear(); } // Assert(g_DpcData._dwThreadId == GetCurrentThreadId()); @@ -486,6 +491,11 @@ void InitDpcData() InitializeListHead(&(g_DpcData.DpcQueue)); } +bool IsDpcActive() +{ + return g_DpcData.IsDpcActive.test(); +} + static constexpr uint32_t XBOX_TSC_FREQUENCY = 733333333; // Xbox Time Stamp Counter Frequency = 733333333 (CPU Clock) static constexpr uint32_t XBOX_ACPI_FREQUENCY = 3375000; // Xbox ACPI frequency (3.375 mhz) @@ -1258,7 +1268,9 @@ XBSYSAPI EXPORTNUM(119) xbox::boolean_xt NTAPI xbox::KeInsertQueueDpc InsertTailList(&(g_DpcData.DpcQueue), &(Dpc->DpcListEntry)); // TODO : Instead of DpcQueue, add the DPC to KeGetCurrentPrcb()->DpcListHead // Signal the Dpc handling code there's work to do - HalRequestSoftwareInterrupt(DISPATCH_LEVEL); + if (g_DpcData.IsDpcActive.test() == false) { + HalRequestSoftwareInterrupt(DISPATCH_LEVEL); + } // OpenXbox has this instead: // if (!pKPRCB->DpcRoutineActive && !pKPRCB->DpcInterruptRequested) { // pKPRCB->DpcInterruptRequested = TRUE; @@ -1279,7 +1291,11 @@ XBSYSAPI EXPORTNUM(121) xbox::boolean_xt NTAPI xbox::KeIsExecutingDpc { LOG_FUNC(); + // This is the correct implementation, but it doesn't work because our Prcb is per-thread instead of being per-processor +#if 0 BOOLEAN ret = (BOOLEAN)KeGetCurrentPrcb()->DpcRoutineActive; +#endif + BOOLEAN ret = (BOOLEAN)IsDpcActive(); RETURN(ret); } diff --git a/src/core/kernel/init/CxbxKrnl.h b/src/core/kernel/init/CxbxKrnl.h index 5f89fb5b0..c11af39cc 100644 --- a/src/core/kernel/init/CxbxKrnl.h +++ b/src/core/kernel/init/CxbxKrnl.h @@ -156,6 +156,7 @@ void CxbxKrnlPanic(); void CxbxKrnlNoFunc(); void InitDpcData(); // Implemented in EmuKrnlKe.cpp +bool IsDpcActive(); /*! kernel thunk table */ extern uint32_t CxbxKrnl_KernelThunkTable[379]; From 062752e1a7e70c3590dd498c9d61392c33be0de2 Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Thu, 2 Mar 2023 22:53:18 +0100 Subject: [PATCH 3/3] Review remarks --- src/core/kernel/exports/EmuKrnlKe.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlKe.cpp b/src/core/kernel/exports/EmuKrnlKe.cpp index 3300b8587..8bbfbc8d5 100644 --- a/src/core/kernel/exports/EmuKrnlKe.cpp +++ b/src/core/kernel/exports/EmuKrnlKe.cpp @@ -1268,7 +1268,7 @@ XBSYSAPI EXPORTNUM(119) xbox::boolean_xt NTAPI xbox::KeInsertQueueDpc InsertTailList(&(g_DpcData.DpcQueue), &(Dpc->DpcListEntry)); // TODO : Instead of DpcQueue, add the DPC to KeGetCurrentPrcb()->DpcListHead // Signal the Dpc handling code there's work to do - if (g_DpcData.IsDpcActive.test() == false) { + if (!IsDpcActive()) { HalRequestSoftwareInterrupt(DISPATCH_LEVEL); } // OpenXbox has this instead: @@ -1291,11 +1291,12 @@ XBSYSAPI EXPORTNUM(121) xbox::boolean_xt NTAPI xbox::KeIsExecutingDpc { LOG_FUNC(); - // This is the correct implementation, but it doesn't work because our Prcb is per-thread instead of being per-processor #if 0 + // This is the correct implementation, but it doesn't work because our Prcb is per-thread instead of being per-processor BOOLEAN ret = (BOOLEAN)KeGetCurrentPrcb()->DpcRoutineActive; -#endif +#else BOOLEAN ret = (BOOLEAN)IsDpcActive(); +#endif RETURN(ret); }