[Qemu-devel] broken win32 coroutines (was Re: qemu 1.6.1)

[Paolo Bonzini]

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 = &current;

    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