Re: [Qemu-devel] [PATCH v2] thread-win32: fix GetThreadContext() permanently fails

[Olga Krishtal <okrishtal@parallels.com>]

[-- Attachment #1: Type: text/plain, Size: 5011 bytes --]

On 25/06/15 15:17, Zavadovsky Yan wrote:
> On Thu, Jun 25, 2015 at 12:11 PM, Olga Krishtal <okrishtal@parallels.com>
> wrote:
>
>> On 24/06/15 15:25, Zavadovsky Yan wrote:
>>
>>> Calling SuspendThread() is not enough to suspend Win32 thread.
>>> We need to call GetThreadContext() after SuspendThread()
>>> to make sure that OS have really suspended target thread.
>>> But GetThreadContext() needs for THREAD_GET_CONTEXT
>>> access right on thread object.
>>> More info about this technique can be found here:
>>> http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
>>>
>>> This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
>>> and change oddity 'while(GetThreadContext() == SUCCESS)' to
>>> 'if(GetThreadContext() == FAILED){exit(1);}'.
>>> So this block of code will continue only after successful
>>> grabbing of thread context(i.e. when thread is really suspended).
>>> And halts otherwise with more verbose error message than previous.
>>> Signed-off-by: Zavadovsky Yan <zavadovsky.yan@gmail.com>
>>> ---
>>>    cpus.c                   | 14 ++++++++------
>>>    util/qemu-thread-win32.c |  4 ++--
>>>    2 files changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 4f0e54d..0df6a7d 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -1089,8 +1089,8 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>>            CONTEXT tcgContext;
>>>              if (SuspendThread(cpu->hThread) == (DWORD)-1) {
>>> -            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
>>> -                    GetLastError());
>>> +            fprintf(stderr, "qemu:%s: SuspendThread GetLastError:%lu\n",
>>> +                    __func__, GetLastError());
>>>                exit(1);
>>>            }
>>>    @@ -1098,15 +1098,17 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>>>             * suspended until we can get the context.
>>>             */
>>>            tcgContext.ContextFlags = CONTEXT_CONTROL;
>>> -        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
>>> -            continue;
>>>
>> I would like to ask you if you have faced this situation in reality?
>> I have some doubts about changing the while() loop. According to the
>> article -
>>
> SuspendThread is async operation. But GetContext is not async. And
> GetContext forces this pending suspension to complete.
>
> thread may mot be suspended immediately after code, due to the busy
>> scheduler.
>> If we do exit(1) just right after check of GetThreadContext(..) we can
>> miss the situation when
>> scheduler is too busy at the moment and just go down.  From this point of
>> view this while -
>> is busy loop and gives the scheduler the opportunity to do its job.
> If GetThreadContext fails in first call it will fail in next consecutive
> calls for this thread handle.
Afaik GetThreadContext can fail if the thread is in running state, it 
means that
thread has not been suspended yet, for example it is running on other 
cpu with hight irql.
Instead of waiting we are going to go down. I agree that busy loop 
should be somehow changed,
but going down every time.*
*
>
>> So, I am not sure about it.
>>
>>> +        if (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
>>> +            fprintf(stderr, "qemu:%s: GetThreadContext
>>> GetLastError:%lu\n",
>>> +                    __func__, GetLastError());
>>> +            exit(1);
>>>            }
>>>              cpu_signal(0);
>>>              if (ResumeThread(cpu->hThread) == (DWORD)-1) {
>>> -            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
>>> -                    GetLastError());
>>> +            fprintf(stderr, "qemu:%s: ResumeThread GetLastError:%lu\n",
>>> +                    __func__, GetLastError());
>>>                exit(1);
>>>            }
>>>        }
>>> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>>> index 406b52f..823eca1 100644
>>> --- a/util/qemu-thread-win32.c
>>> +++ b/util/qemu-thread-win32.c
>>> @@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
>>>          EnterCriticalSection(&data->cs);
>>>        if (!data->exited) {
>>> -        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
>>> -                            thread->tid);
>>>
>> What was before the usage of this flag?
> GetThreadContext returns zero, i.e. error. GetLastError says "5" i.e.
> "access denied".
>
> I mean the behavior? As I can see it worked even without this flag.
>
> This code works because:
> a)GetThreadContext works as Sleep(N) and scheduler have enough time to
> finish async suspend.
> or
> b)GetThreadContext always forces suspension to complete before checking
> handle's desired access and doing its own job.
> or
> c)something else, documentation on SuspendThread is weak.
>
>
>>   +        handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME |
>>> THREAD_GET_CONTEXT,
>>> +                            FALSE, thread->tid);
>>>        } else {
>>>            handle = NULL;
>>>        }
>>>
>>


[-- Attachment #2: Type: text/html, Size: 7192 bytes --]