[PATCH 0/1] MR5238: krnl386: Pause other tasks while executing a callback.
This change necessitated splitting K32WOWCallback16Ex into two functions to be able to spawn new 16-bit tasks without stopping the old ones. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56260 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238
From: Alex Henrie <alexhenrie24(a)gmail.com> This change necessitated splitting K32WOWCallback16Ex into two functions to be able to spawn new 16-bit tasks without stopping the old ones. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56260 --- dlls/krnl386.exe16/kernel16_private.h | 4 + dlls/krnl386.exe16/ne_module.c | 2 +- dlls/krnl386.exe16/task.c | 94 +++++++++++++++- dlls/krnl386.exe16/wowthunk.c | 152 ++++++++++++++------------ 4 files changed, 176 insertions(+), 76 deletions(-) diff --git a/dlls/krnl386.exe16/kernel16_private.h b/dlls/krnl386.exe16/kernel16_private.h index 9cf60d068a7..1441fab2324 100644 --- a/dlls/krnl386.exe16/kernel16_private.h +++ b/dlls/krnl386.exe16/kernel16_private.h @@ -283,8 +283,12 @@ extern void TASK_ExitTask(void); extern HTASK16 TASK_GetTaskFromThread( DWORD thread ); extern TDB *TASK_GetCurrent(void); extern void TASK_InstallTHHook( THHOOK *pNewThook ); +extern void TASK_SuspendAll(void); +extern void TASK_ResumeAll(void); +/* wowthunk.c */ extern BOOL WOWTHUNK_Init(void); +extern BOOL WOWTHUNK_CallWithRegs( DWORD arg_count, void *args, CONTEXT *context, BOOL interrupt ); extern WORD DOSMEM_0000H; extern WORD DOSMEM_BiosDataSeg; diff --git a/dlls/krnl386.exe16/ne_module.c b/dlls/krnl386.exe16/ne_module.c index 2f2431884b9..db555e0b954 100644 --- a/dlls/krnl386.exe16/ne_module.c +++ b/dlls/krnl386.exe16/ne_module.c @@ -1240,7 +1240,7 @@ DWORD NE_StartTask(void) TRACE("Starting main program: cs:ip=%04lx:%04lx ds=%04lx ss:sp=%04x:%04x\n", context.SegCs, context.Eip, context.SegDs, CURRENT_SS, CURRENT_SP); - WOWCallback16Ex( 0, WCB16_REGS, 0, NULL, (DWORD *)&context ); + WOWTHUNK_CallWithRegs( 0, NULL, &context, FALSE ); ExitThread( LOWORD(context.Eax) ); } return hInstance; /* error code */ diff --git a/dlls/krnl386.exe16/task.c b/dlls/krnl386.exe16/task.c index 534b912e884..d55e427336e 100644 --- a/dlls/krnl386.exe16/task.c +++ b/dlls/krnl386.exe16/task.c @@ -77,6 +77,15 @@ static UINT16 nTaskCount = 0; static HTASK16 initial_task, main_task; +static CRITICAL_SECTION task_list_cs; +static CRITICAL_SECTION_DEBUG task_list_debug = +{ + 0, 0, &task_list_cs, + { &task_list_debug.ProcessLocksList, &task_list_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": task_list_cs") } +}; +static CRITICAL_SECTION task_list_cs = { &task_list_debug, -1, 0, 0, 0, 0 }; + /*********************************************************************** * TASK_InstallTHHook */ @@ -109,6 +118,8 @@ TDB *TASK_GetCurrent(void) /*********************************************************************** * TASK_LinkTask + * + * Be sure to lock task_list_cs before calling this function. */ static void TASK_LinkTask( HTASK16 hTask ) { @@ -131,6 +142,8 @@ static void TASK_LinkTask( HTASK16 hTask ) /*********************************************************************** * TASK_UnlinkTask + * + * Be sure to lock task_list_cs before calling this function. */ static void TASK_UnlinkTask( HTASK16 hTask ) { @@ -239,11 +252,6 @@ static BOOL TASK_FreeThunk( SEGPTR thunk ) /*********************************************************************** * TASK_Create - * - * NOTE: This routine might be called by a Win32 thread. Thus, we need - * to be careful to protect global data structures. We do this - * by entering the Win16Lock while linking the task into the - * global task list. */ static TDB *TASK_Create( NE_MODULE *pModule, UINT16 cmdShow, LPCSTR cmdline, BYTE len ) { @@ -396,6 +404,8 @@ void TASK_CreateMainTask(void) STARTUPINFOA startup_info; UINT cmdShow = 1; /* SW_SHOWNORMAL but we don't want to include winuser.h here */ + RtlEnterCriticalSection( &task_list_cs ); + GetStartupInfoA( &startup_info ); if (startup_info.dwFlags & STARTF_USESHOWWINDOW) cmdShow = startup_info.wShowWindow; pTask = TASK_Create( NULL, cmdShow, NULL, 0 ); @@ -414,6 +424,8 @@ void TASK_CreateMainTask(void) /* (no need to get the win16 lock, we are the only thread at this point) */ TASK_LinkTask( pTask->hSelf ); main_task = pTask->hSelf; + + LeaveCriticalSection( &task_list_cs ); } @@ -470,10 +482,16 @@ static DWORD CALLBACK task_start( LPVOID p ) HeapFree( GetProcessHeap(), 0, data ); _EnterWin16Lock(); + + RtlEnterCriticalSection( &task_list_cs ); TASK_LinkTask( pTask->hSelf ); pTask->teb = NtCurrentTeb(); + LeaveCriticalSection( &task_list_cs ); + ret = NE_StartTask(); + _LeaveWin16Lock(); + return ret; } @@ -521,6 +539,66 @@ HTASK16 TASK_GetTaskFromThread( DWORD thread ) } +/*********************************************************************** + * TASK_SuspendAll + * + * Emulates a hardware interrupt by pausing all other tasks. Only one task can + * run at a time, so pausing the other tasks typically has no effect. However, + * some programs depend on hardware interrupts interrupting the active task in + * order to call a callback. We don't have real hardware interrupts, but there + * can be background threads that fulfill the same role, processing data and + * calling callbacks when finished. + */ +void TASK_SuspendAll(void) +{ + TDB *p; + + RtlEnterCriticalSection( &task_list_cs ); + + p = TASK_GetPtr( hFirstTask ); + + while (p) + { + DWORD thread_id = HandleToULong( p->teb->ClientId.UniqueThread ); + if (thread_id != GetCurrentThreadId()) + { + HANDLE thread = OpenThread( THREAD_SUSPEND_RESUME, FALSE, thread_id ); + SuspendThread( thread ); + CloseHandle( thread ); + } + p = TASK_GetPtr( p->hNext ); + } + + RtlLeaveCriticalSection( &task_list_cs ); +} + + +/*********************************************************************** + * TASK_ResumeAll + */ +void TASK_ResumeAll(void) +{ + TDB *p; + + RtlEnterCriticalSection( &task_list_cs ); + + p = TASK_GetPtr( hFirstTask ); + while (p) + { + DWORD thread_id = HandleToULong( p->teb->ClientId.UniqueThread ); + if (thread_id != GetCurrentThreadId()) + { + HANDLE thread = OpenThread( THREAD_SUSPEND_RESUME, FALSE, thread_id ); + ResumeThread( thread ); + CloseHandle( thread ); + } + p = TASK_GetPtr( p->hNext ); + } + + RtlLeaveCriticalSection( &task_list_cs ); +} + + /*********************************************************************** * TASK_CallTaskSignalProc */ @@ -559,7 +637,9 @@ void TASK_ExitTask(void) TASK_CallTaskSignalProc( USIG16_TERMINATION, pTask->hSelf ); /* Remove the task from the list to be sure we never switch back to it */ + RtlEnterCriticalSection( &task_list_cs ); TASK_UnlinkTask( pTask->hSelf ); + RtlLeaveCriticalSection( &task_list_cs ); if (!nTaskCount || (nTaskCount == 1 && hFirstTask == initial_task)) { @@ -737,8 +817,12 @@ void WINAPI SetPriority16( HTASK16 hTask, INT16 delta ) else if (newpriority > 15) newpriority = 15; pTask->priority = newpriority + 1; + + EnterCriticalSection( &task_list_cs ); TASK_UnlinkTask( pTask->hSelf ); TASK_LinkTask( pTask->hSelf ); + LeaveCriticalSection( &task_list_cs ); + pTask->priority--; } diff --git a/dlls/krnl386.exe16/wowthunk.c b/dlls/krnl386.exe16/wowthunk.c index b729c38e8f1..d4760834c22 100644 --- a/dlls/krnl386.exe16/wowthunk.c +++ b/dlls/krnl386.exe16/wowthunk.c @@ -422,94 +422,106 @@ WORD WINAPI K32WOWHandle16( HANDLE handle, WOW_HANDLE_TYPE type ) } /********************************************************************** - * K32WOWCallback16Ex (KERNEL32.55) + * WOWTHUNK_CallWithRegs */ -BOOL WINAPI K32WOWCallback16Ex( DWORD vpfn16, DWORD dwFlags, - DWORD cbArgs, LPVOID pArgs, LPDWORD pdwRetCode ) +BOOL WOWTHUNK_CallWithRegs( DWORD arg_size, void *args, CONTEXT *context, BOOL interrupt ) { /* * Arguments must be prepared in the correct order by the caller * (both for PASCAL and CDECL calling convention), so we simply * copy them to the 16-bit stack ... */ - char *stack = (char *)CURRENT_STACK16 - cbArgs; + char *stack = (char *)CURRENT_STACK16 - arg_size; - memcpy( stack, pArgs, cbArgs ); + memcpy( stack, args, arg_size ); - if (dwFlags & WCB16_REGS) + if (TRACE_ON(relay)) { - CONTEXT *context = (CONTEXT *)pdwRetCode; + DWORD count = arg_size / sizeof(WORD); + WORD * wstack = (WORD *)stack; + + TRACE_(relay)( "\1CallTo16(func=%04lx:%04x", context->SegCs, LOWORD(context->Eip) ); + while (count) TRACE_(relay)( ",%04x", wstack[--count] ); + TRACE_(relay)( ") ss:sp=%04x:%04x ax=%04x bx=%04x cx=%04x dx=%04x si=%04x di=%04x bp=%04x ds=%04x es=%04x\n", + CURRENT_SS, CURRENT_SP, + (WORD)context->Eax, (WORD)context->Ebx, (WORD)context->Ecx, + (WORD)context->Edx, (WORD)context->Esi, (WORD)context->Edi, + (WORD)context->Ebp, (WORD)context->SegDs, (WORD)context->SegEs ); + SYSLEVEL_CheckNotLevel( 2 ); + } - if (TRACE_ON(relay)) - { - DWORD count = cbArgs / sizeof(WORD); - WORD * wstack = (WORD *)stack; - - TRACE_(relay)( "\1CallTo16(func=%04lx:%04x", context->SegCs, LOWORD(context->Eip) ); - while (count) TRACE_(relay)( ",%04x", wstack[--count] ); - TRACE_(relay)( ") ss:sp=%04x:%04x ax=%04x bx=%04x cx=%04x dx=%04x si=%04x di=%04x bp=%04x ds=%04x es=%04x\n", - CURRENT_SS, CURRENT_SP, - (WORD)context->Eax, (WORD)context->Ebx, (WORD)context->Ecx, - (WORD)context->Edx, (WORD)context->Esi, (WORD)context->Edi, - (WORD)context->Ebp, (WORD)context->SegDs, (WORD)context->SegEs ); - SYSLEVEL_CheckNotLevel( 2 ); - } + /* push return address */ + stack -= sizeof(SEGPTR); + *((SEGPTR *)stack) = call16_ret_addr; + arg_size += sizeof(SEGPTR); - /* push return address */ - stack -= sizeof(SEGPTR); - *((SEGPTR *)stack) = call16_ret_addr; - cbArgs += sizeof(SEGPTR); + if (interrupt) TASK_SuspendAll(); + wine_call_to_16_regs( context, arg_size, call16_handler ); + if (interrupt) TASK_ResumeAll(); - _EnterWin16Lock(); - wine_call_to_16_regs( context, cbArgs, call16_handler ); - _LeaveWin16Lock(); - - if (TRACE_ON(relay)) - { - TRACE_(relay)( "\1RetFrom16() ss:sp=%04x:%04x ax=%04x bx=%04x cx=%04x dx=%04x bp=%04x sp=%04x\n", - CURRENT_SS, CURRENT_SP, - (WORD)context->Eax, (WORD)context->Ebx, (WORD)context->Ecx, - (WORD)context->Edx, (WORD)context->Ebp, (WORD)context->Esp ); - SYSLEVEL_CheckNotLevel( 2 ); - } + if (TRACE_ON(relay)) + { + TRACE_(relay)( "\1RetFrom16() ss:sp=%04x:%04x ax=%04x bx=%04x cx=%04x dx=%04x bp=%04x sp=%04x\n", + CURRENT_SS, CURRENT_SP, + (WORD)context->Eax, (WORD)context->Ebx, (WORD)context->Ecx, + (WORD)context->Edx, (WORD)context->Ebp, (WORD)context->Esp ); + SYSLEVEL_CheckNotLevel( 2 ); } - else + + return TRUE; /* success */ +} + +/********************************************************************** + * K32WOWCallback16Ex (KERNEL32.55) + */ +BOOL WINAPI K32WOWCallback16Ex( DWORD func, DWORD flags, DWORD arg_size, void *args, DWORD *ret_out ) +{ + char *stack; + DWORD ret; + + if (flags & WCB16_REGS) + return WOWTHUNK_CallWithRegs( arg_size, args, (CONTEXT *)ret_out, TRUE ); + + /* + * Arguments must be prepared in the correct order by the caller + * (both for PASCAL and CDECL calling convention), so we simply + * copy them to the 16-bit stack ... + */ + stack = (char *)CURRENT_STACK16 - arg_size; + memcpy( stack, args, arg_size ); + + if (TRACE_ON(relay)) { - DWORD ret; + DWORD count = arg_size / sizeof(WORD); + WORD * wstack = (WORD *)stack; + + TRACE_(relay)( "\1CallTo16(func=%04x:%04x,ds=%04x", + HIWORD(func), LOWORD(func), CURRENT_SS ); + while (count) TRACE_(relay)( ",%04x", wstack[--count] ); + TRACE_(relay)( ") ss:sp=%04x:%04x\n", CURRENT_SS, CURRENT_SP ); + SYSLEVEL_CheckNotLevel( 2 ); + } - if (TRACE_ON(relay)) - { - DWORD count = cbArgs / sizeof(WORD); - WORD * wstack = (WORD *)stack; - - TRACE_(relay)( "\1CallTo16(func=%04x:%04x,ds=%04x", - HIWORD(vpfn16), LOWORD(vpfn16), CURRENT_SS ); - while (count) TRACE_(relay)( ",%04x", wstack[--count] ); - TRACE_(relay)( ") ss:sp=%04x:%04x\n", CURRENT_SS, CURRENT_SP ); - SYSLEVEL_CheckNotLevel( 2 ); - } + /* push return address */ + stack -= sizeof(SEGPTR); + *((SEGPTR *)stack) = call16_ret_addr; + arg_size += sizeof(SEGPTR); - /* push return address */ - stack -= sizeof(SEGPTR); - *((SEGPTR *)stack) = call16_ret_addr; - cbArgs += sizeof(SEGPTR); - - /* - * Actually, we should take care whether the called routine cleans up - * its stack or not. Fortunately, our wine_call_to_16 core doesn't rely on - * the callee to do so; after the routine has returned, the 16-bit - * stack pointer is always reset to the position it had before. - */ - _EnterWin16Lock(); - ret = wine_call_to_16( (FARPROC16)vpfn16, cbArgs, call16_handler ); - if (pdwRetCode) *pdwRetCode = ret; - _LeaveWin16Lock(); + /* + * Actually, we should take care whether the called routine cleans up + * its stack or not. Fortunately, our wine_call_to_16 core doesn't rely on + * the callee to do so; after the routine has returned, the 16-bit + * stack pointer is always reset to the position it had before. + */ + TASK_SuspendAll(); + ret = wine_call_to_16( (FARPROC16)func, arg_size, call16_handler ); + TASK_ResumeAll(); + if (ret_out) *ret_out = ret; - if (TRACE_ON(relay)) - { - TRACE_(relay)( "\1RetFrom16() ss:sp=%04x:%04x retval=%08lx\n", CURRENT_SS, CURRENT_SP, ret ); - SYSLEVEL_CheckNotLevel( 2 ); - } + if (TRACE_ON(relay)) + { + TRACE_(relay)( "\1RetFrom16() ss:sp=%04x:%04x retval=%08lx\n", CURRENT_SS, CURRENT_SP, ret ); + SYSLEVEL_CheckNotLevel( 2 ); } return TRUE; /* success */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5238
This does refactoring that should probably be split off into other patches. I don't think it's right to pause whenever K32WOWCallback16Ex() is executed. Calling back from 32-bit code to 16-bit code isn't exclusive to hardware interrupts. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63566
On Tue Mar 5 19:02:59 2024 +0000, Zebediah Figura wrote:
This does refactoring that should probably be split off into other patches. I don't think it's right to pause whenever K32WOWCallback16Ex() is executed. Calling back from 32-bit code to 16-bit code isn't exclusive to hardware interrupts. Only one task can execute at a time anyway, so pausing the other threads usually won't have any effect. Are you saying that the functions that call WOWCallback16(Ex) should call TASK_SuspendAll and TASK_ResumeAll instead of calling them from within WOWCallback16Ex? Or are you proposing something else?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63571
On Tue Mar 5 19:02:59 2024 +0000, Alex Henrie wrote:
Only one task can execute at a time anyway, so pausing the other threads usually won't have any effect. Are you saying that the functions that call WOWCallback16(Ex) should call TASK_SuspendAll and TASK_ResumeAll instead of calling them from within WOWCallback16Ex? Or are you proposing something else? It works, but it's not declarative. I would think that whatever is doing hardware interrupts should be the place that's suspending win16 tasks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63583
On Tue Mar 5 19:14:45 2024 +0000, Zebediah Figura wrote:
It works, but it's not declarative. I would think that whatever is doing hardware interrupts should be the place that's suspending win16 tasks. I can certainly export those two functions from krnl386 and then call them from mmsystem. Out of curiosity, what do you mean by "declarative" in this context?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63584
On Tue Mar 5 19:28:14 2024 +0000, Alex Henrie wrote:
I can certainly export those two functions from krnl386 and then call them from mmsystem. Out of curiosity, what do you mean by "declarative" in this context? I've been thinking about this since yesterday and although we could make mmsystem call a `__wine_suspend_all_tasks` function or a `__wine_wow_callback16` function that has an argument to specify whether to suspend other threads or to lock the Win16Mutex, it seems much less elegant. Here's why: If the callback is called from a task thread, it must be the current task thread because 16-bit Windows didn't have preëmptive multitasking, and TASK_SuspendAll has no effect. (We could even detect that case by checking whether the Win16Mutex is already locked by the current thread, and skip the call to TASK_SuspendAll if it is.) If the callback is called from a non-task thread, it must be a simulated hardware interrupt that needs to call TASK_SuspendAll. We don't have to change every function that might call WOWCallback16(Ex) from a different thread because WOWCallback16Ex can decide what to do based on whether it was called from the current task thread or from a different thread; it doesn't need any additional information.
Does that make sense, or is there something I'm missing? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63712
I can certainly export those two functions from krnl386 and then call them from mmsystem. Out of curiosity, what do you mean by "declarative" in this context?
It's a difficult term to pin down, but somewhat abstractly, it means that code declares *what* should happen, not *how*, taking a top-down approach and putting logic where it belongs. More concretely, when some logic is conceptually conditional based on P, you want to check P in some way, rather than some unrelated or semi-related condition (or, in this case, no condition at all) that happens to be always the same as P in practice. This broadly makes code more readable, because it's communicating its intentions, and less fragile, because you don't have to remember the conditions that make P just happen to be true. In this case, we conceptually want to suspend the task when calling code from an interrupt. We are calling user mode code from an interrupt in specific places, so we should suspend interrupts in those places. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63951
In this case, we conceptually want to suspend the task when calling code from an interrupt. We are calling user mode code from an interrupt in specific places, so we should suspend interrupts in those places.
The bigger question is, do we actually need to do this? What bug is going to be fixed by suspending the tasks? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5238#note_63964
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Alexandre Julliard (＠julliard)
-
Zebediah Figura (＠zfigura)