Il 26/10/2013 11:51, Stefan Weil ha scritto:
> unpatched QEMU, crash with assertion
>
> 00448670 <_qemu_coroutine_switch>:
> 448670: 53 push %ebx
> 448671: 83 ec 18 sub $0x18,%esp
> 448674: c7 04 24 a8 62 6d 00 movl $0x6d62a8,(%esp)
> 44867b: 8b 5c 24 24 mov 0x24(%esp),%ebx
> 44867f: e8 ec 9e 27 00 call 6c2570
> <___emutls_get_address>
> 448684: 89 18 mov %ebx,(%eax)
> 448686: 8b 44 24 28 mov 0x28(%esp),%eax
> 44868a: 89 43 24 mov %eax,0x24(%ebx)
> 44868d: 8b 43 20 mov 0x20(%ebx),%eax
> 448690: 89 04 24 mov %eax,(%esp)
> 448693: ff 15 c0 5f 83 00 call *0x835fc0
> 448699: 83 ec 04 sub $0x4,%esp
> 44869c: 8b 44 24 20 mov 0x20(%esp),%eax
> 4486a0: 8b 40 24 mov 0x24(%eax),%eax
> 4486a3: 83 c4 18 add $0x18,%esp
> 4486a6: 5b pop %ebx
> 4486a7: c3 ret
>
>
> patched, works
>
> 00448620 <_qemu_coroutine_switch>:
> 448620: 83 ec 1c sub $0x1c,%esp
> 448623: c7 04 24 a8 62 6d 00 movl $0x6d62a8,(%esp)
> 44862a: 89 5c 24 14 mov %ebx,0x14(%esp)
> 44862e: 8b 5c 24 24 mov 0x24(%esp),%ebx
> 448632: 89 74 24 18 mov %esi,0x18(%esp)
> 448636: e8 25 9f 27 00 call 6c2560
> <___emutls_get_address>
> 44863b: 8b 30 mov (%eax),%esi
> 44863d: 89 18 mov %ebx,(%eax)
> 44863f: 8b 44 24 28 mov 0x28(%esp),%eax
> 448643: 89 43 24 mov %eax,0x24(%ebx)
> 448646: 8b 43 20 mov 0x20(%ebx),%eax
> 448649: 89 04 24 mov %eax,(%esp)
> 44864c: ff 15 c0 5f 83 00 call *0x835fc0
> 448652: 8b 46 24 mov 0x24(%esi),%eax
> 448655: 83 ec 04 sub $0x4,%esp
> 448658: 8b 5c 24 14 mov 0x14(%esp),%ebx
> 44865c: 8b 74 24 18 mov 0x18(%esp),%esi
> 448660: 83 c4 1c add $0x1c,%esp
> 448663: c3 ret
The only difference here is basically that "from" is being saved in
%esi across the calls to __emutls_get_address and SwitchToFiber. But
as Peter found out, the fix really happens because now the compiler
will not inline qemu_coroutine_switch anymore.
Peter provided another dump, this time from Win64. It is the
coroutine_trampoline with inlined qemu_coroutine_switch:
0: push %rdi
1: push %rsi
2: push %rbx
3: sub $0x30,%rsp
7: mov %rcx,%rbx
a: lea ...(%rip),%rcx
11: mov ...(%rip),%rax
18: mov %rax,0x28(%rsp)
1d: xor %eax,%eax
1f: callq ___emutls_get_address
24: mov ...(%rip),%rsi
2b: mov %rax,%rdi
2e: nop2
30: mov 0x8(%rbx),%rcx # ecx = co->entry_arg
34: callq *(%rbx) # co->entry(co->entry_arg);
36: mov 0x10(%rbx),%rax # load co->caller
3a: mov %rax,(%rdi) # "current" = co->caller; (wrong current!)
3d: movl $0x2,0x48(%rax) # co->caller->action = COROUTINE_TERMINATE;
44: mov 0x40(%rax),%rcx # load co->caller->fiber
48: callq *%rsi
4a: jmp 30 <coroutine_trampoline+0x30>
4c: nopl 0x0(%rax)
Some offsets are incomplete because this is from a .o, so not
linked, but it's already enough to see that ___emutls_get_address
(from "current = to_;" in qemu_coroutine_switch) is being hoisted
outside the loop, which becomes:
Coroutine *co = co_;
Coroutine **p_current = ¤t;
while (true) {
co->entry(co->entry_arg);
CoroutineWin32 *from = DO_UPCAST(CoroutineWin32, base, co);
CoroutineWin32 *to = DO_UPCAST(CoroutineWin32, base, co->caller);
*p_current = to_;
to->action = action;
SwitchToFiber(to->fiber);
return from->action;
}
This is wrong if co->entry yields out of the coroutine which is then
restarted on another thread. This can happen quite often. Typically.
a coroutine is started when a VCPU thread does bdrv_aio_readv:
VCPU thread
main VCPU thread coroutine I/O coroutine
bdrv_aio_readv ----->
start I/O operation
thread_pool_submit_co
<------------ yields
back to emulation
Then I/O finishes and the thread-pool.c event notifier triggers in
the I/O thread. event_notifier_ready calls thread_pool_co_cb, and
the I/O coroutine now restarts *in another thread*:
iothread
main iothread coroutine I/O coroutine (formerly in VCPU thread)
event_notifier_ready
thread_pool_co_cb -----> current = I/O coroutine;
call AIO callback
But on Win32, because of the bug, the "current" being set here the
current coroutine of the VCPU thread, not the iothread.
noinline is a good-enough workaround, and quite unlikely to break in
the future.
Paolo
