Re: [Qemu-block] [PATCH 3/3] coroutine: add x86 specific coroutine backend

[Paolo Bonzini]

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