From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48006) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz5PM-0008OU-04 for qemu-devel@nongnu.org; Mon, 23 Jun 2014 10:39:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wz5PC-0006e1-NZ for qemu-devel@nongnu.org; Mon, 23 Jun 2014 10:39:43 -0400 Received: from mail-we0-x230.google.com ([2a00:1450:400c:c03::230]:36854) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz5PC-0006dl-EA for qemu-devel@nongnu.org; Mon, 23 Jun 2014 10:39:34 -0400 Received: by mail-we0-f176.google.com with SMTP id u56so7042633wes.7 for ; Mon, 23 Jun 2014 07:39:33 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <53A83C22.6090208@redhat.com> Date: Mon, 23 Jun 2014 16:39:30 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <20131023073949.GA4527@bom.nom.co> <52679025.3000106@redhat.com> <526830E6.6070904@weilnetz.de> <5268F8B4.7030105@redhat.com> <52694CC1.6040301@weilnetz.de> <5269958A.50400@redhat.com> <526B908B.20104@weilnetz.de> In-Reply-To: <526B908B.20104@weilnetz.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] broken win32 coroutines (was Re: qemu 1.6.1) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: "Michael W. Bombardieri" , qemu-devel@nongnu.org, Stefan Hajnoczi 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 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