From 6f27d335f74a9630157c86b1ad0e717009cede3f Mon Sep 17 00:00:00 2001 From: Silent Date: Mon, 19 Oct 2020 21:27:16 +0200 Subject: [PATCH 1/3] Change ThreadId parameter type to PWORD Purely cosmetic change, does not change any functionality. --- src/core/kernel/common/ps.h | 4 ++-- src/core/kernel/exports/EmuKrnlPs.cpp | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/kernel/common/ps.h b/src/core/kernel/common/ps.h index a9da41d00..e5ec71aba 100644 --- a/src/core/kernel/common/ps.h +++ b/src/core/kernel/common/ps.h @@ -25,7 +25,7 @@ namespace xbox XBSYSAPI EXPORTNUM(254) ntstatus_xt NTAPI PsCreateSystemThread ( OUT PHANDLE ThreadHandle, - OUT PHANDLE ThreadId OPTIONAL, + OUT PDWORD ThreadId OPTIONAL, IN PKSTART_ROUTINE StartRoutine, IN PVOID StartContext, IN boolean_xt DebuggerThread @@ -40,7 +40,7 @@ XBSYSAPI EXPORTNUM(255) ntstatus_xt NTAPI PsCreateSystemThreadEx IN ulong_xt ThreadExtensionSize, IN ulong_xt KernelStackSize, IN ulong_xt TlsDataSize, - OUT PHANDLE ThreadId OPTIONAL, + OUT PDWORD ThreadId OPTIONAL, IN PKSTART_ROUTINE StartRoutine, IN PVOID StartContext, IN boolean_xt CreateSuspended, diff --git a/src/core/kernel/exports/EmuKrnlPs.cpp b/src/core/kernel/exports/EmuKrnlPs.cpp index e3eb2be71..9a62cdc16 100644 --- a/src/core/kernel/exports/EmuKrnlPs.cpp +++ b/src/core/kernel/exports/EmuKrnlPs.cpp @@ -166,7 +166,7 @@ void PspSystemThreadStartup XBSYSAPI EXPORTNUM(254) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThread ( OUT PHANDLE ThreadHandle, - OUT PHANDLE ThreadId OPTIONAL, + OUT PDWORD ThreadId OPTIONAL, IN PKSTART_ROUTINE StartRoutine, IN PVOID StartContext, IN boolean_xt DebuggerThread @@ -210,7 +210,7 @@ XBSYSAPI EXPORTNUM(255) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThreadEx IN ulong_xt ThreadExtensionSize, IN ulong_xt KernelStackSize, IN ulong_xt TlsDataSize, - OUT PHANDLE ThreadId OPTIONAL, + OUT PDWORD ThreadId OPTIONAL, IN PKSTART_ROUTINE StartRoutine, IN PVOID StartContext, IN boolean_xt CreateSuspended, @@ -284,9 +284,9 @@ XBSYSAPI EXPORTNUM(255) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThreadEx } }*/ - *ThreadHandle = (HANDLE)_beginthreadex(NULL, KernelStackSize, PCSTProxy, iPCSTProxyParam, NULL, (unsigned int*)&dwThreadId); + *ThreadHandle = reinterpret_cast(_beginthreadex(NULL, KernelStackSize, PCSTProxy, iPCSTProxyParam, NULL, reinterpret_cast(&dwThreadId))); if (ThreadId != NULL) - *ThreadId = (xbox::HANDLE)dwThreadId; + *ThreadId = dwThreadId; // Note : DO NOT use iPCSTProxyParam anymore, since ownership is transferred to the proxy (which frees it too) From b04980a1501dbb89987836b846bf434f9a5ef77c Mon Sep 17 00:00:00 2001 From: Silent Date: Mon, 19 Oct 2020 21:49:46 +0200 Subject: [PATCH 2/3] Remove hStartedEvent It doesn't seem to be useful anymore (does not protect any thread-unsafe initialization), and it was encouraging the OS scheduler to keep executing the child thread before returning the parent thread from PsCreateSystemThreadEx Test case: The Warriors and its race condition on event creation (child thread waits on the event parent thread creates AFTER spawning the child thread) --- src/core/kernel/exports/EmuKrnlPs.cpp | 77 +++++---------------------- 1 file changed, 14 insertions(+), 63 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlPs.cpp b/src/core/kernel/exports/EmuKrnlPs.cpp index 9a62cdc16..9d69606a9 100644 --- a/src/core/kernel/exports/EmuKrnlPs.cpp +++ b/src/core/kernel/exports/EmuKrnlPs.cpp @@ -55,7 +55,6 @@ typedef struct _PCSTProxyParam IN PVOID StartContext; IN PVOID SystemRoutine; IN BOOL StartSuspended; - IN HANDLE hStartedEvent; } PCSTProxyParam; @@ -69,8 +68,7 @@ void LOG_PCSTProxy PVOID StartRoutine, PVOID StartContext, PVOID SystemRoutine, - BOOL StartSuspended, - HANDLE hStartedEvent + BOOL StartSuspended ) { LOG_FUNC_BEGIN @@ -78,7 +76,6 @@ void LOG_PCSTProxy LOG_FUNC_ARG(StartContext) LOG_FUNC_ARG(SystemRoutine) LOG_FUNC_ARG(StartSuspended) - LOG_FUNC_ARG(hStartedEvent) LOG_FUNC_END; } @@ -105,40 +102,32 @@ static unsigned int WINAPI PCSTProxy { CxbxSetThreadName("PsCreateSystemThread Proxy"); - PCSTProxyParam *iPCSTProxyParam = (PCSTProxyParam*)Parameter; + PCSTProxyParam *iPCSTProxyParam = static_cast(Parameter); - PVOID StartRoutine = iPCSTProxyParam->StartRoutine; - PVOID StartContext = iPCSTProxyParam->StartContext; - PVOID SystemRoutine = iPCSTProxyParam->SystemRoutine; - BOOL StartSuspended = iPCSTProxyParam->StartSuspended; - HANDLE hStartedEvent = iPCSTProxyParam->hStartedEvent; - - // Once deleted, unable to directly access iPCSTProxyParam in remainder of function. - free(iPCSTProxyParam); + // Copy params to the stack so they can be freed + PCSTProxyParam params = *iPCSTProxyParam; + delete iPCSTProxyParam; LOG_PCSTProxy( - StartRoutine, - StartContext, - SystemRoutine, - StartSuspended, - hStartedEvent); + params.StartRoutine, + params.StartContext, + params.SystemRoutine, + params.StartSuspended); // Do minimal thread initialization InitXboxThread(g_CPUXbox); - SetEvent(hStartedEvent); - - if (StartSuspended == TRUE) { + if (params.StartSuspended == TRUE) { SuspendThread(GetCurrentThread()); } - auto routine = (xbox::PKSYSTEM_ROUTINE)SystemRoutine; + auto routine = (xbox::PKSYSTEM_ROUTINE)params.SystemRoutine; // Debugging notice : When the below line shows up with an Exception dialog and a // message like: "Exception thrown at 0x00026190 in cxbx.exe: 0xC0000005: Access // violation reading location 0xFD001804.", then this is AS-DESIGNED behaviour! // (To avoid repetitions, uncheck "Break when this exception type is thrown"). - routine(xbox::PKSTART_ROUTINE(StartRoutine), StartContext); + routine(xbox::PKSTART_ROUTINE(params.StartRoutine), params.StartContext); // This will also handle thread notification : LOG_TEST_CASE("Thread returned from SystemRoutine"); @@ -245,22 +234,15 @@ XBSYSAPI EXPORTNUM(255) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThreadEx // create thread, using our special proxy technique { - DWORD dwThreadId = 0, dwThreadWait; - bool bWait = true; - HANDLE hStartedEvent = CreateEvent(NULL, TRUE, FALSE, NULL); - if (hStartedEvent == NULL) { - std::string errorMessage = CxbxGetLastErrorString("PsCreateSystemThreadEx could not create PCSTProxyEvent"); - CxbxKrnlCleanup(errorMessage.c_str()); - } + DWORD dwThreadId = 0; // PCSTProxy is responsible for cleaning up this pointer - PCSTProxyParam *iPCSTProxyParam = (PCSTProxyParam*)malloc(sizeof(PCSTProxyParam)); + PCSTProxyParam *iPCSTProxyParam = new PCSTProxyParam; iPCSTProxyParam->StartRoutine = (PVOID)StartRoutine; iPCSTProxyParam->StartContext = StartContext; iPCSTProxyParam->SystemRoutine = (PVOID)SystemRoutine; // NULL, XapiThreadStartup or unknown? iPCSTProxyParam->StartSuspended = CreateSuspended; - iPCSTProxyParam->hStartedEvent = hStartedEvent; /* // call thread notification routine(s) @@ -290,37 +272,6 @@ XBSYSAPI EXPORTNUM(255) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThreadEx // Note : DO NOT use iPCSTProxyParam anymore, since ownership is transferred to the proxy (which frees it too) - EmuLog(LOG_LEVEL::DEBUG, "Waiting for Xbox proxy thread to start..."); - - while (bWait) { - dwThreadWait = WaitForSingleObject(hStartedEvent, INFINITE); - switch (dwThreadWait) { - case WAIT_TIMEOUT: { // The time-out interval elapsed, and the object's state is nonsignaled. - EmuLog(LOG_LEVEL::WARNING, "Timeout while waiting for Xbox proxy thread to start...\n"); - bWait = false; - break; - } - case WAIT_OBJECT_0: { // The state of the specified object is signaled. - EmuLog(LOG_LEVEL::DEBUG, "Xbox proxy thread is started."); - bWait = false; - break; - } - default: { - if (dwThreadWait == WAIT_FAILED) // The function has failed - bWait = false; - - std::string ErrorStr = CxbxGetLastErrorString("While waiting for Xbox proxy thread to start"); - EmuLog(LOG_LEVEL::WARNING, "%s\n", ErrorStr.c_str()); - break; - } - } - } - - - // Release the event - CloseHandle(hStartedEvent); - hStartedEvent = NULL; - // Log ThreadID identical to how GetCurrentThreadID() is rendered : EmuLog(LOG_LEVEL::DEBUG, "Created Xbox proxy thread. Handle : 0x%X, ThreadId : [0x%.4X]", *ThreadHandle, dwThreadId); From b7b2c24fdb15fbcf07b1cfa10f87e4d98a0d4691 Mon Sep 17 00:00:00 2001 From: Silent Date: Mon, 19 Oct 2020 23:59:22 +0200 Subject: [PATCH 3/3] Start child threads suspended and finalize their initialization before resuming * Closes a possible race condition where a child thread uses ThreadHandle or dwThreadId AND starts before _beginthreadex even returns * Allows to remove hardcoded sleeps "slowing down" parent thread execution in favour of more reliable affinity mask changes on the child thread Test case: The Warriors and its race condition on event creation (child thread waits on the event parent thread creates AFTER spawning the child thread) --- src/core/kernel/exports/EmuKrnlPs.cpp | 45 +++++++++++++++------------ 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/src/core/kernel/exports/EmuKrnlPs.cpp b/src/core/kernel/exports/EmuKrnlPs.cpp index 9d69606a9..674c75a21 100644 --- a/src/core/kernel/exports/EmuKrnlPs.cpp +++ b/src/core/kernel/exports/EmuKrnlPs.cpp @@ -54,7 +54,6 @@ typedef struct _PCSTProxyParam IN PVOID StartRoutine; IN PVOID StartContext; IN PVOID SystemRoutine; - IN BOOL StartSuspended; } PCSTProxyParam; @@ -67,25 +66,29 @@ void LOG_PCSTProxy ( PVOID StartRoutine, PVOID StartContext, - PVOID SystemRoutine, - BOOL StartSuspended + PVOID SystemRoutine ) { LOG_FUNC_BEGIN LOG_FUNC_ARG(StartRoutine) LOG_FUNC_ARG(StartContext) LOG_FUNC_ARG(SystemRoutine) - LOG_FUNC_ARG(StartSuspended) LOG_FUNC_END; } -void InitXboxThread(DWORD_PTR cores) +// Overload which doesn't change affinity +void InitXboxThread() { // initialize FS segment selector EmuGenerateFS(CxbxKrnl_TLS, CxbxKrnl_TLSData); _controlfp(_PC_53, _MCW_PC); // Set Precision control to 53 bits (verified setting) _controlfp(_RC_NEAR, _MCW_RC); // Set Rounding control to near (unsure about this) +} + +void InitXboxThread(DWORD_PTR cores) +{ + InitXboxThread(); if (!g_UseAllCores) { // Run this thread solely on the indicated core(s) : @@ -111,16 +114,11 @@ static unsigned int WINAPI PCSTProxy LOG_PCSTProxy( params.StartRoutine, params.StartContext, - params.SystemRoutine, - params.StartSuspended); + params.SystemRoutine); // Do minimal thread initialization - InitXboxThread(g_CPUXbox); - - if (params.StartSuspended == TRUE) { - SuspendThread(GetCurrentThread()); - } + InitXboxThread(); auto routine = (xbox::PKSYSTEM_ROUTINE)params.SystemRoutine; // Debugging notice : When the below line shows up with an Exception dialog and a @@ -242,7 +240,6 @@ XBSYSAPI EXPORTNUM(255) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThreadEx iPCSTProxyParam->StartRoutine = (PVOID)StartRoutine; iPCSTProxyParam->StartContext = StartContext; iPCSTProxyParam->SystemRoutine = (PVOID)SystemRoutine; // NULL, XapiThreadStartup or unknown? - iPCSTProxyParam->StartSuspended = CreateSuspended; /* // call thread notification routine(s) @@ -266,21 +263,29 @@ XBSYSAPI EXPORTNUM(255) xbox::ntstatus_xt NTAPI xbox::PsCreateSystemThreadEx } }*/ - *ThreadHandle = reinterpret_cast(_beginthreadex(NULL, KernelStackSize, PCSTProxy, iPCSTProxyParam, NULL, reinterpret_cast(&dwThreadId))); + HANDLE handle = reinterpret_cast(_beginthreadex(NULL, KernelStackSize, PCSTProxy, iPCSTProxyParam, CREATE_SUSPENDED, reinterpret_cast(&dwThreadId))); + *ThreadHandle = handle; if (ThreadId != NULL) *ThreadId = dwThreadId; + if (!g_UseAllCores) { + // Run this thread solely on the indicated core(s) : + SetThreadAffinityMask(handle, g_CPUXbox); + } + + CxbxKrnlRegisterThread(handle); + + // Now that ThreadId is populated and affinity is changed, resume the thread (unless the guest passed CREATE_SUSPENDED) + if (!CreateSuspended) { + ResumeThread(handle); + } + // Note : DO NOT use iPCSTProxyParam anymore, since ownership is transferred to the proxy (which frees it too) // Log ThreadID identical to how GetCurrentThreadID() is rendered : - EmuLog(LOG_LEVEL::DEBUG, "Created Xbox proxy thread. Handle : 0x%X, ThreadId : [0x%.4X]", *ThreadHandle, dwThreadId); - - CxbxKrnlRegisterThread(*ThreadHandle); + EmuLog(LOG_LEVEL::DEBUG, "Created Xbox proxy thread. Handle : 0x%X, ThreadId : [0x%.4X]", handle, dwThreadId); } - SwitchToThread(); - Sleep(10); - RETURN(xbox::status_success); }