[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/3] coroutine: add x86 specific coroutine backe
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-block] [PATCH 3/3] coroutine: add x86 specific coroutine backend |
Date: |
Wed, 13 Mar 2019 18:12:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
On 13/03/19 15:14, Kevin Wolf wrote:
>> + /* Immediately enter the coroutine once to pass it its address as the
>> argument */
>> + co->base.caller = qemu_coroutine_self();
>> + start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>> + CO_SWITCH(current, co, 0, "jmp coroutine_trampoline");
>> + finish_switch_fiber(fake_stack_save);
>> + co->base.caller = NULL;
> ...but why is this necessary? CO_SWITCH() always passes the coroutine in
> rdi, not just here, so wouldn't the first real call do this, too?
>
> Ah, I see, because of the 'jmp coroutine_trampoline'. But the comment is
> really misleading. Actually, I think the code would become simpler if
> you just put the address of coroutine_trampoline on the initial stack
> and then have 'ret' unconditionally (see below for a quick attempt at
> something to squash in).
Actually, it becomes even simpler if I do "call coroutine_trampoline".
Then I don't have to fiddle with the stack pointer anymore in order to
correct the alignment: the ABI says that %sp must be aligned before the
call, and it already will be if I let "call" push the return address.
It's true that then I wouldn't really need the CO_SWITCH at all here,
but leaving it there might make it a bit simpler to port to other
architectures, by removing the target-dependent stack pointer manipulation.
I'll fix the comment though.
>> + return &co->base;
>> +}
>> +
>> +#ifdef CONFIG_VALGRIND_H
>> +#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>> +/* Work around an unused variable in the valgrind.h macro... */
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>> +#endif
>> +static inline void valgrind_stack_deregister(CoroutineX86 *co)
>> +{
>> + VALGRIND_STACK_DEREGISTER(co->valgrind_stack_id);
>> +}
>> +#if defined(CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE) && !defined(__clang__)
>> +#pragma GCC diagnostic pop
>> +#endif
>> +#endif
> Another candidate for sharing instead of duplicating? (You could
> trivially pass the valgrind_stack_id instead of the CoroutineX86
> object.)
Yes, good idea.
Paolo
- [Qemu-block] [PATCH 0/2] coroutine: add x86 specific coroutine backend, Paolo Bonzini, 2019/03/11
- [Qemu-block] [PATCH 1/3] qemugdb: fix licensing, Paolo Bonzini, 2019/03/11
- [Qemu-block] [PATCH 3/3] coroutine: add x86 specific coroutine backend, Paolo Bonzini, 2019/03/11
- [Qemu-block] [PATCH 2/3] qemugdb: allow adding support for other coroutine backends, Paolo Bonzini, 2019/03/11
- Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] coroutine: add x86 specific coroutine backend, no-reply, 2019/03/11
- Re: [Qemu-block] [PATCH 0/2] coroutine: add x86 specific coroutine backend, Stefan Hajnoczi, 2019/03/13
- Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] coroutine: add x86 specific coroutine backend, Peter Maydell, 2019/03/13